Advertisements
Posted by: jsonmez | October 19, 2010

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.

Here is the original code.

public static DataFileParser GetParser(DataFile dataFile)
{
    switch ((FileType)dataFile.ValidationFormat.FileType)
    {
        case FileType.PDF:
            return new PdfParser(dataFile);
        case FileType.Image:
            return new ImageParser(dataFile);
        case FileType.CsvWithHeaderRow:
            return new CsvParser(dataFile, true);
        case FileType.Csv:
            return new CsvParser(dataFile, false);
        default:
            throw new NotImplementedException("There is no parser for " + 
dataFile.ValidationFormat.FileType.ToString());
    }
}

 

Surprisingly simple solution

Try and say that 3 times fast.

I thought about this a bit and at first was having a hard time coming up with a solution.  Then I typed the code into an editor and realized how easy it is.

The trick here is that it looks like something other than the simple case of data mapping to logic, but it isn’t.

  • The logic in this case is the creation of the Parser. 
  • The data is the file type.

Once you think of it in those terms you can easily solve it using the pattern I mention in my previous post on the subject

Switch be gone!

private static Dictionary<FileType, Func<DataFile, DataFileParser>> DataFileTypeToParserCreatorMap = 
    new Dictionary<FileType, Func<DataFile, DataFileParser>>()                                                                                            
{ 
    {FileType.PDF, file => new PdfParser(file)},
    {FileType.Image, file => new ImageParser(file)},
    {FileType.CsvWithHeaderRow, file => new CsvParser(file, true)},
    {FileType.Csv, file => new CsvParser(file, false)}
};

public static DataFileParser GetParserRefactored(DataFile dataFile)
{
    Func<DataFile, DataFileParser> parserCreator;
    if(!DataFileTypeToParserCreatorMap.TryGetValue(
                             dataFile.ValidationFormat.FileType, 
                             out parserCreator))
        throw new NotImplementedException("There is no parser for " +
            dataFile.ValidationFormat.FileType.ToString());

    return parserCreator(dataFile);
}

 

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.

private static Dictionary<FileType, Type> DataFileTypeToParserTypeMap = new Dictionary<FileType, Type>()
{
    { FileType.PDF, typeof(PdfParser)},
    { FileType.Image, typeof(ImageParser)},
    { FileType.CsvWithHeaderRow, typeof(CsvWithHeaderParser)},
    { FileType.Csv, typeof(CsvParser)}                 
};

public static DataFileParser GetParser(DataFile dataFile)
{
    Type parserType;
    if(!DataFileTypeToParserTypeMap.TryGetValue(
            dataFile.ValidationFormat.FileType, 
            out parserType))
        throw new NotImplementedException("There is no parser for " +
                    dataFile.ValidationFormat.FileType.ToString());

    return (DataFileParser) Activator.CreateInstance(parserType, dataFile);
}

 

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.
Advertisements

Responses

  1. In the first GetParserRefactored, I think you want to call TryGetValue on the dictionary… Checking for null wouldn’t help because the dictionary will already have thrown an exception.

    • 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.)

      Anyway, thanks for the heads up. I can’t slip anything by you! 😉

      • 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());
        }

        return parserCreator(dataFile);

      • Actually it is

        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.)

      • Note to self: No coding after 3 AM 😛

        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…

  2. 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.

    • Mostly your comment makes sense, except this part: “Not to mention that the static dictionary property, might not be in the same location as the method”.

      Who’s writing your code? And why are they moving stuff around to make them less readable?

  3. 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.

    • Your welcome, I am glad it helped you. 🙂

  4. 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.

  5. […] Refactoring Switches Advanced […]


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

Categories

%d bloggers like this: