Saturday 6 February 2010

Exceptional? No.

Spotted this the other day whilst making a small change to the code at work. It was strange, something felt very wrong but I couldn't quite put it into words. I see this sort of stuff occasionally that just feels wrong but I can't always say exactly what it is. As luck would have it a coworker wandered in at that moment so we looked at it together, he declared if he;d written code like that he should be shot. Here it is. It's an accessor method on a class representing a row in a db table.

public boolean getColumnBlah() {

    try {
        return columnBlah.getValue();
    } catch (NullPointerException ex) {
        return false;
    }
}

It was the use of a NPE to catch an empty value, if the default was meant to be false, make it false by default surely and assume it won't be null in this method. Also, where did the NPE come from, obviously some lazy dev assumed it was due to a null value on that column, and not as was possible from some place else in the getValue() call which was entirely possible. If it was you'd be silently failing at something else and detecting the failure would be far less likely.

Exceptions should indicate something exceptional happened not something predicatable and expected. Special bonus points awarded this time as Mr Cowrker joining me in looking at this turned out after some cvs detective work to be responsible. Some time ago admitedly so we decided he'd not leave the company with his head hung low. He did also point out that paying the company back for the 2 years emplyment after creating the sinful code and rolling back all his commits for the last 2 years would be pretty hard work as well.

1 comment:

  1. Think he was too specific, should have just tried catching a Throwable. There's lots of other unexpected things that could cause an exception , its always safest to just catch a Throwable (don't log anything, but put a comment in to note that something unexpected happened).

    ReplyDelete