Convert key search to lower case

[chivalry]

As far as I can tell, the check for the key being in the instance cache will never be executed. You're creating fflib_SObjectDescribe instances in three places:

• fflib_SObjectDescribe.getDescribe(String) (line 193)
• fflib_SObjectDescribe.getDescribe(Schema.SObjectType) (line 202)
• fflib_SObjectDescribe.getDescribe(Schema.DescribeSObjectResult) (line 210)

Each time you're only doing so after you've checked if the token (lower cased) is in the instance cache and only create the new instance if it's not.

But maybe you're directly instantiating this class in other places. If not, the check (and the exception) can be removed. If so, then this check should have the token converted to lower case.

---

This change is 

[chivalry]

I'll look into doing that, but it's not actually simple to do so. My actual point about this is that the check, as it currently is written, doesn't have a code path that will execute line 81, and it might be better to simply remove lines 80 and 81. If I'm incorrect about that, then maybe it makes sense to keep the check for throwing the error, but this check makes sure the key isn't already in the instanceMap, but each time you create a new instance of fflib_SObjectDescribe, you've already checked that there isn't.

[PeterLin888]

Thank you, @chivalry, for this pull request!! :)

It seemed to me that this pull request #364 is a good fix as the Anonymous Apex below. CC: @daveespo @ImJohnMDaniel @stohn777 @wimvelzeboer

Map<String, String> testMap = new Map<String, String>();
// See Line 83
testMap.put(String.valueOf(Account.SObjectType).toLowerCase(), 'toLowerCase');
System.debug('testMap= ' + testMap); // DEBUG|testMap= {account=toLowerCase}
String testStringContainsKey = String.valueOf(Account.SObjectType); // DEBUG|testStringContainsKey= Account
System.debug('testStringContainsKey= ' + testStringContainsKey);
// Fix from this pull request at Line 80
String fixTestStringContainsKey = String.valueOf(Account.SObjectType).toLowerCase(); // DEBUG|testStringContainsKey= account
System.debug('testStringContainsKey= ' + fixTestStringContainsKey);
// Fix from this pull request at Line 80 See Line 80 if(instanceCache.containsKey( String.valueOf(token).toLowerCase() ))
System.assert(testMap.containsKey(fixTestStringContainsKey), 'Fix from this pull request at Line 80 if(instanceCache.containsKey( String.valueOf(token).toLowerCase() ))');

// See Line 80 if(instanceCache.containsKey( String.valueOf(token) ))
System.assert(testMap.containsKey(testStringContainsKey), 'Test Line 80: if(instanceCache.containsKey( String.valueOf(token) ))');

// Error on line 14, column 1: System.AssertException: Assertion Failed: Test Line 80: if(instanceCache.containsKey( String.valueOf(token) ))
// AnonymousBlock: line 14, column 1

[wimvelzeboer]

Hi @chivalry The constructor is private and therefore cannot be called from any other place. There are currently just the three places you mentioned. All three are doing a toLowerCase operation first prior to checking if the value is already in the instanceCache. The keys of this map are all in lower case.

So, I agree with you that line 80 doesn't really make sense. In the current code, it will never find a duplicate, and never hit that exception. That exception is indeed dead-code.

And I agree with you, we either should add the toLowerCase in anticipation of future changes to this class or remove the IF condition and exception entirely.

@daveespo This doesn't seem to be a bug nor can we hit this piece of code with anonymous apex, its more a matter of cleaning up or removing dead-code.

My preference would be to remove the lines 80-81.

[chivalry]

My preference would be to remove the lines 80-81.

Me too, actually, but that may be because part of my job is primary code reviewer on our project, and I'm kind of nitpicky about things like that.