Unnecessary Complexity Case Study #1: Untyped Enum Comparisons

This post is the first in a series of posts on specific examples of unnecessary complexity.  

Consider this code:

enum MyEnum  { First, Second }
public void Match1(MyEnum e)
{
    if (e.ToString() == "First") { }
}

The problem lies in the condition of the if.  The developer is converting an enum instance to a string for the purposes of a comparison, circumventing type safety.  The primary problem here is that the statement can no longer safely participate in automated refactorings.  For example, if my MyEnum.First is renamed this code will break such that the condition is always false and the body will never execute.  A Find Usages executed on MyEnum.First would not identify this site.  

It also performs worse than the alternatives.  I hesitate to even mention that though because, like most micro performance optimizations, it is only going to matter in uncommon situations like a tight loop or heavy load.

The correct code is this:

if(e == MyEnum.First) { }

The code is now typesafe, fast (it’s a comparison of integers), and refactoring friendly.  And it’s a simple enough fix.  The exact steps are:

1. Scan the enum values list for a member whose name exactly matches the string literal.
2. If found, replace the string literal with a reference to the qualified enum value.

There are some special cases to consider as well.

Here the string is being manipulated before comparing, by calling ToLower().

if(e.ToString().ToLower() == "first") { }

When comparing the result of a ToLower() or ToUpper() call the string constant is single cased, likely with the intention of reducing typos.  I suspect the programmer considered this defensive programming.

Unnecessary complexity begets bugs. When your code base is littered with untyped enum comparisons, this mistake is inevitable:

if(e.ToString().ToLower() == "First") { }

The string constant actually matches the declared enum casing, yet due to the ToLower() call the condition is always false, and the branch is never executed.  This permutation is particularly dangerous as the fix revives a branch that hasn’t been executing. You are eliminating a side effect, and a danger of eliminating side effects is breaking other parts of the system that could be dependent on them.

I have created a tool for automating the correction of untyped enum comparisons.  It’s a Resharper plug-in based quick fix and is available as part of my Agent Ralph project.

Using Agent Ralph to automate a Make Enum Comparison Typesafe code correction.

Currently it only handles the simple case, pictured above.  I hope to correct that deficiency soon.

Thanks to Mark for the great suggestion on a WordPress friendly code syntax highlighter.