Advertisements
Posted by: jsonmez | June 25, 2010

Don’t Chain Failure States in Returns

I had originally started writing this post, thinking that I knew that answer to the problem I am about to demonstrate.

What surprised me is how much more clean a try catch solution seems to work than my original solution.

The unclean code

public void Process()
{
    foreach (string line in file)
    {
        string[] columns = line.split(',');
        foreach (string column in columns)
        {
            if (hasBadData(column))
            {
                LogErrorMessage();
                return;
            }

            Print(column);
        }
    }
}

I am using a short example here, so the refactoring might not really jump out to you as necessary, but let’s refactor it to be as clean as possible anyway.  (This comes from a much larger example where the refactoring is a must.)

Refactor 1: Returning booleans

public void Process()
{
    foreach (string line in file)
    {
        if (!ProcessLine(line))
        {
            return;
        }
    }
}

private bool ProcessLine(string line)
{
    string[] columns = line.split(',');
    foreach (string column in columns)
    {
        if (!ProcessColumn(column))
        {
            return false;
        }
    }
    return true;
}

private bool ProcessColumn(string column)
{
    if (hasBadData(column))
    {
        LogErrorMessage();
        return false;
    }

    Print(column);
    return true;
}

We’ve broken each loop into its own method and separated the handling of each line and each column in each line into its own method.  A good refactor that makes things a little more clear.

Notice the bool return types on the new methods and the special handling to return early if a result is false.  Most people when refactoring the first example (myself included) forget that the first example breaks out of both loops with an early return.

If you don’t take this into account, your behavior will be different.  When you refactor, your behavior had better not be different.

This is the crux of the problem.  Returning bools is bad.  It doesn’t make sense that our methods return bool.  What does the bool mean?  We happen to know it means success or failure, but will the next reader?  It seems very out of place, and look at the verbosity to handle the return types.

Refactor 2: Storing error state

This next refactor was what this post was going to be titled.  I was going to suggest in this post that you should not return boolean error states, but instead use the error state as part of your class.  I still think this is a decent solution.

private bool IsFatalError { get; set; };

public void Process()
{
    foreach (string line in file)
    {
        ProcessLine(line);
        if (this.IsFatalError)
        {
            return;
        }
    }
}

private void ProcessLine(string line)
{
    string[] columns = line.split(',');
    foreach (string column in columns)
    {
        ProcessColumn(column);
        if (this.IsFatalError)
        {
            return;
        }
    }
}

private void ProcessColumn(string column)
{
    if (hasBadData(column))
    {
        LogErrorMessage();
        this.IsFatalError = true;
        return;
    }

    Print(column);
}

What we have done here is stored some state information about our object instead of chaining result codes down the method calls.  This is a huge improvement, because our meaning is much more explicit and our object is holding its own state.

It still is quite verbose.  Why do we have to check for a fatal error everywhere?  Seems like we are likely to forget.

Still, much better than returning booleans.  If you are in a class don’t be afraid to manipulate the state of that class internally.  Many times you can eliminate return values and parameters passed into private methods in a class by simply making that data part of the class.

So, the verbosity was still bothering me.  So, I tried this:

Refactor 3: Exception handler flow control

public void Process()
{
    try
    {
        foreach (string line in file)
        {
            ProcessLine(line);
        }
    }
    catch (BadColumnDataException exception)
    {
        LogErrorMessage();
    }
}

private void ProcessLine(string line)
{
    string[] columns = line.split(',');
    foreach (string column in columns)
    {
        ProcessColumn(column);
    }
}

private void ProcessColumn(string column)
{
    if (hasBadData(column))
    {
        throw new BadColumnDataException();
    }

    Print(column);
}

This is surprisingly clean.  Notice that we don’t actually have to put if conditions in the loops to check whether or not we should return early?  Instead, if an exception is thrown, flow will jump immediately to the catch block where we will log the error message.

I’m a little bit uncomfortable to be using exception handling as a flow control mechanism, but given that:

  1. When encountering the error, we are aborting the normal flow of the program completely (breaking out of two loops).
  2. It is so much more clean and clear.

I think this is actually my preferred solution.  I normally would never use exceptions where I could reasonably detect the failure condition, but I may have to rethink that viewpoint.

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. I like refactor #3. I did something very similar recently and was wondering if it was an atrocity or not.

    • Yeah, I kinda shyed away from 3 at first, still I had actually turned #2 into #3 and cut out so much code. (In the real implementation).
      Then there was no question in my mind that even though #3 feels a little wrong, that it is the best solution.

  2. […] 26, 2010 by handcraftsman I ran across this thoughtful post by John Sonmez showing a revision process whose goal was “refactor it to be as clean as […]

  3. Good topic. I started to respond here but then it started to get long so I turned it into post. http://handcraftsman.wordpress.com/2010/06/26/a-refactoring-exercise-printing-file-columns/

  4. […] Don’t Chain Failure States in Returns (John Sonmez) […]

  5. That doesn’t seem like using exceptions for flow control to me. At least, not in the evil sense. Since the method with the try/catch is just returning after catching the exception, it looks more like just normal exception handling.

    In my mind, using exceptions for flow control looks more like something like this obviously contrived example:

    public void DoSomething()
    {
    int x;
    try
    {
    x = GetValueOfX();
    }
    catch
    {
    x = 5;
    }

    // do something meaningful to X
    }

  6. […] the situation it’s not an exception and there are no exceptions to that rule.  (Okay, there is one exception I blogged about earlier, but it makes the code so much […]


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: