Refactoring Switches Advanced

KevDog posted a question in response to my post Pulling out the Switch: It’s Time for a Whooping and I thought it would be good to go ahead and answer it as a post since it is a pretty interesting real world example of a somewhat difficult switch statement to get rid of.

That is the quickest solution that preserves the existing code as much as possible.

Another solution

With the first solution we pushed the object creation into the map.

If we can make the constructor for all the parsers the same, we can use reflection to dynamically create our instances by looking up the type in the dictionary.

In this example, I assume that we have refactored CsvParser to have a constructor that only takes one parameter and internally sets a value of usesHeader to false, and we have created a CsvWithHeaderParser that inherits from the CsvParser and sets usesHeader to true.

Pretty similar solution. I prefer the first though for several reasons:

The refactor is localized, where the second solution has to touch other classes.

Reflection makes you lose compile time safety.

You may create a new parser that you want to have more parameters for the constructor. With the second solution, you will have a hard time doing that.

The first solution gives you ultimate flexibility in setting up the constructor of the parser. If you wanted to do 5 steps for a particular parser, you could.

Anyway, next time you run into a switch statement that is hard to figure out how to refactor, try to break it into a mapping between data and logic. There is always a solution.

As always, you can subscribe to this RSS feed to follow my posts on Making the Complex Simple. Feel free to check out ElegantCode.com where I post about the topic of writing elegant code about once a week. Also, you can follow me on twitter here.

Yeah, you are correct. I guess I called the code like I wished it would work.
Why does .NET have to throw an exception if something isn’t in a dictionary?
It requires a call to check if the thing is there and a call to get it, which requires two lookups.

Much better to just return null if it isn’t there. (I believe that is the Java implementation of Map, but I could be wrong.)

Yes, it is the Java implementation. And no, you can’t slip anything by me!

The correct idiom to use in C# is almost always TryGetValue – but it doesn’t require two calls!

Func parserCreator;
if (!TryGetValue(DataFileTypeToParserCreatorMap, out parserCreator)) {
throw new NotImplementedException(“There is no parser for ” +
dataFile.ValidationFormat.FileType.ToString());
}

Func parserCreator;
if(!DataFileTypeToParserCreatorMap.TryGetValue(dataFile.ValidationFormat.FileType,
out parserCreator))
throw new NotImplementedException(“There is no parser for ” +
dataFile.ValidationFormat.FileType.ToString());

return parserCreator(dataFile);

Ick, I hate using out though. Hmm, the only other alternative is to check for the value (also yuck), or catch an exception (even more yuck.)

Yes, I hate using out too, for a few reasons: First, I have declared a variable on a different line than the one where I set its value. Second, I’ve introduced code that looks mutable (value is set apart from declaration) when it isn’t. Third, I’ve lost any chance of using the value in the same line and making a readable expression.
There are problems with returning null when there is no value – sometimes there’s a difference between the two. But I usually have my public class DefaultValueDictionary : Dictionary {
public override V this[K key] {
// at least I think it was an override; maybe it was new. Can’t remember now.
get { V result; TryGetValue(key, out result); return result; }
set { base[key] = value; }
}

Since TryGetValue does promise to set the default value when the key isn’t found, I don’t even have to check its return value here…

Honestly, I would stick with the switch statement in this scenario. It’s fewer lines of code, much more readable, executes faster, most likely optimized by the compiler, and is self documenting.

I always try and keep in mind that developers that come along behind me and work on my code may not have the experience that I do. If I’m debugging and I see code like this refactoring; I’m going to slow down on this method to determine what the original developer was trying to do. Not to mention that the static dictionary property, might not be in the same location as the method (They might have to track that down just to determine what the next method being called is). Whereas the switch statement is obvious.

Anyone who’s picked up a development book can understand the first one.

I wouldn’t recommend this to anyone, but it’s fun in an academic sense. Very interesting refactoring.

You are correct. Probably in my next post though, I will show why this really is a better solution.
If you can make the parser self-register themselves in the dictionary, you can really get the benefit of using a dictionary vs. a switch.
In a bigger code base, I would assume that this kind of refactor would have a larger benefit if you have switch statements sprinkled throughout the code.

Thanks for the help with my little coding problem, I very much appreciate the knowledge drop. I was mushing around with reflection at first, which I hated for the reasons you mentioned, it just seemed like too much to go through for something like this.

If things aren’t going to change at runtime, the switch is by far the cleanest.
Also, in your first method you should but the new in a lambda so that they are created lazily in case instances have any state, assuming they are flyweight and just call what are basically static methods anyway.
By the time you do one of two options you’ve outlined correctly, you’re just re-inventing your own IOC container – so you may as well use one.