Java, Exceptions – Exception Treatment with or without Recursion

exceptionsjava

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?

Best Answer

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.

Related Topic