Thursday, May 27, 2010

Exception smells

Proper exception handling is an important element for building robust, reliable systems. However quite often it's a somewhat neglected aspect of the design, emphasized only in later phases of the development process - sometimes even after the product hits production. I think a reason for that pattern is: much of the exception handling is there to satisfy non-functional requirements. Such requirements are often implicit and the real impact of not meeting them may not become obvious until the system is put into use. I was a developer in a big project some years ago where exception handling strategy was not a topic in design discussions until the 5th sprint. Needless to say, this late introduction was a painful process.

Finding good generic practices on exception handling is not so easy, it depends too much on the concrete context. It's much easier to think about what are bad practices instead. Here are a few things I don't like to see in the source code in general.

Swallowing the exception

try {
doSomething();
} catch(MyException e) {
// do nothing
}
Problems remain undetected.

Fake return value

try {
return doSomething();
} catch (MyException e) {
log.error(e.getMessage(), e);
return null;
}
Only logging the exception may be fine in some scenarios, but the made-up return value is asking for trouble. Without logging it's even worse.

Lossy wrapping

try {
doSomething();
} catch(MyException e){
throw new MyOtherException(e.getMessage());
}
Stacktrace of the original exception gets lost.

Checked base exception

public void doSomething() throws Exception {
}
Specifying the base exception adds no real value.

Throwing the kitchen sink

private void doSomething() throws MyException, AnotherException, YetAnotherException {
}

public void foo() {
try {
doSomething();
} catch(MyBaseException e) {
// handle the exception
}
}
If they're handled the same way, definition of all those checked exceptions becomes pointless in the method signature.

Catching base exception

private void doSomething() throws MyException {

}

public void foo() {
try {
doSomething();
} catch(Exception e) {
// handle the exception
}
}
The wrong exception may be handled.

Throwing exception in finally

try {
doSomething();
} finally {
cleanUp();
}
This code is fine, as long as cleanUp() can never throw an exception. If it does, the details of any exception thrown in doSomething() will be lost.

Relying on cause

catch (MyException e) {
if (e.getCause() instanceof MySubException) {
// ..
}
Relies too much on the composition of the exception. That might change and the code won't work as expected.

Log and throw

catch(MyException e) {
log.error(e.getMessage(), e);
throw new MyOtherException(e);
}
Only partial stack trace gets logged, the exception can potentially be logged multiple times. In some scenarios it can be justified (loss of exception details at remote invocations).

No comments:

Post a Comment