Home > Java > Test For Null In Exception Catch Block Indicative Of Anti-Pattern

Test For Null In Exception Catch Block Indicative Of Anti-Pattern

In a blog posting about Project Coin I came across this example: (first comment is mine)

// Please do not write code like this:
private static void customBufferStreamCopy(File source, File target) {
    InputStream fis = null;
    OutputStream fos = null;
    try {
        fis = new FileInputStream(source);
        fos = new FileOutputStream(target);
        byte[] buf = new byte[8192];
        int i;
        while ((i = fis.read(buf)) != -1) {
            fos.write(buf, 0, i);
        }
    }
    catch (Exception e) {
        e.printStackTrace();
    } finally {
        close(fis);
        close(fos);
    }
}

private static void close(Closeable closable) {
    if (closable != null) {
        try {
            closable.close();
        } catch (IOException e) {
            e.printStackTrace();
        }
    }
}

At first sight and leaving the typos (closable) and other naming issues for what they are, this looks okay. There are however several problems with this code:

  • The code is monolithic.
  • Callers of customBufferStreamCopy have no idea if the call failed or succeeded.
    Since this method is a simple API for copying files, callers should be notified if the copy action failed: the method should throw IOException.
  • Do Not Catch java.lang.Exception. Ever. Please just don’t.
    Never catch Exception directly; catch one of its subclasses. And yes, there are exceptions to this rule, but if you do not know them you should simply adhere to the mantra: Do Not Catch java.lang.Exception.
  • Variable initialization to null.
    This is an ugly hack to prevent compiler errors. Ugly hacks like this are indicative you are doing something suboptimal or even wrong.
  • Test for null in catch block.
    Although I have seen this anti-pattern so many times, I still feel a little sad when I come across an instance of it. If you need to test wether or not something needs cleaning up, your exception handling is needlessly complex. The right way of using objects that need special cleanup or disposal is NOT: 

    // the wrong way: needlessly complex
    
    try {
        // get reference to object
        // use object
    } catch (/* ... */ {
        // error handling
    } finally {
        // dispose of object only if object exists
    }

    It can be simpler:

    // simpler is better (in this case at least)
    
    // get reference to object
    try {
        // use object
    } catch (/* ... */ {
        // error handling
    } finally {
        // dispose of object
    }

Incorporating the above criticisms, we can rewrite the copy method as follows:

// This is better…
private static void customCopy(File source, File target) throws IOException {
    InputStream fis = new FileInputStream(source);
    try {
        OutputStream fos = new FileOutputStream(target);
        try {
            byte[] buf = new byte[8192];
            int i;
            while ((i = fis.read(buf)) != -1) {
                fos.write(buf, 0, i);
            }
        } finally {
            fos.close();
        }
    } finally {
        fis.close();
    }
}

Advantages:

  • No if-statements: simpler execution flow.
  • Better modularity.
  • No compiler hacks.
  • No need for a close utility method.

This code still looks monolithic, but as a direct result of the first two advantages it is now trivial to refactor the method into three separate methods:

// And this is best!
private static void customCopy(File source, File target) throws IOException {
    InputStream fis = new FileInputStream(source);
    try {
        customCopy(fis, target);
    } finally {
        fis.close();
    }
}

private static void customCopy(InputStream source, File target) throws IOException {
    OutputStream fos = new FileOutputStream(target);
    try {
        customCopy(source, fos);
    } finally {
        fos.close();
    }
}

private static void customCopy(InputStream source, OutputStream target) throws IOException {
    byte[] buf = new byte[8192];
    int i;
    while ((i = source.read(buf)) != -1) {
        fos.write(buf, 0, i);
    }
}

Now we have three lean methods that are easy to understand and test. A lot better than the monolithic example we started out with.

About these ads
Categories: Java Tags: ,
  1. bbakker
    01/02/2011 at 21:59

    I think that the intension of the blog post on project Coin is to show the ARM feature of Java 7, which eliminates the need for the boilerplate code of closing resources.

    A big difference between your method and the ‘original’ one though is that you declare an IOException in the throws of your method, exposing implementation, where the original handles the exception within the method. Therefore most resources are created within the try-block, allowing to handle the possible exception of its constructor too.

    The ARM feature of project Coin will really improve resource handling, allowing to focus on that what’s really important (the copy in this case). As shown in the project Coin post that actually is the better way.

  2. Leo Mekenkamp
    02/02/2011 at 11:46

    The intension was to show off new language features, but examples tend to stick in the mind. If a junior java programmer reads the original blog, (s)he might think that that is good coding practice. That is why every example should have clean code.

    And about throwing an IOException: you can also wrap it in a CouldNotCopyException or whatever. Point is you need to throw an exception if the method cannot perform its function as specified. Or create another method that calls customCopy and eats the exception but explicitly tells its caller it does that (for instance by calling it maybeCopy). And by the way: since copying is inherently tied to i/o, throwing an IOException does not expose the implementation.

    While I look forward to the improvements Project Coin will bring, I think advertising its potential by using bad examples works to its disadvantage. I can write very shitty code and by using GOTO-statements clean up that code, but that does not in any way mean GOTO-statements by itself are any good.

  3. Gamma
    23/04/2011 at 08:23

    The problem with this refactoring is that close() can throw an exception. So, if fos.write() excepts and then fis.close() also excepts, the IOE that customCopy(File, File) throws only indicates that the InputStream didn’t close properly and says nothing about the failure to write data to the OutputStream.

  4. Leo Mekenkamp
    03/05/2011 at 14:52

    The original code behaves in the same way in that the original exception (from fos.write()) is not thrown. This refactoring does not introduce that behavior. Note that I left out logging because I wanted to focus the other bad part of the original code.

    Secondly, If close() in this refactoring is a problem because close() may throw an exception, than by that same logic any code that may throw an exception poses a problem in a finally block, even a trivial ‘new Object();’. Only the small subset of all possible Java statements that are more or less guaranteed not to throw (runtime) exceptions do not give ‘problems’. Only using a subset of Java in finally blocks is not something I would advocate.

    Thirdly, if the call to fis.close() fails with an exception, then the whole customCopy action fails. If the call to fos.write() fails, the whole customCopy action fails. Either way, customCopy fails. It does not matter if the calling code does not know what part of the customCopy action has failed, because that is an implementation detail. The caller only knows of the method he calls and not how the call is implemented. The call either succeeds or fails throws an exception. Note that what should be logged is different to what should be thrown.

  1. No trackbacks yet.

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s

Follow

Get every new post delivered to your Inbox.

Join 205 other followers