Reading image from jar

Question

I am using this static method in my class IconManager to read an image from jar file. imagepath is a relative path to the image.

This code works perfect. But I was told it is error-prone, because does not handle Exceptions correctly.

private static BufferedImage readBufferedImage (String imagePath) {
    try {
        InputStream is = IconManager.class.getClassLoader().getResourceAsStream(imagePath);
        BufferedImage bimage = ImageIO.read(is);
        is.close();
        return bimage;
    } catch (Exception e) {
        return null;
    }
}

How would you refactor the code?

Answer 1

• Don't catch (or throw) Exception.

• Try not to return null. If you cannot return a normal value you want one of the following to happen:

  1. Program quits with an error. (Your current program seems to do this)
  2. You expect calling site to take some action and try to recover and continue execution
  3. You do not want to hassle the caller of the method and try to continue execution with a sensible default value.

  If you want program to quit with an error. Wrap the exception in a runtime exception with a meaningful (to the developer, not user) message. That way in your logs you will see what went wrong and where. If you return a null what you probably get is a nondescript NullPointerException down the line.

  If you want the caller of the method to check if something went wrong, you want to throw a checked exception. Null checks always get ignored and your program crashes and burns in inopportune moments and users think, rightly, the program is of low quality. If you do not have and cannot create a suitable Exception, you can propagate the exception.

  Third option is usually ignored but sometimes continuing with some default value is the most logical thing to do. Think what browsers do if they cannot download an image, which basically is an IOException. They show a white rectangle with a red cross on it. A web page with some of the images missing is more useful than a page full of stack trace. And note that a white rectangle with a cross behaves like a normal image in terms of layout, hence do not break the page layout. This is the essence of the Null Object Pattern.

• (Updated as per @Sulthan) You should call the InputStream.close() in a finally block. Because if ImageIO.read throws an exception you never reach the close() statement.

A few notes about the example code below:

1. Resource leaks may occur if InputStream.close throws. There isn't much you can do about it.
2. Wrapping example and the ImageReadException do not pull their weights as they are. they are here for demonstration.
3. Method names are shortened to fit screen.

Here is the basic case:

private static BufferedImage readPropagate(String imagePath) throws IOException {
    InputStream is = null;
    try {
        is = IconManager.class.getClassLoader().getResourceAsStream(imagePath);
        // an example of how null checks can easily be forgotten 
        if (is == null) {
            throw new FileNotFoundException("Resource not found: " + imagePath);
        }
        return ImageIO.read(is);
    } finally {
        if (is != null) {
            is.close();
        }
    }
}

Here are some examples of what can be done when the above example is not sufficient:

public static class ImageReadException extends Exception {
    private static final long serialVersionUID = 1L;

    public ImageReadException(String message, Throwable cause) {
        super(message, cause);
    }

    public ImageReadException(String message) {
        super(message);
    }
}

private static BufferedImage readWrap(String imagePath) throws ImageReadException {
    try {
        return readPropagate(imagePath);
    } catch (IOException e) {

        // log with your normal logger
        logException(e, "An error occured while loading image " + imagePath);

        // Message should be something meaningful to the calling site
        throw new ImageReadException("An error occured while loading image", e);
    }
}

private static BufferedImage readWithDefault(String imagePath) {
    try {
        return readPropagate(imagePath);
    } catch (IOException e) {

        // log with your normal logger
        logException(e, "An error occured while loading image " + imagePath);

        //default image may be passed as a parameter instead
        return IconManager.DEFAULT_IMAGE;
    }
}

// YOU SHOULD NOT DO THIS IN LIBRARY CODE
// The decision of whether to crash the program belongs to the top level application
private static BufferedImage readThrowSilently(String imagePath) {
    try {
        return readPropagate(imagePath);
    } catch (IOException e) {
        // you can do this, e.g., while loading a desktop application
        // you discover a vital resource could not be loaded
        // continuing execution does not make sense
        throw new RuntimeException("An error occured while loading image " + imagePath, e);
    }
}

private static void logException(IOException e, String message) {
    Logger.getLogger(IconManager.class.getName()).log(Level.SEVERE, message, e);
}

Answer 2

If you have java7:

private static final BufferedImage emptyBufferedImage = new BufferedImage(0, 0, BufferedImage.TYPE_INT_RGB);

private static BufferedImage getBufferedImageFromInternalPath(final String imagePath) {
    BufferedImage result = null;
    try (final InputStream inputStream = IconManager.class.getClassLoader().getResourceAsStream(imagePath)) {
        result = ImageIO.read(inputStream);
    } catch (final Exception e) {
        // log exception
    }
    return result == null ? emptyBufferedImage : result;
}

Some thoughts: I would avoid any return statements inside try/catch/finally. They are well defined in the JLS, but confuses most readers easily.
The try-with-resources statement is available since java 7: http://docs.oracle.com/javase/tutorial/essential/exceptions/tryResourceClose.html
If you return null or a default value depends on the use case. Personally I think a default value is the more clean way, but null could be quite ok for some internal usage. KISS principle.

For further comments, see post from abuzittin gillifirca.

Answer 3

Before the return null; you have to had add an error message to your Logger (or print to console an error) with e.getMessage() , so you will know where is the problem.

Otherwise, when your code is published, nobody can know why or where your code hang.