What is the Best practice for try catch blocks to create clean code?

Question

Possible Duplicate:
Best practices for exception management in JAVA or C#

I've read a question earlier today on stackoverflow and it made me think about what is the best practice for handling exceptions.

So, my question is what is the best practice to handle exceptions to produce clean and high quality code.

Here is my code, I think it's quiet straight forward but please let me know if I'm wrong or not clear! I've tried to keep in mind testability and same abstraction level in methods.

Every constructive comment is welcomed. :)

import java.awt.Point;
import java.io.Closeable;
import java.io.FileInputStream;
import java.io.FileNotFoundException;
import java.io.IOException;
import java.io.ObjectInputStream;
import java.util.List;

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import com.google.common.base.Preconditions;

/**
 * <p>This is a dummy code.</p>
 * The aim is present the best practice on exception separation and handling.
 */
public class ExceptionHandlingDemo {
    // System.out is not a good practice. Using logger is good for testing too (can be checked if the expected error messages are produced).
    private Logger logger = LoggerFactory.getLogger(ExceptionHandlingDemo.class);

    // instance of cannot work with List<Point>
    private interface PointList extends List<Point> {}

    /**
     * The method that loads a list of points from a file.
     * @param path - The path and the name of the file to be loaded.
     * Precondition: path cannot be {@code null}. In such case {@link NullPointerException} will be thrown. 
     * Postcondition: if the file don't exist, some IOException occurs or the file doesn't contain the list the returned object is {@code null}.
     * */
    /* (Google throws NullpointerExceptio) since it is not forbidden for the developers to throw it. I know this is arguable but this is out of topic for now. */
    public List<Point> loadPointList(final String path) {
        Preconditions.checkNotNull(path, "The path of the file cannot be null");

        List<Point> pointListToReturn = null;
        ObjectInputStream in = null;

        try {
            in = openObjectInputStream(path);
            pointListToReturn = readPointList(in);
        } catch (final Throwable throwable) {
            handleException(throwable);
        } finally {
            close(in);
        }

        return pointListToReturn;
    }

    /*======== Private helper methods by now ========*/

    private ObjectInputStream openObjectInputStream(final String filename) throws FileNotFoundException, IOException {
        return new ObjectInputStream(new FileInputStream(filename));
    }

    private List<Point> readPointList(final ObjectInputStream objectInputStream) throws IOException, ClassNotFoundException {
        final Object object = objectInputStream.readObject();

        List<Point> ret = null;

        if (object instanceof PointList) {
            ret = (PointList) object;
        }
        return ret;
    }

    private void handleException(final Throwable throwable) {
        // I don't know the best practice here ...
        logger.error(throwable.toString());
    }

    private void close(final Closeable closeable) { 
        if (closeable != null) {
            try {
                closeable.close();
            } catch (IOException e) {
                logger.error("Failed closing: %s", closeable);
            }
        }
    }

    /*======== Getters and setters by now. ========*/

    // ...

    /**
     * @param args
     */
    public static void main(String[] args) {
        ExceptionHandlingDemo test = new ExceptionHandlingDemo();
        test.loadPointList("test-filename.ext");
    }
}

EDITED:

What I want to avoid is writing lot of catch cases after each other...

Solution 1

A few suggestions at first glance:

1. You shouldn't catch Throwable and instead catch as specific of an exception as possible. The trouble with catching Throwable is that will include Error classes like OutOfMemoryError and the like. You want to let those pass through (they're unchecked for a reason).
2. When you log exceptions always pass the exception and not just its toString(). It's very hard to diagnose problems without the stack trace.
3. You probably don't want a general exception handling method.

So at places you catch exceptions you want something like this:

} catch (IOException e) {
    logger.error("some relevant message", e);
    // now handle the exception case
}

The message should include some contextual information if possible. Anything that might help tracking down the problem when someone is hunting through logs.

Solution 2

For checked exceptions that I'm not going to bother, I always use org.apache.commons.lang.UnhandledException.

For example

/**
 * Calls Thread.sleep(millis)
 */
public static void threadSleep(final long millis) {
    try {
        Thread.sleep(millis);
    } catch (final InterruptedException e) {
        throw new UnhandledException(e);
    }
}

Solution 3

Always catch the most specific exception possible--in otherwise never catch throwable.

The most important thing to me is that you NEVER have an empty catch block--one of these can take an amazing amount of time to find if something inside the try actually throws an exception.

I personally like to remove checked exceptions as quickly as possible and replace them with pre/post condition checks if possible. Same with unchecked exceptions to a lesser degree--however unchecked exceptions are actually a pretty good way to indicate programmer error like parameter checking to ensure object state (Although asserts may be better yet)

Comments

[Bruno Reis]

As a side note, I would recommend that you mark your private helper methods static, since that's what they are: they take inputs and produce outputs solely based on those inputs. When I saw no static keyword on your method declaration, I was forced to check if the methods really did not use any state, which is bad, since the code force me to focus on something not important at all to read it. This question deals that: stackoverflow.com/questions/3346764/…

[artbristol]

+1 for never having empty catch blocks - and run PMD/FindBugs to pick this up

[uthomas]

I wanted to create a general exception handling method. Could you explain me why is it bad practice? I mean I can see that this way I can swallow some exceptions that shouldn't be so, but until now I didn't know any better way to keep the business logic clean. So what I didn't wanted to see is catch cases after each other...

[WhiteFang34]

If you have a common block of code that you want to reuse for some exceptions then a general method would be fine. You only have one line in that method at the moment however, so it's not any less code at the place making the call. If you do keep it you should at least add a method variable to pass the relevant message. I don't think it's unclean to have specific logging and specific exception handling in your business logic though. If you find yourself repeating the same block of code then I would abstract it out (as a general rule even).

[jyoungdev]

What if your logger throws while you're logging an exception?

[Bill K]

Hopefully it's smart enough to report it's exception to stderr with your exception wrapped as the cause so at least you have something.