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

  • 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?
                }
            }
        }
    

    Can you give us us case for this? Why can't you handle every error type in the top level catch?

    I've seen this kind of code recently, performed by unexperienced programmers who don't really know what they are calling inside the try blocks, and they don't want to bother testing the code. In the code sample I've seen, It was the same operation but performed each time with fallback parameters.

    @LokiAstari -Your example is a try in the Finally Section.. Where there is no Catch. This is a nested in the Try section.. It's different.

    Why should it be an anti-pattern?

    +1 for "more try catches?"

    It's not an antipattern, it's only ugly.

    I have a situation where I am working in an EJB. Every exception that gets thrown out of the EJB gets wrapped in an EJB Exception, and the real cause ends up halfway down a 500 line stack track. Java prohibits you from having a multiple exception catch with the catch-all Exception. I am going to wrap important sections in a try-catch as you proceed through the process method, then catch the exception to report the specific issue, then rethrow it to the outer exception handler.

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

    Not pretty, but sometimes there are no alternatives.

    Well, you could do the try-catch one after another, without nesting.

    @MisterSmith - Not always.

    Could you find an example of such a situation?

    if you do the try catch one after another then surely the code inside the second try will be executed even if the first catch clause is not invoked?

    @AndyC - And if your nested `catch` throws?

    Yes this is sort of what I was trying to get at. Of course there comes a point in your nested try/catch statements where you just have to say enough is enough. I was making a case for nesting as opposed to sequential try/catch's, saying that there are situations where you only want the code inside the second try to execute if the first try falls over.

    @Oded Even if the second catch throws, i don't see why it can't be done sequentially. Of course, it would need some boolean flags to check if the previous try-catch succeeded.

    @MisterSmith - If the point of nesting `try{}catch{}` blocks is to avoid bubbling the exception, then having a nested `catch{}` throw will not work.

    @MisterSmith: I'd prefer nested try-catches to sequential try-catches that are partially controlled with flag variables (if they were functionally the same).

    try{ transaction.commit(); } catch { try{ transaction.rollback(); } catch { seriouslogging() } notsoseriouslogging(); } is an example of a necessary nested try catch

    @Thanos good example.

    @MisterSmith thanks, its something you'll get to see everyday... :) I also just added the example as an answer.

    A good exemple of such place is logging an error. First you try to log it into DataBase if it fail you log it to a file

    At least extract the catch block to a method, guys! Let's at least make this readable.

  • 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...

  • 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"
  • 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.

  • 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.

  • Not an anti-pattern per se, but a code pattern that tells you need to refactor.

    And it's pretty easy, you just have to know a rule of thumb which is writing no more than a try block in the same method. If you know well to write related code together, usually is just copying and pasting each try block with its catch blocks and pasting it inside a new method, and then replace the original block with a call to this method.

    This rule of thumb is based on Robert C. Martin's suggestion from his book 'Clean Code':

    if the keyword 'try' exists in a function, it should be the very first word in the function and that there should be nothing after the catch/finally blocks.

    A quick example on "pseudo-java". Suppose we have something like this:

    try {
        FileInputStream is = new FileInputStream(PATH_ONE);
        String configData = InputStreamUtils.readString(is);
        return configData;
    } catch (FileNotFoundException e) {
        try {
            FileInputStream is = new FileInputStream(PATH_TWO);
            String configData = InputStreamUtils.readString(is);
            return configData;
        } catch (FileNotFoundException e) {
            try {
                FileInputStream is = new FileInputStream(PATH_THREE);
                String configData = InputStreamUtils.readString(is);
                return configData;
            } catch (FileNotFoundException e) {
                return null;
            }
        }
    }
    

    Then we could refactor each try catch and in this case each try-catch block tries the same thing but in different locations (how convenient :D), we have only to copy paste one of the try-catch blocks and make a method of it.

    public String loadConfigFile(String path) {
        try {
            FileInputStream is = new FileInputStream(path);
            String configData = InputStreamUtils.readString(is);
            return configData;
        } catch (FileNotFoundException e) {
            return null;
        }
    }
    

    Now we use this with the same purpose as before.

    String[] paths = new String[] {PATH_ONE, PATH_TWO, PATH_THREE};
    
    String configData;
    for(String path : paths) {
        configData = loadConfigFile(path);
        if (configData != null) {
            break;
        }
    }
    

    I hope that helps :)

    good example. this example is truly the kind of code we have to refactor. however some other times, nested try-catch is necessary.

  • It is surely decreasing the code readability. I would say, if you have chance, then avoid nesting try-catches.

    If you have to nest try-catches, always stop for a minute and think:

    • do I have the chance to combine them?

      try {  
        ... code  
      } catch (FirstKindOfException e) {  
        ... do something  
      } catch (SecondKindOfException e) {  
        ... do something else    
      }
      
    • should I just simply extract the nested part into a new method? The code will be much more clean.

      ...  
      try {  
        ... code  
      } catch (FirstKindOfException e) {  
         panicMethod();  
      }   
      ...
      
      private void panicMethod(){   
      try{  
      ... do the nested things  
      catch (SecondKindOfException e) {  
        ... do something else    
        }  
      }
      

    It is obvious if you have to nest three or more levels of try-catches, in a single method, that is a sure sign of time for refactor.

  • 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.

  • 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();
    
  • 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.

License under CC-BY-SA with attribution


Content dated before 6/26/2020 9:53 AM