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...

Answer 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.

Answer 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);
    }
}

Answer 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)

Answer 4

There some options you can choose.

1. One of the most important adnvantages of exceptions using is that you can have the only exception handler for all cases. When you write your code you should think about functionality first and only then about exception handling. As a result you may skip in some places try/catch at all for unchecked exceptions and declare your methods as throws SomeCheckedException for checked exceptions. This allows you to have the minimum number of exception handlers.

2. If you don't know what exactly to do with an exception, just rethrow it.

3. If you catch checked exception but you don't want clients that use your code have to process exceptions, you may checked exceptions update into unchecked. (Hibernate processes exceptions by this way. They catch checked sql exception and throw unchecked exceptions instead of this)
4. If the previous recommendation are not convenient for your, then you may also choose some options:
   • you may just add logging and rethrow the same exception
   • you may use exception chains by throwing new exception with initCause() method
   • you may think that code that invokes your code should not know anything about the originial exception, you may archive this either by creating new exception or by using fillInStackTrace() method.
5. Concerning catching Throwable. Yes in the methods you should not catch it. But as a rule the client that uses your programe need not to see excetpions at all, as a result, you may catch Throwable in the some exception handler that is the top in the chain.