Don’t Chain Failure States in Returns

Written By John Sonmez

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

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

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.

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

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.