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

8

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.

answered Jun 02 '16 at 17:08

Robert Harvey

198,589
55
464
673

I see what you mean. I've added some details to further specify my situation. In the inner loop, I want to use the value of y, which is dependent on the iteration i (I'm actually using a for each, but this should get my point across). My inner catch will say something about the value of y when it is caught, while the outer catch is independent of the loop. Can I still get around the nesting, or am I locked in by my decision to give loop-dependent info? – Weasemunk Jun 02 '16 at 17:34
I still think I would refactor the inner loop into its own method, where you can do whatever you want with the exception. – Robert Harvey Jun 02 '16 at 23:10
Thanks, I combined your answer with @Martin Brendan's to produce a clean solution. – Weasemunk Jun 03 '16 at 17:42

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);
        }
    }
}

This answer made the most sense for my situation, as it allowed me to functionally separate my error catching rather than providing an alias for the internal catch, which would still happen anyway — Weasemunk, Jun 03 '16 at 17:36

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.

Thanks for the comment. 1+2 were not really relevant with respect to the actual code, but I think your perspective regarding throwing within the loop makes sense — Weasemunk, Jun 03 '16 at 17:33

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

3 Answers3