Today I was forced to read some code for a developer that I loathe. Just when I believe I’ve seen every stupid thing you can do with code I am surprised by a new structure that he repeats everywhere through his code. Here’s what I saw today…
string myString = (GetStringFromConfiguration(”keyvalue”) + “”).ToLower();
At first I did a double take, it doesn’t make sense to add an empty string to a string … that is unless you’re trying to prevent the null reference exception from ToLower(). Adding null to any string results in that string. Thus, you *CAN* work around null references by using this technique. However, here is why you shouldn’t.
When you do this you actually force the CLR to create an extra, unnecessary new string object. It creates a temporary string object which it concatenates the result of the function and the empty string into. Another string is created when the string is taken ToLower(). That one can’t be helped, but the intervening one — the one caused by the concatenation, can be eliminated.
OK, sure, it’s a detail and it probably won’t matter much to most people but it will perform slower, require more memory, etc. The biggest thing for me is that it’s really odd from a readability/understandability perspective. (Did I mention that there are no comments in this code?)
Ideally, I’d create a GetStringFromConfiguration function overload that took a second parameter of what I wanted back if the entry wasn’t in the configuration file. That way the code doesn’t concern itself with the details, it knows that it will always get back a valid string. This is in fact what I do when I have to be concerned about getting back a null when I need it to be functionally equivelent of a default or an empty string.
As a sidebar, it would have been nice to see a String.Empty instead of ““ … oh well, you can’t win them all.