As you may have noticed, I haven’t blogged much recently. About a year and a half ago, I took on a long-term project that has soaked up all of my free time, and then some. Apologies to anyone who came here looking for content; I intend to post more frequently now.
I had thought the concept in the title of this post, and the idea of avoiding it, was so widespread that it didn’t bear repetition. Apparently not; today’s post is directly inspired by some code I came across this afternoon while debugging a problem at work.
The code snippet that caught my attention was along these lines:
SomeBooleanVariable = Replace(StringVariableToCheck, ” “, “”).StartsWith(“ExpectedText”)
My initial reaction was “Wow.”
Now, of course, anyone with a year or more experience with VB.NET, or any other modern language, will quickly puzzle out the intent. But why should they have to? That is wasted time.
Isn’t it vastly more clear what you wanted to do when you write the code like this?:
SomeBooleanVariable = StringVariableToCheck.Contains(“ExpectedText”)
It should be obvious to any .NET programmer, even to non-programmers, what your intent is.
Even the Replace() function is disguising intent here. It’s not needed for the comparison the way I re-wrote it, so I took it out. However, even in a situation where you need to remove the whitespace, I wouldn’t use Replace(). Replace() implies you want to change the contents of the string variable, which is not at all what we’re trying to accomplish! Instead, use the function designed for this purpose: the Trim() method of the String object.
It may seem over-zealous to worry about something like this, but after suffering through dozens of similar types of constructions in the codebase I’m working on, I realized that the time I am wasting determining what the code is trying to do is adding up to a meaningful amount. Worse, it may cause someone to misinterpret the code, and introduce a bug while extending, modifying, or refactoring it.
I have purposely refrained from mentioning another issue in the above code to avoid distracting from my main point. However, in the interest of completeness, I should note it, as well: the string comparison as written is brittle, and not culture-aware. It is case-sensitive, which may or may not be what you want; either way, you would better declare your intent by using a function like String.Compare(), which has optional parameters that let you determine case-sensitivity while also declaring your intent to ignore capitalization or not; it also has the side benefit of allowing you to specify a culture. You may not be writing code that is designed to run in other cultures, but these days you are severely limiting your market if you are; and chances are high your code, if not your application, will wind up in another country whether you intended it to or not. If you’re creating websites for public consumption, you can’t really prevent it! Why not present the best possible representation of your code/company by making a few simple changes? Finally, ignoring culture can actually cause subtle bugs when you interact with user input.
For more on this, see
Best Practices for Using Strings in the .Net Framework.
See you next time.
