3

I've entered in an argument with a fellow colleague about a piece of code. While he thinks that it is ok, I don't. But I don't have any more arguments than that calling the same method (recursion) in a catch block is not a good practice.

So here is a pseudocode of what we are arguing about:

public String someMethod( SomeType param1, SomeType param2 ..., int attempts ){
    int max_attempts = 5;
    try{
        //doSomething here;
        return result;
    }catch (Exception e){
        if ( attempts == max_attempts ){
            throw new RuntimeException(e);
        }

        //log exception
        //sleep for some time

        //here is the part I do not agree.
        return someMethod( param1, param2 ..., attempts+1 );
    }
}

My suggestion is to have a structure like this one:

public String someMethod( SomeType param1, SomeType param2... ) 
                                           throws Exception /*or whatever*/{
     //do something
     return result;
}

public String executeProcess( SomeType param1, SomeType param2 ... ){
    int attempts = 0;
    boolean ok = false;
    String result;
    do{
        try{
            result = someMethod( param1, param2... );
            ok=true;
        }catch( /*specific exceptions here*/ e ){
            attempts++;
            //log exception
            //sleep for some time
        }
    }while( !ok || attempts<5 ); //just to be clear 5 here would be a constant

    if ( attempts >= 5 ){ 
        throw new RuntimeException(e);
    }
    return result;
}

In my solution could even evolve into something even more elegant like an executor class with a process list and priority, etc. (of course only if needed).

So which one of these is the best course of action and what would be the arguments to defend it (pros and cons).

What the someMethod does is to create a HttpURLConnection and send a POST request to a webservice that will return a JSON string which will then be handled and converted to an object outside the context of this method (the conversion part).

All exception handling will be based on the connection with the webservice.

What I forgot to mention here is that his version of the code has already been in production for a long time, therefore there aren't any "programming bugs". The only exceptions that have been thrown are ones concerning the connection/timeouts (whatever) related to the webservice.

I even talked to him to go the log (history) file and get all specific exceptions and put in the catch block.

Considering that the exceptions that have been handled will be the specific ones, what would be the best solution?

  • Could you describe what this method actually does and what the exceptions mean? Your first code snippet is something I actually did once, but only as a short-term desperate way to try and catch more information about a rare but serious production crash; context matters a lot. Also consider a third option where someMethod returns null to indicate failure so your looping code can be much simpler. – Ixrec Feb 18 '16 at 13:13
  • Hi there @Ixrec Thanks for taking the time. I've updated the question with a context about someMethod – Jorge Campos Feb 18 '16 at 13:20
5

It looks like the intent is to try 5 times, logging the problem each time. If none of the attempts succeed, give up after 5 tries. I'll label this the scaffold, and I'll call the thing to try 5 times the core functionality.

Let's look at the two versions with reference to some coding guidelines from around the web. For simplicity, I'll call versions R for the recursive version and S for the non-recursive version because it has separate code for the scaffold and the core functionality.

S conforms better to SOLID principles than R, in particular to the single responsibility principle, but also to some extent to the Liskov substitution principle.

  • single responsibility - separating the scaffold from the core functionality allows each to concentrate on their own intricacies.; and

  • Liskov substitution - separating out the core functionality allows other core functionality to be used with the same scaffold, as you observe.

Your non-recursive version embodies the coding intent more clearly and has less mingling between core functionality and exception code. This helps with code review and maintenance. To improve on it, you can consider using another of the SOLID principles: dependency inversion. Instead of having the generic executeProcess depend on the concrete someMethod, add a call-back as a parameter toexecuteProcess and call executeProcess with (an encapsulated form of) someMethod. This allows you to put executeProcess into a generic library rather than copy-paste-modify it for each new someOtherMethod that might be introduced later.

Mingling business logic with exception handling is the first warning in this list of best practices for exception handling. Even the sleep can be considered part of the clean-up, perhaps relegated to a finally block or placed outside the exception mechanism altogether.

Another issue with R is that the recursion (into the code that threw the exception) occurs before the exception is fully handled.

Your refactored version is the better option of the two.

  • Thanks for the input! The sleep part is just to the retry logic because most of the errors is about timeouts or network problems. If it wasn't there, most probably the retry attempts will fail all attempts. – Jorge Campos Feb 18 '16 at 15:05
  • @JorgeCampos I understand and have edited my answer. Hope it's clearer now. – Lawrence Feb 18 '16 at 15:08
  • 1
    Downvoting this as it's an opinion answer (and I disagree with the opinion expressed, which is the problem with opinion answers). – David Arno Feb 18 '16 at 15:29
  • It might have other flaws, but the recursive version looks way clearer and less cluttered to me. – Andres F. Feb 18 '16 at 21:00
  • 1
    That's great and well explained. I will accept it. – Jorge Campos Feb 19 '16 at 11:15
4

Both code example are bad, since you wait and retry regardless of the kind of exception throw. If the "do something" section throw an exception due to a programming bug or some other permanent error condition, then it doesn't make sense to retry. It should only retry in case of specific conditions where it makes sense, eg. timeouts from a service call.

That said, the first version is fewer lines of code and less complexity. I find it easier to read.

  • That is the part I forgot to mention, I even argued with him that catch just exception instead of specific ones is a bad practice. He said that it should catch whatever error and try again either way. I've understood that because this is in production for a long time and the "programming bugs" doesn't exists any more. Just the exceptions related with connections with the webservice. I talked to him to go the log (history) file and get all specific exceptions and put in the catch block. In that case what would be the best case? – Jorge Campos Feb 18 '16 at 14:09
  • I've updated the question adding this case. I would like to know about the self calling in the catch block. But you are totally right about the handling only specific exceptions. – Jorge Campos Feb 18 '16 at 14:17
2

There is another good reason not to use recursion here, beside the points raised above, which is that if the retry limit is reached, the exception that is eventually thrown will have a needlessly padded stack trace, which may make understanding the cause of the exception harder.

1

I faced this problem once, after digging into my old code, this is the "pattern" I used to handle this case:

public String someMethod(SomeType param1, SomeType param2, ...)
{
    for(int attempts = 1; attempts <= maxAttempts; attempts++)
    {
        try
        {
            return doSomeWebServiceCall();
        }
        catch(/*specific exception here*/ e)
        {
            if(attempts == maxAttempts)
            {
                throw new RuntimeException(e);
            }

            //log'n'sleep
        }
    }

    throw new RuntimeException("Unreachable");
}

I'm not particularly happy with the RuntimeException "unreachable" but otherwise, the code seems quite clear (even after years) for me. The most important idea here is to keep the logic related to the web service call independant from all exception handling/retry/sleep.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.