There a couple of exception handling patterns that I come across regularly that I believe to be plain wrong or harmful. In order to get rid of them I’d like to present them here and explain the reasons why they should be avoided:
Log-Log-Log and Rethrow
Imagine a typically layered application design where you have a DAO layer at the bottom, a service or business logic layer in between and a frontend layer on top. Now imagine that you decided to wrap most exceptions from the lower layer and rethrow them with an exception which is more apropriate to the level of abstraction that you are currently on:
DAO Layer:
public T getById( Id id ) {
try {
....
} catch( Exception e ) {
LOG.error( e.getMessage(), e );
throw new WrapperException( e );
}
}
Service Layer:
public T doSomething( Id id )
try {
T foo = getById( id );
....
} catch( Exception e ) {
LOG.debug( e.getMessage(), e );
throw new WrapperException( e );
}
}
This happens again in the UI layer. You usually do not have per layer logfiles, as this would make it harder to see the flow of the application. But when you log the exception on every layer, it leads to a lot of log clutter. The best way is to define a layer that logs the exception, the business layer being the obvious candidate here. Thus you have a defined way to handle lower layer exceptions. This pattern does not appear that often anymore, as unchecked exceptions are generally considered the better solution today.
Fail Silently
There are two versions of the fail silently antipattern:
Version 1:
try {
....
} catch (Exception e ) {
// empty block<
}
Version 2:
try {
....
return true;
} catch (Exception e ) {
return true;
}
The two versions of the Fail Silently Pattern have the same problem: you will never know that an exception happened. Combined with the fact that Exception is caught all kinds of runtime problems can be hidden. Usually, you should always catch specialized exceptions that you expect and can handle. If you’re doing frameworks or using libraries that might throw all kinds of exceptions, or you write handling code that needs to handle all kinds of exceptions on some way you still must log what is happening there. Otherwise you will never be able to explain certain behavior in production. And an exception handling block should never return the same value as the usual code path. After all it is an exception handling block.
Fail with least possible information
try {
....
} catch (Exception e ) {
LOG.error("Something went wrong");
}
There are two simple rules when handling exceptions: always log the exception including the stacktrace and always include any data which might have lead to the stacktrace. I still see it today that people discard all information directly at their fingertips to log out what they think might have happened at that point. Or what they have expected to happen. Together with to wide catch clauses this leads to the problem that you do not really know what happened in production, you just believe you know. Even worse, you might stick to assumption someone else made about the error at the point. So when you write a try-catch-block, always see that the variables used in the try are also logged out in the catch.
Deduct from vagueness
try {
....
} catch ( Exception e ) {
throw new CustomerNameTakenException( "The user already exists" );
}
This is somehow the worst case of the above mentioned pattern. Here not only the wrong information is logged but it is used to create error messages. This might lead to all kinds of wrong behavior. Just because you know that a certain layer or lower level function behaves in a certain way does not make it an interface. Maybe a different exception is thrown, maybe something else went wrong.
As usual, these points are open for discussion. But I believe that these are some general rules everyone needs to apply when writing exception handling code.
blog.thiesen.org 