Exception treatment with/without recursion

Question

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?

Answer 1

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.

Answer 2

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.

Answer 3

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.

Answer 4

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.