Clean Code: Refactoring eines doppelten try-Blocks

Ich habe kürzlich beim Aufräumen einer alten Codebase ein Codesnippet gefunden, das schon auf den ersten Blick verdächtig aussah. Beim Refactoring stellte sich heraus dass es nicht nur unschön war, sondern je nach Szenario auch eine Exception verschluckt werden kann. Ich erkläre in diesem Beitrag, was das Problem war und wie es behoben wurde.

Gehen wir vom eigentlichen Phänomen abstrahiert von folgendem Code aus:

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}

Beim Betrachten des Codes kann man sich zu Recht fragen, welche Exception im catch-Block ankommt:

• Beide Exceptions?
• Nur die NullPointerException?
• Nur die IllegalArgumentException?
• Gar keine Exception?

Hier die Konsolenausgabe wenn man obigen Code ausführt:

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

Offensichtlich wird also die ursprüngliche NullPointerException „verschluckt“ und kommt niemals im catch-Block an. Das allein ist schon unschön, abgesehen davon dass zwei verschachtelte try-Blöcke sehr eigenartig wirken. Im obigen Beispiel hat man allerdings noch den Vorteil, dass Eclipse den finally-Block gelb markiert und mitteilt, dass dieser nicht normal endet.

Nun zum ursprünglichen Kontext und dem „echten“ Code-Snippet:

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("Fehler beim Schreiben von " + filename + ": " + e.getMessage(), e);
13}

Was ist schlecht an diesem Code? Erstens haben wir hier das gleiche verschachtelte try-try-Konstrukt wie oben im allgemeinen Beispiel. Dies kann also potenziell zum Verschlucken der ursprünglichen Exception führen. Außerdem ist hier das Problem einer potenziellen zusätzlichen Exception im finally-Block nicht mithilfe von Eclipse erkennbar, da FileOutputStream#close() eine IOException wirft. Da diese von dem catch-Block gefangen wird, meckert Eclipse den finally-Block nicht an. Grundsätzlich kann man über diesen Code keine Aussage treffen, ob er sich immer wie gewünscht verhält.

Um diesen Code zu verbessern, haben wir den finally-Block hinter den catch-Block verschoben und außerdem IOUtils#closeQuietly(OutputStream) verwendet, was eventuell auftretende Exceptions beim Schließen des Streams für sich behält. Das Ergebnis:

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

Damit ist der Code nicht nur qualitativ besser, er funktioniert jetzt auch definitiv wie beabsichtigt und ist im gleichen Zuge wesentlich besser lesbar.

Eine Anmerkung zu closeQuietly: es handelt sich hierbei keineswegs um eine Patentlösung für das Schließen jeder Art von Stream. Selbstverständlich sollte man closeQuietly nur dann verwenden, wenn man genau weiß dass man die Exception auf keinen Fall sehen will. Falls man dazu aber noch etwas loggen oder sonst etwas zur Behandlung der Exception tun möchte, sollte man natürlich den klassischen Weg gehen und im finally-Block nochmal try-catch einbauen. Den Code der dabei herauskommt darf sich jeder selbst ausdenken 🙂