Tuesday, 30 August 2011

Checked Exceptions Part 3

I found a bug last week that was caused by a checked exception anti-pattern or maybe code smell. I name this smell "foo() throws Exception". The premise is that a method throws Exception, Throwable or any other exception which is low enough down in the class tree to capture the exceptions which may be thrown by the code that is being worked on. The software engineer happily beavers away without realising that the code is throwing all kinds of exceptions that are not being handled correctly. The handler is higher up the method chain and detached from what the code is actually doing so cannot reliably act, most likely it is simply logging exceptions. Once handled and logged nothing more is done possibly leaving the thread and/or application in an invalid state.

This can be caused by the programmer wanting the exception handling to go away perhaps because exceptions were used too liberally and now there is more exception handling than business logic. Maybe handling the exception is too uncomfortable since there is no plan B, there is only failure, but the application is long running and cannot be restarted so a catch-all was a last resort for an error that supposedly could not happen.

The application in question simulates a GSM-R application and calls are made to get the active call which the application is currently dealing with.  The other party could end the call at any time and the system is completely asynchronous and multi-threaded and it is highly likely that no call could exist the next time you check for it.  The exception being thrown was part of the null-checking strategy which has proved highly effective in reducing NPEs.  Instead of a null being returned from a method, a checked exception is returned: CallDoesNotExistException.  So this exception could happen in normal application flow and must be handled by application logic.

As it turned out the fix was remarkably elegant once the functionality as a whole was understood (read remembered).  A catch for CallDoesNotExistException was added to the catch-all handler which would navigate the user out of the process they were in to one of the two possible application states fixing similar issues in the entire application.  This change was helped by the fact that CallDoesNotExistException is task specific and other parts of the application could easily identify the issue in the code, as opposed to a generic exception which would be almost impossible to identify whether it was checked or not.

No comments: