Is nesting try-catch statements still a code smell if nested within a loop?

Question

I have heard that nesting try-catch statements can often be a code smell, so I'm wondering whether this situation is an exception. If not, what would be some good ways to refactor?

My code looks like the following:

try{
    X x = blah;
    otherStuff;
    for (int i = 0; i < N; i++){
        try{
            String y = Integer.toString(i);
            f(x);
        }
        catch(Exceptions1 e1){
            System.err.println(... + y + ...);
            System.exit(0);
        }
}
catch(Exceptions2 e2){
    ...
}

I'm using Exceptions here to indicate a multi-catch.

e2 is used to catch exceptions thrown by initializing x and doing otherStuff. Ideally, I would have had my try-catch surround only those two lines, but I use x within my loop and wanted to avoid the "unused assignment" warnings caused by initializing to null outside the try-catch.

e1 was not multi-caught with e2 because I want to provide the user with information about iteration details and thus wanted a catch block within the loop.

score 8 · Answer 1 · answered Jun 02 '16 at 17:08

Unless you intend to process the entire inner loop whether an exception occurs or not, your code is essentially equivalent to

try{
    X x = blah;
    otherStuff;
    for (...){ 
       f(x)
    }
}
catch(Exceptions1 e1){
    ...
}
catch(Exceptions2 e2){
    ...
}

which does not require nesting.

If you still need the inner exception handling, refactor the inner exception and its enclosing method into a new method. This also has the added benefit of forcing you to think about other ways to process the inner loop; exceptions can become very expensive in terms of performance, if a lot of them are thrown.

score 7 · Accepted Answer · answered Jun 03 '16 at 13:30

If the e2 catch is, as you say, only to catch errors in initializing x and doing otherStuff you could extract it to a seperate method. This also seperates the logic nicely and allows you to potentially give a meaningful name to otherStuff.

public X Foo() {
    try{
        X x = blah;
        otherStuff;

        return x;
    }
    catch(Exceptions2 e2){
        ...
    }
}

public void Bar() {    
    X x = Foo();
    for (int i = 0; i < N; i++){
        try{
            String y = Integer.toString(i);
            f(x);
        }
        catch(Exceptions1 e1){
            System.err.println(... + y + ...);
            System.exit(0);
        }
    }
}

score 3 · Answer 3 · answered Jun 03 '16 at 11:35

I have a few observations on your question:

In the example you provided you don't need exceptions at all. You would only need to check the string y, log any error, and exit the loop. Nothing is exceptional about y being invalid and needing to be logged. The fact you are using exceptions in this case is indication of code smell.
Multi-Catch exceptions are not really meant to be used for wrapping large swaths of code and catching anything that might go wrong. They are meant to help distinguish between exceptions in the case where a single section of code can produce a series of different exceptions. Since you are using mutli-catch exceptions as a kind of "catch all" approach, this does indicate code smell.
It doesn't make any sense to put a try catch within the loop unless you plan on continuing to iterate through the rest of the loop even if an exception is found on one of the elements. Your sample code shows you plan to exit the loop if any one item is exceptional.

I think you might be better off with something like:

try
{
     var x = blah;
     DoStuff();
     foreach(var i in icollection) // i is an int
     {
          var y = SomethingDependentOnI(i);

          // check y for explicit exceptional states
          if (IsSomethingWrongWithY(y))
               throw new Exception1(y);
     }
}
catch (Exception1 e1)
{
}
catch (Exception2 e2)
{
}

This is much more clear, but it still suffers from the problems described above.

Is nesting try-catch statements still a code smell if nested within a loop?

3 Answers3