Clean Code: refactoring a double try block

I recently cleaned up some old code and during this adventure, I came across a couple of lines that looked kind of ugly and potentially broken to me. As we refactored this code, we found out that it was not only bad code, but depending on the situation it could also swallow an exception. In this article, I’ll tell you what the problem was and how we resolved it.

Take the following code as an abstract example of what we found:

1public class Test {
2 
3    public static void main(String[] args) {
4        try {
5            try {
6                throw new NullPointerException();
7            } finally {
8                throw new IllegalArgumentException();
9            }
10        } catch(Exception e) {
11            e.printStackTrace();
12        }
13    }
14}

You may ask yourself which of the two exceptions will reach the catch block:

• Both exceptions?
• Only the NullPointerException?
• Only the IllegalArgumentException?
• No exception at all?

If you run this code, you will see this in the console:

1java.lang.IllegalArgumentException
2    at Test.main(Test.java:9)

It’s obvious that the original NullPointerException is “swallowed”, it will never reach the catch block. Additionally, nested try blocks are quite undesirable, at least in this case we’re looking at. At least in the example above, Eclipse will provide you with a warning because the finally block doesn’t complete normally.

Now let’s look at the original context and the original code:

1FileOutputStream out = null;
2try {
3    try {
4        out = new FileOutputStream(filename);
5        out.write(fileContent);
6    } finally {
7        if (out != null) {
8            out.close();
9        }
10    }
11} catch (IOException e) {
12    logger.error("Error writing to " + filename + ": " + e.getMessage(), e);
13}

What’s bad about this code? First, we have the same try-try-sequence as in the abstract example given earlier. This can potentially lead to a situation where the original exception gets swallowed. Also for this code, you will never get a warning from Eclipse: FileOutputStream#close() might throw an IOException, but it would be caught by the catch block, thus Eclipse doesn’t warn you about the finally block. This code has another flaw: you cannot precisely determine whether it’s ever going to behave the way you want it to.

In order to improve this code, we moved the finally block behing the catch block. Additionally, we put IOUtils#closeQuietly(OutputStream) in place in order to knowingly ignore potential exceptions while closing the stream. The result:

1FileOutputStream out = null;
2try {
3    out = new FileOutputStream(filename);
4    out.write(fileContent);
5} catch (IOException e) {
6    logger.error("Error writing to " + filename + ": " + e.getMessage(), e);
7} finally {
8    IOUtils.closeQuietly(out);
9}

This code looks and works much better than before and it definitely does what it’s supposed to do.

Keep in mind that closeQuietly is not the ever-perfect tool in every potential context of closing streams. You should only ever use closeQuietly if you are absolutely sure that you want to ignore an exception and why you want to ignore it. Should you intend to log a message or do anything else in order to handle that exception, you should definitely put a try-catch into the finally block. You may imagine the resulting code for yourself 🙂