Beliebte Suchanfragen

Cloud Native

DevOps

IT-Security

Agile Methoden

Java

|
//

Clean Code: refactoring a double try block

18.5.2010 | 2 minutes of reading time

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 🙂

|

share post

Likes

0

//

More articles in this subject area

Discover exciting further topics and let the codecentric world inspire you.

//

Gemeinsam bessere Projekte umsetzen.

Wir helfen deinem Unternehmen.

Du stehst vor einer großen IT-Herausforderung? Wir sorgen für eine maßgeschneiderte Unterstützung. Informiere dich jetzt.

Hilf uns, noch besser zu werden.

Wir sind immer auf der Suche nach neuen Talenten. Auch für dich ist die passende Stelle dabei.