Is using nested try-catch blocks an anti-pattern?

Question

Is this an antipattern? It is an acceptable practice?

    try {
        //do something
    } catch (Exception e) { 
        try {
            //do something in the same line, but being less ambitious
        } catch (Exception ex) {
            try {
                //Do the minimum acceptable
            } catch (Exception e1) {
                //More try catches?
            }
        }
    }

Answer 1

This is sometimes unavoidable, especially if your recovery code might throw an exception.

Not pretty, but sometimes there are no alternatives.

Answer 2

I don't think its an antipattern, just widely misused.

Most nested try catch's are indeed avoidable and ugly us hell, usually the product of junior developers.

But there are times you can't help it.

try{
     transaction.commit();
   }catch{
     logerror();
     try{
         transaction.rollback(); 
        }catch{
         seriousLogging();
        }
   }

Also, you'll need an extra bool somewhere to signify the failed rollback...

Answer 3

The logic is fine - it can make perfect sense in some situations to try a fallback approach, which could itself experience exceptional events.... hence this pattern is pretty much unavoidable.

However I would suggest the following to make the code better:

• Refactor the inner try...catch blocks out into separate functions, e.g. attemptFallbackMethod and attemptMinimalRecovery.
• Be more specific about the particular exception types that are being caught. Do you really expect any Exception subclass and if so do you really want to handle them all the same way?
• Consider whether a finally block might make more sense - this is usually the case for anything that feels like "resource cleanup code"

Answer 4

I don't think it's automatically an anti-pattern, but I'd avoid it if I can find an easier and cleaner way to do the same thing. If the programming language you're working in has a finally construct, that might help clean this up, in some cases.

Answer 5

I solved this situation like this (try-catch with fallback):

$variableForWhichINeedFallback = null;
$fallbackOptions = array('Option1', 'Option2', 'Option3');
while (!$variableForWhichINeedFallback && $fallbackOptions){
    $fallbackOption = array_pop($fallbackOptions);
    try{
        $variableForWhichINeedFallback = doSomethingExceptionalWith($fallbackOption);
    }
    catch{
        continue;
    }
}
if (!$variableForWhichINeedFallback)
    raise new ExceptionalException();

Answer 6

I've seen this pattern in network code, and it actually makes sense. Here's the basic idea, in pseudocode:

try
   connect;
catch (ConnectionFailure)
   try
      sleep(500);
      connect;
   catch(ConnectionFailure)
      return CANT_CONNECT;
   end try;
end try;

Basically it's a heuristic. One failed attempt to connect could just be a network glitch, but if it happens twice, that probably means the machine you're trying to connect to really is unreachable. There are probably other ways to implement this concept, but they'd most likely be even uglier than the nested tries.

Answer 7

I actually think it's an antipattern.

In some cases you might want multiple try-catches, but only if you DONT KNOW what kind of error you are looking, eg:

public class Test
{
    public static void Test()
    {            
        try
        {
           DoOp1();
        }
        catch(Exception ex)
        {
            // treat
        }

        try
        {
           DoOp2();
        }
        catch(Exception ex)
        {
            // treat
        }

        try
        {
           DoOp3();
        }
        catch(Exception ex)
        {
            // treat
        }
    }

    public static void Test()
    {
        try
        {
            DoOp1();
            DoOp2();
            DoOp3();
        }
        catch (DoOp1Exception ex1)
        {
        }
        catch (DoOp2Exception ex2)
        {
        }
        catch (DoOp3Exception ex3)
        {
        }
    }
}

If you don't know what you're looking for you HAVE to use the first manner, which is IMHO, ugly and not functional. I guess the latter is much better.

So, if you know what KIND of error you are looking for, be specific. No need for nested or multiple try-catches inside the same method.

Answer 8

I've "had" to do this in a test class coincidentially (JUnit), where the setUp() method had to create objects with invalid constructor parameters in a constructor that threw an exception.

If I had to make the construction of 3 invalid object fail, for example, I would need 3 try-catch blocks, nested. I created a new method instead, where the exceptions where caught, and the return value was a new instance of the class I was testing when it succeeded.

Of course, I only needed 1 method because I did the same 3 times. It might be not such a good solution for nested blocks that do totally different things, but at least your code would become more readable in most cases.

Answer 9

Yes, if you use them like this. :-)

private ParseResult<Statement> parseStatement(int p) {
    try {
        return (ParseResult) parseIfElseStm(p);
    } catch (ParseException e1) {
    try {
        return (ParseResult) parseIfStm(p);
    } catch (ParseException e2) {
    try {
        return (ParseResult) parseWhileStm(p);
    } catch (ParseException e3) {
    try {
        return (ParseResult) parseIterationStm(p);
    } catch (ParseException e4) {
    try {
        return (ParseResult) parseForStm(p);
    } catch (ParseException e5) {
    try {
        return (ParseResult) parseReturnStm(p);
    } catch (ParseException e6) {
    try {
        return (ParseResult) parseLocalDefStm(p);
    } catch (ParseException e7) {
    try {
        return (ParseResult) parseExpStm(p);
    } catch (ParseException e8) {
    try {
        return (ParseResult) parseBlockStm(p);
    } catch (ParseException e9) {
    try {
        // Try the empty statement
        parseChar(p++, ';');
        return new ParseResult<Statement>(EmptyStm.INST, p);
    } catch (ParseException e10) {
        throw new ParseException("expecting statement");
    }}}}}}}}}}
}

Answer 10

It's okay. A refactoring to consider is pushing the code into its own method, and using early exits for success, letting you write the different attempts to do something at the same level:

try {
    // do something
    return;
} catch (Exception e) {
    // fall through; you probably want to log this
}
try {
    // do something in the same line, but being less ambitious
    return;
} catch (Exception e) {
    // fall through again; you probably want to log this too
}
try {
    // Do the minimum acceptable
    return;
} catch (Exception e) {
    // if you don't have any more fallbacks, then throw an exception here
}
//More try catches?

Once you have it broken out like that, you could think about wrapping it up in a Strategy pattern.

interface DoSomethingStrategy {
    public void doSomething() throws Exception;
}

class NormalStrategy implements DoSomethingStrategy {
    public void doSomething() throws Exception {
        // do something
    }
}

class FirstFallbackStrategy implements DoSomethingStrategy {
    public void doSomething() throws Exception {
        // do something in the same line, but being less ambitious
    }
}

class TrySeveralThingsStrategy implements DoSomethingStrategy {
    private DoSomethingStrategy[] strategies = {new NormalStrategy(), new FirstFallbackStrategy()};
    public void doSomething() throws Exception {
        for (DoSomethingStrategy strategy: strategies) {
            try {
                strategy.doSomething();
                return;
            }
            catch (Exception e) {
                // log and continue
            }
        }
        throw new Exception("all strategies failed");
    }
}

Then just use the TrySeveralThingsStrategy, which is a kind of composite strategy (two patterns for the price of one!).

One huge caveat: don't do this unless your strategies are themselves sufficiently complex, or you want to be able to use them in flexible ways. Otherwise, you're larding a few line of simple code with a huge pile of unnecessary object-orientation.