Critique my exception handling strategy

Question

I've been doing object-oriented programming for most of the past 7 years, using Java on and off during that time. Some things I'm certain I have an excellent grasp of, such as the most useful design patterns. In fact, the following code allowed me to crank out a little system in under a day's time, that would handle the one particular instance we are ready to implement now, while being flexible enough to handle the future requirements I've been informed of:

public void importAndArchive(File detectedFile) throws FileNotFoundException, IOException {
        File workingCopy = returnWorkingCopy(detectedFile);
        List<String[]> csvData = csvData(workingCopy);
        setHeaderFields(csvData.get(0));

        importData(csvData); //subclass will implement this abstract method

        archiveWorkingCopy(workingCopy);
    }

I don't show the above to boast about my grasp of the template method, but rather as a starting point to discuss a gap I have in my abilities that is glaring in light of my OO design abilities. And that gap is, a systematic approach to exception handling. Indeed you can see in that method signature I am for the time being re-throwing some exceptions, but actually I've gone on to eradicate the very same sort of thing in another corner of the application.

Before I go much further though, since this is my first attempt to be especially systematic, I would like some validation of what I've done thus far. Our application loops over a bunch of files, "processes" them, and "archives" them. Pretty standard fare. One decision I've made in order to roll out a prototype as quickly as possible is as follows. The application gets initialized based on data in a (ResourceBundled) properties file. The various methods in the ResourceBundle API throw unchecked exceptions, but I am not really handling them whatsoever for the time being, on the grounds that they will prevent the app from starting, and the stacktrace can suffice for the time being.

I have however chosen a library for CSV handling which throws checked exceptions. NetBeans then made it very easy to proliferate those exceptions to begin with ;) The following is a method that I've since reworked that actually handles those exceptions:

private String detectImportHandler(File detectedFile) throws Exception {
    String[] firstLine = null;

    try {
        /*
         * CSVReader throws checked exceptions
         */
        CSVReader csvReader = new CSVReader(new FileReader(detectedFile));
        firstLine = csvReader.readNext();
        csvReader.close();
    }
    catch(Exception x) {
        throw new Exception("CSVReader unable to process file: " + x.getMessage());
    }

    try {
        return firstLine[1];
    }
    catch(Exception x) {
        /*
         * since we're re-throwing CSVReader's checked exceptions it seems easiest
         * to also re-throw null-pointer and/or array-out-of-bounds errors
         */
        throw new Exception(
                "First line null or did not have importHandlerType field: " + x.getMessage());
    }
}

The above method is being called thusly, inside a loop that processes the files:

    try {
        importHandlerType = detectImportHandler(detectedFile);
    }
    catch(Exception x) {
        unusableFileErrors.put(detectedFile.getName(), x.getMessage());
        continue;
    }

unusableFileErrors is a Map, and I figure when I am done iterating over the files, I can then use this map and the file-specific messages it contains to handle things at a higher level, such as logging, moving the files somewhere else on the system, etc.

Anyway I've gone on long enough. I have ordered the book "Robust Java" and I'm hoping between it, and the SO community, I can improve this neglected facet of my abilities. I've seen other similar questions on SO but I figure that also requesting specific advice in the context of actual code might be of benefit.

Answer 1

Generally I handle exceptions in one of two places:

1. If I can handle an exception without the user having to know, I handle it as close to the error as possible.
2. If the error requires some sort of output (logging, pop up a dialog, write message to stdout) I let it pass down to some centralized point, at which point it is caught and distributed to the appropriate output mechanisms.

This seems to work well enough for me.

Answer 2

Small note: I'd suggest chaining the actual cause rather than just appending the original message; otherwise you'll lose your stack traces and debugging will be more painful than it needs to be.

Beyond that: I don't see a whole lot of value in the try/catch blocks inside detectImportHandler(). Since the method already throws Exception, there's no need to re-wrap whatever's thrown by CSVReader, and the NullPointerException will be caught in the calling try/catch just as well as anything else.

What would be more valuable, potentially, would be a finally clause that closes the FileReader (and/or CSVReader), if an exception can be thrown either by readNext() or by the CSVReader constructor.

Answer 3

A few remarks:

1. When wrapping an exception e, you just use the e.getMessage(). Instead I suggest you use use new Exception(message, e). This will set the cause of the exception, so the entire stacktrace will also contain a stacktrace of the original exception. This can be very useful for debugging the cause of your exception.

2. Instead of throwing Exception, I suggest you throw more explicit exceptions, e.g. FileNotFoundException. Use the exceptions defined by the Java API when they makes sense.

3. Instead of catching Exception, I suggest you explicitly state which exceptions to catch.

4. (Optional replacement for 2) Some developers prefer to - instead of defining an exception class for each kind of exception - throw a general exception containing an error key indicating the type of the exception, e.g. new DetailedException(GeneralBusinessErrors.PARSING_ERROR). This makes it easier to assign language dependent resource files for your business errors.

5. It may be useful to add debug data to your exception. For example, if a file was not found, it could be information such as which file you tried to open. This is very useful in maintenance where you may be unable to reproduce the problem and/or debug using a debugger.

6. It may be useful to have your system logging any thrown unchecked exceptions that are not caught. Generally speaking, it may be useful to log any exception that is thrown (caught or not).

Answer 4

Catching and throwing plain old Exception is bad - you get all the runtime exceptions, whether you want them or not (probably not).

I try to throw checked exceptions that make sense in the context of the method that they're coming from. For example if I had a loadCustomer(long id) I'd throw a ObjectLoadException rather than an SQLException.

And yes you read right, my preference is checked exceptions - I like the fact that the consumer of a method has to explicitly decide what to do when an exception occurs, it increases readability through the imagined call stack when reading code imho.

Answer 5

Typically I would employ the catch-rethrow approach if I wanted to translate implementation-specific exceptions into exceptions that I wanted to expose via my API.

For example, suppose the import method from your example takes a URL rather than a File. If this URL happened to refer to a File I would not want to expose FileNotFoundException on the API (this is an implementation detail), but rather catch and rethrow this (e.g. as an ImportException) but still chaining to the underlying FileNotFoundException cause (as suggested by David).

Also, I typically do not catch and rethrow unchecked exceptions (like NullPointerException) although do in certain cases (e.g. Spring's DataAccessException).

Answer 6

I already gave @Kolibri an up-vote, so read his first, but I have a few things to add.

Exception handling is something that is sort of a black art and the expected practices change from language to language. In Java, look at the method signature as a contract between your method and the caller.

If the caller breaks their end of the contract (e.g. passes in "detectedFile" as null), you want to throw an unchecked exception. Your API has given them the contract and they've explicitly broken it. Their problem. (Ignore that FileNotFoundException is checked for now, that's sort of a beef I have with Java)

If the caller passes in proper data and you can't complete their request, however, that's when you throw a checked exception. For instance, if you can't read their file due to a disk error (IOException for instance), that's you breaking your end of the contract and it makes sense to throw a checked exception. Checked exceptions warn the caller of all the possible things that could go wrong when they call your method.

To that end, it depends on what kind of granularity you want to give the user for handling your exceptions. Throwing "Exception" is usually undesirable, because like others have said, it gives you no granularity into handling what happened. The caller doesn't immediately know if they broke the contract or your method did, because Exception includes both checked and unchecked exceptions. In addition, maybe your caller wants to handle a FileNotFoundException vs. a IOException differently in their code. If you just throw Exception, they lose the ability to do this easily. Defining your own exceptions can expand upon this concept and allow callers of your method to handle exceptions from your method in a more granular matter. The key here is that if you throw 10 different exceptions and the caller doesn't care about the difference, they can just do "catch(Exception e)" and catch them all in one shot. If you just "throws Exception" though, the user no longer has a choice in whether they handle different exceptions in a different way or just catch them all.

I also highly agree with the comments about using "throw new XXXException(message,e)" instead of "throw new Exception(message + e.getMessage())". If you use the first method, you'll maintain the internal stack traces all the way back to the original error. If you use the second method, the stack trace will now only trace back to your "throw new Exception" line and you'll be forced to do more debugging to figure out what caused that Exception to happen. This has bitten me in the past.

Finally, don't be afraid to use Exception.printStackTrace() in a catch if it's going to help you. Users hate seeing stack traces and you should handle exceptions where you can, but if it's a truly unexpected exception and having a stack trace will help you greatly debugging in the future, just print the stack trace.

Wow, that was like an essay. I'll stop now. Good luck! :-D

=====================

Addendum: I just caught @pjp's comment and it bears repeating...You should definitely be calling that ".close()" method in a "finally" block after your "catch" statement. Otherwise in the exception case your method will return without immediately closing the resource that you allocated & opened.

Answer 7

I think there is another dimension to the problem which hasn't been covered yet. And that is that exception handling is not just about the coding technique, it is about the error condition and what you want to do about it. Java's checked exceptions are a mixed bag because it forces you to think about the error conditions when you may not want to, causing some developers to do things that are worse than doing nothing in their desire to get the compiler out of the their way. (Unchecked exceptions have the opposite problem - it causes the developers to forget about error handling even very common scenarios such as a dropped network connection).

There are arguably three classes of exceptions, which correspond to the three types in Java: Checked Exceptions, Runtime Exceptions and Errors.

Errors should only be handled at a very high level in the application and generally should be regarded as non-recoverable. Even though OutOFMemoryError is somewhat recoverable in practice there is just too much that went wrong in sequences of code that you don't think about (such as Object initialization) that can leave things in a bad state that you should only try if you can really segregate the code (such as in an application server).

Runtime Exceptions, in the short hand, should be things that are deterministic and will always go wrong given the same inputs. For example, if you pass a null reference to a method that doesn't allow nulls (NullPointerException) or if you do an Integer.parseInt, a given String the result will be the same every time.

Checked Exceptions, again in a short hand definition, are things that you cannot know ahead of time if they will go wrong or not. FileNotFoundException, for example, the file may be there when you created the File object, you can even check, but milliseconds later something could delete it out from under you.

If you design and react to exceptions with those categories in mind (realizing that not all API designers follow them - for example XML exceptions are checked even though the behavior is deterministic for a given XML input, and of course Spring just decided to avoid checked exceptions altogether) then you will have clear ways to think about what kind of error handling you need. What do you need to do when a Network connection goes down is very different (depending, of course, on your project) than what you do if the JVM runs out of memory and both are very different than what happens if you have to parse an Integer but don't get one from user input.

Once you are thinking in those terms, what you do about exception handling will flow more naturally from what type of error handling and robustness you are trying to achieve.

Answer 8

There are some extremely good answers already, but I wanted to contribute an additional thought.

There isn't just a single error-handling strategy that fits all cases.

In general, you should be consciously making a choice between two fundamentally opposite cases. The choice will differ depending on what you are building, and what layer in your application you are currently working on. The two alternatives are:

1. Fail Fast. Immediately an error arises, you assemble all the information you have and ensure it reaches somebody who can do something about it - either in your calling code, by throwing an Exception, or perhaps calling the help desk, who can get the code fixed before any data corruption occurs. Masking the error, or failing to capture sufficient information about it, will make it harder to figure out what went wrong.

2. Be bulletproof. Carry on regardless, because stopping would be a disaster. Of course, you still need to capture information about the failure and report it somehow, but in this case, you need to figure out how you are going to recover and carry on.

Many bugs are introduced because the programmer adopts the second strategy when they should have adopted the first. If it isn't obvious how you should recover from the situation, you should probably be throwing an Exception and letting whoever called you make a sensible choice - they may have a better idea what to do than you do, but they need to have sufficient information to decide what that is.

Another useful piece of advice is to ensure that the exception you throw makes sense at the API level. If your API is designed to render data in html format, you don't want to see a database exception thrown simply because somebody has decided to enable the audit trail. Cases like this are often best handled using chained exceptions - throw new ApiException(causeException); for example.

Finally, I want to give my take on unchecked and checked exceptions. I like to reserve unchecked exceptions for situations that mean a programmer has made a mistake. Passing null to a method that requires an object reference, for example. Where the error could arise even if the programmer is perfect (FileNotFound if the file has been deleted since the programmer checked its existence, for example), a checked exception is usually appropriate. Essentially, the programmer who creates the API is saying that "even if you got all the inputs right, then this problem might still crop up, and you may have to deal with it".