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.
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:
Post a Comment