Error Handling

I came across a fairly typical error handling mistake today and thought I’d share it.

The code was something like this:

class Factory {
    static Foo getFoo() {
        try {
            // load a .properties file, read a key from it
            ...
            return new Foo(propValue);
        } catch (Exception ex) {
            log.error("Failed to find key in properties file: " + ex.toString());
            return null;
        }
    }
}

and the caller did e.g.

    Factory.getFoo().doSomething();

The thing to note here is that the caller is assuming that getFoo() method never returns null.

That’s probably a good assumption on the part of the caller. The factory method should always succeed unless there is a programmer error (e.g. the key does not exist in the .properties file) or a system error (e.g. an I/O error accessing the file). Neither of which the caller should be expected to handle gracefully. That’s what exceptions are for!

In the event of an error, we’d get a NullPointerException from the caller site and an error printed to the logfile. If, for example, you are running this code in a debugger, you’ll be looking at a NullPointerException with no clue as to what caused the exception.

The moral of the story here is to think clearly about when errors should be handled gracefully and when you should just let exceptions be passed back up the stack.

In the end, I added a new exception type:

class FactoryException extends RuntimeException {
    
    private static final String ERROR_MSG = "Failed to lookup key {0} in file {1}";

    private String propsFile;
    private String propsKey;

    FactoryException(String propsFile, String key, Throwable cause) {
        super(MessageFormat.format(ERROR_MSG, propsFile, key), cause);
        this.propsFile = propsFile;
        this.propsKey = propsKey;
    }

    String getPropsFile() {
        return propsFile;
    }

    String getPropsKey() {
        return propsKey;
    }
}

and then re-factored the original code:

class Factory {
    static Foo getFoo() {
        try {
            // load a .properties file, read a key from it
            ...
            return new Foo(propValue);
        } catch (Exception ex) {
            throw new FactoryException(propsFile, propsKey, ex);
        }
    }
}

so now if there is an error, rather than getting a fairly useless NullPointerException you get:

FactoryException: Failed to lookup key foo in file bar.properties
    ...
Caused by: java.io.FileNotFoundException: ...

5 Responses to “Error Handling”

  1. Bob Bobson says:

    It’s only called refactoring if you change the internals of something without changing the behaviour. The behaviour of your factory method now behaves differently, so this isn’t refactoring. It’s just ‘changing the code’.

    Stop trying to get buzz words in if you don’t even know what they mean!!

  2. markmc says:

    Thanks for that Bob, I stand corrected

    By the way, you’re not Bob from ‘Bob the Angry Flower’ fame by any chance ? :-)

  3. As an FP adept, I’d say what you actually want is an ‘Option’ type, as it’s called in Scala, or ‘Maybe’ in Haskell (most FP languages have something similar) and the corresponding Monad instance for easier/nicer coding.

    Or a ‘safe navigation’ operator as found in e.g. Groovy (‘?.’), which is related to the Elvis-operator (as found in Groovy as well: ‘?:’ and might have made it into Java7).

  4. Simon says:

    Eh, I’ve seen worse. Much of the code I deal with day-to-day is much like the first one, but using log.debug instead of log.error. Because hiding major problems from those not running with debugging is just *so* helpful…

  5. Simon says:

    Actually, no – a lot of the code I see would go even further, and load the properties file from a static{} block, leaving the method to just return the looked-up value. Because that way, an exception thrown in the static{} block causes the classloader to choke on it, and subsequent references to the class to result in obscure NoClassDefFoundErrors.

    I hate my co-workers some times…