Throwing exceptions to control flow - code smell?

Question

Consider this code (Java, specifically):

public int doSomething()
{
    doA();

    try { doB(); }
    catch (MyException e)
    { return ERROR; }

    doC();
    return SUCCESS;
}

Where doB() is defined as:

private void doB() throws MyException

Basically, MyException exists only in case doB() meets some condition (which is not catastrophic, but does need to somehow raise this condition) so that doSomething() will know to exit with an error.

Do you find the use of an exception, in this case to control flow, acceptable? Or is this a code smell? If so, how would you refactor this?

Answer 1

I've never liked using exceptions for control flow (in some languages, like the CLR, they're expensive).

If you can modify doB(), the best thing to do is change it to return a boolean that indicates success or failure, so your example would look like:

public int doSomething()
{
    doA();

    if (!doB()) {
        return ERROR;
    }

    doC();
    return SUCCESS;
}

Answer 2

I think it is very bad use of exceptions, plain and simple. I would create normal functions that return normal values and leave exceptions to what they are designed for.

Answer 3

Using exceptions to control flow, is definitely bad. Exceptions should be thrown only in exceptional conditions.

However, having a utility method that simply either throws an exception or does nothing is not particularly bad. For example, you may wish to validate all the arguments on entry to a method, or check that internal state is still valid between complex calculations (that for some reason you cannot force internally, perhaps you're accepting closures and executing them). In these cases it would not be a bad idea to extract the consistency checking into its own method, to improve the readability of the "actual" method implementation.

The dividing line is really that you should throw an exception when something outside of normal operating parameters is encountered, and things are so bad that there's no real way for the method to proceed with what it does.

As an example of what not to do, this would be using exceptions for flow control:

public int calculateArraySize(Object[] array)
{
   int i = 0;
   try
   {
      array[i++];
   }
   catch (ArrayIndexOutOfBoundsException ignore) {}
   return i;
}

I believe it will return the right result, but it will be horribly slow and inefficient, as well as difficult to read for people who are used to exceptions being used properly. ;-)

On the other hand, something like this would be OK in my opinion:

public void myMethod(int count, Foobar foo) throws MyPackageException
{
   validateArgs(count, foo);

   // Stuff
}

private void validateArgs(int count, Foobar foo) throws MyPackageException
{
   if (m_isClosed)
   {
      throw new IllegalStateException("Cannot call methods on a closed widget");
   }

   if (count < 0)
   {
      throw new IllegalArgumentException("count must be greater than zero");
   }

   foo.normalise(); // throws MyPackageException if foo is not fully initialised at this point
}

Despite the fact that all the second method does is potentially throw an exception, it's not doing this to control the program flow but is raising them in response to exceptional conditions.

Answer 4

Depending on doB's logic, you could have some return values pointing if it was ok or not, and then doSomething could use the returned value to handle the situation in an appropriate way.

Answer 5

It's ok to throw Exception in case of error, as in doB(). But problem is function doSomething().

You shouldn't use return statements to indicete success or failure. You should do:

try { 
    doB();
} catch (MyException e) {
    throw MyException2(e);
}

Answer 6

Is it really important for doC() to be executed when doB() fails? If not, why not simply let the Exception propagate up the stack to where it can be handled effectively. Personally, I consider using error codes a code smell.

Edit: In your comment, you have described exactly the scenarion where you should simply declare

public void doSomething() throws MyException

Answer 7

My only problem with the OP's code is that you're mixing paradigms -- doB shows an error by throwing an exception, while doSomething shows an error by returning a code. Ideally, you would pick one or the other. Of course, in legacy maintenance, you might not have that luxury.

If you pick returning error codes, that's OK, but I dislike it because it encourages you to use side channels (like global variables) to communicate state on failure, rather than bundling that state into an exception and letting it bubble up the stack until you can do something about it.

Answer 8

Exceptions are for non-standard behaviour. So yes it is ok to use them for controlling flow.

As Sun puts it:

Generally speaking, do not throw a RuntimeException or create a subclass of RuntimeException simply because you don't want to be bothered with specifying the exceptions your methods can throw.

Here's the bottom line guideline: If a client can reasonably be expected to recover from an exception, make it a checked exception. If a client cannot do anything to recover from the exception, make it an unchecked exception.

Just remember to not subclass RuntimeException but Exception as those are faster.

Answer 9

It entirely depends on what that error condition is, and what the method's job is. If returning ERROR is a valid way of handling that error for the calling function, why would it be bad?

Often, however, it is a smell. Consider this:

bool isDouble(string someString) {
    try {
        double d = Convert.ParseInt32(someString);
    } catch(FormatException e) {
        return false;
    }
    return true;
}

That is a very big code smell, because you don't expect a double value. You just want to know whether a string contains a double.

Sometimes, the framework you use doesn't have other ways of doing what you want. For the above, there is a better way:

bool isDouble(string someString) {
    bool success;
    Convert.TryParseInt32(someString, ref success);
    return success;
}

That kind of exceptions have a special name, coined by some dude whose blog i read recently. But sadly, i forgot its name. Please comment if you know it. Last but not least, the above is pseudo code. I'm not a c# developer so the above doesn't compile, I'm sure, but TryParseInt32 / ParseInt32 demonstrates that well i think so i'm going with C#.

Now, to your code. Let's inspect two functions. One smells, and the other doesn't:

1. Smell

public int setupSystem() {
    doA();

    try { doB(); }
    catch (MyException e)
    { return ERROR; } 

    doC();
    return SUCCESS;
}

That's a code smell, because when you want to setup a system, you don't want it to fail. Failing to setup a system means you can't continue without handling that error.

2. Ok

public int pingWorkstation() {
    doA();

    try { doB(); }
    catch (MyException e)
    { return ERROR; } 

    doC();
    return SUCCESS;
}

That is ok, because the purpose of that method is to test whether the workstation is still reachable. If it's not, then that is part of the result of that method, and not an exceptional case that needs an alternative return path.

Answer 10

Exceptions should be used when:

• a function cannot complete normally, and
• when there is no return value that can be used to indicate the failure, or
• when the thrown exception communicates more information than return FAILURE; can, for instance, nested exceptions or the like.

Remember above all that exceptions, like return values or method parameters, are simply messages sent between different parts of the code. Strive to optimize the balance between the information communicated via these methods, and the simplicity of the API. When a simple SUCCESS/FAILURE flag is all that's needed (and the method doesn't need to return other values), use that. If the method already must return a value, you usually need to use an exception (which is, to one way of looking at it, simply an "exceptional" return value from the method). If the failure information that must be communicated is too rich to be communicated in a return value (for instance, the reason for an I/O failure), use an exception.

Finally, error handling is a design decision, and there is no one-size-fits-all set of rules that will cover all cases.

Answer 11

As others have indicated, it really depends on the intended functionality of the methods involved here. If doSomething()'s role is to check something, returning a code that allows normal execution of its calling methods to continue (albeit on different paths depending on the code returned) then having it return that int may be fine. In that case, though, doB() should probably be consistent and return a boolean or something that doSomething() can check instead of throwing an exception which won't really do anything else.

If doB() were a public method of a another class that might have other uses outside of doSomething() (as opposed to a private method in the same class as you indicated) and it throws an exception and you want to use it in doSomething() (which has the same role indicated above), then your example would be fine.

The fact that the code you return when doB() throws an exception is called ERROR, though, indicates that it's probably a failure that would prevent whatever operation doSomething() is a part of from completing. In that case, you should probably not catch the exception thrown by doB() and just let it be thrown up the stack until it reaches a point where it can be handled or until it reaches your general error handler which can report/log it or whatever.

Answer 12

Here is how I would handle it:

public void doSomething() 
        throws MyException{
    doA();

    try { 
        doB();
    } finally {
        doC();
    }
}

If an error occurs in doB(), the exception will propagate up the stack.

Answer 13

You guys are killing me here. You NEVER want to use exceptions to control flow. It wouldn't be much different than using goto statments everywhere!

Exceptions are exception, meaning something un expected has happened. Something so much so we are halting the execution of the application. Anyone who has worked on an application were this type of exception flow control is baked in will want to hunt down the developer who did this and throttle him (or her).

Using exception as flow control makes maintainence and new feature development a nightmare!

Answer 14

Exceptions should not be used just to control flow. Checked exceptions should be used to tell calling code that certain conditions of the API contract were not met. I would not design doSomething to handle cases where calls to doB would fail often by using a try/catch block. If frequent failure was a case that you needed to deal with, I would design doB to return a success or failure boolean to indicate to its calling methods whether to continue on to their own doC-type methods:

public int doSomething() {
    doA();

    if ( doB() )
       doC();
       return SUCCESS;
    } else { 
       return ERROR; 
    }
}