Obscuring Your Intent in Code

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.

About these ads

  1. #1 by Jason on July 7, 2013 - 10:38 am

    Not that I have any idea of the intent (to your point); but, there is an instance where the initial replace is warranted:
    If “StringVariableToCheck” contained “Expected Text” instead of “ExpectedText” then you’d effectively get a false negative from your own alternative.

    Perhaps it was a fix for some other legacy code that may have place the space and, instead of fixing everything, simple kludged their way to greatness.

    • #2 by andrewcushen on July 11, 2013 - 10:47 am

      Jason-

      Yes, that’s a good point. I perhaps should have mentioned that, in the specific case I was addressing, it was a requirement that spaces in the middle of the string be treated as not matching. Spaces before and after were to be trimmed, but a space in the middle of the string was not allowed.

      And, of course, this was not mentioned anywhere in the code comments; as usual with this codebase, there were no documented requirements. So the fact that the code was not self-documenting and there were no comments was even worse. I had to puzzle this out for myself.

      I should add that the guilty party who wrote this code had a company-wide reputation for trying to show how “clever” he was by using as many different approaches to an end result as possible. Hence the title of this post.

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s

Follow

Get every new post delivered to your Inbox.

%d bloggers like this: