Variants of stream closing pattern in Java

Question

I often see the following pattern both in Java documentation and in other people's code when dealing with streams:

FileInputStream fis = null;
try {
  fis = new FileInputStream("some.file");
  // do something useful with fis
} finally {
  if (fis != null) {
    fis.close();
  }
}

However I personally prefer a different pattern:

FileInputStream fis = new FileInputStream("some.file");
try {
  // do something useful with fis
} finally {
  fis.close();
}

I like the brevity of the latter and I think it's correct. But am I right about correctness? Is there an objective reason to prefer one over another?

Update

Even if I write the examples pretty much how I would write such code in real life my pattern is still more concise. Compare the documentation approach:

public Object processFile(String fn) throws MyException {
  FileInputStream fis = null;
  try {
    fis = new FileInputStream(fn);
    return new Object(); // somehow derived from fis
  } catch (FileNotFoundException e) {
    throw new MySubExceptionForNotFound(e);
  } catch (IOException e) {
    throw new MySubExceptionForIoError(e);
  } finally {
    if (fis != null) {
      IOUtils.closeQuietly(fis);
    }
  }
}

with my approach:

public Object processFile(String fn) throws MyException {
  try {
    FileInputStream fis = new FileInputStream(fn);
    try {
      return new Object(); // somehow derived from fis
    } finally {
      IOUtils.closeQuietly(fis);
    }
  } catch (FileNotFoundException e) {
    throw new MySubExceptionForNotFound(e);
  } catch (IOException e) {
    throw new MySubExceptionForIoError(e);
  }
}

Mine is one line shorter! :D

Interestingly my code does not change if you decide to use fis.close() instead of IOUtils.closeQuietly() while the "official" code will grow another 4 lines.

@assylias yeah it's the right thing to do, thanks. But a) it only works in Java 7, and b) the question stands: why obviously inferior pattern is used throughout the documentation? One would think there ought to be a reason... — SnakE, Aug 13 '14 at 23:48

score 1 · Answer 1 · answered Aug 13 '14 at 22:39

1

The second example that you have posted would not try/catch the possible errors that ... = new FileInputStream("..."); will return if it fails loading the file. In the first example though you automatically also catch the errors that the FileInputStream gives.

I would personally go with the first one to have better error handling.

answered Aug 13 '14 at 22:39

ihsan

576
2
5
18

If `new FileInputStream()` throws then `fis` remains `null` hence there is nothing to close. The first example in my question would check `fis` and skip `close()`. The second example wouldn't even enter the `try`. The net result would be exactly the same. – SnakE Aug 13 '14 at 23:41
Thanks, but my point was that FileInputStream DOES throw and exception if something goes wrong, so you can also make checks on that. – ihsan Aug 14 '14 at 09:15

score 1 · Answer 2 · answered Aug 13 '14 at 22:40

1

The second example does not compile unless you throw the exception from the method, because FileNotFoundException is a checked exception.

Compile.java:5: error: unreported exception FileNotFoundException; must be caught or declared to be thrown
        FileInputStream fis = new FileInputStream("some.file");

The first example also does not compile, because you are missing the appropriate catch block. Here is a compiling example.

import java.io.*;
public class Compile {

    public static void main(String[] args) {
        FileInputStream fis = null;
        try {
            fis = new FileInputStream("some.file");
            // do something useful with fis

            if (fis != null) {
                fis.close();
            }
        }
        catch (FileNotFoundException fnf) {
        }
        catch (IOException ioe) {
        }
        finally {
        }
    }
}

answered Aug 13 '14 at 22:40

merlin2011

71,677
44
195
329

The constructor can throw an unchecked SecurityException at runtime too if there's a security manager configured. – Trevor Tippins Aug 13 '14 at 22:43
@TrevorTippins, That is certainly true, but that is not a checked exception, and I am only addressing compile-time errors. – merlin2011 Aug 13 '14 at 22:44
My code snippets weren't supposed to be full working Java applications. And they weren't supposed to catch all exceptions. You don't always catch all exceptions, do you? Just add a `throws IOException` clause to your `main()` declaration and both my examples will compile as is. – SnakE Aug 13 '14 at 23:57
@SnakE, Most real applications I see do not have `main` throwing Exceptions up to the user. – merlin2011 Aug 14 '14 at 07:37
@merlin2011, most real application don't process files in their `main()`. Also I prefer to throw unhandled exceptions from main() when an application is supposed to be run as an unattended sub-process. This way exceptions can be logged by the parent. – SnakE Aug 14 '14 at 17:03
@SnakE, That is a fair point. I guess it depends on your use case. – merlin2011 Aug 14 '14 at 18:54

Darth Android · Answer 3 · 2014-08-14T15:05:18.887

1

Use try-with-resources:

try (FileInputStream fis = new FileInputStream("some.file")) {
    // some code here
}

To answer the question of why the recommended pattern is the way it is, consider the fact that new FileInputStream(String) throws a checked exception that you should probably catch; The

FileInputStream fis = null;
try {
    fis = new FileInputStream("somefile.txt");
} catch (FileNotFoundException e) {
    return false;
} finally {
    if (fis != null) {
        fis.close();
    }
}

allows you to handle it in the same try/catch block as your other file-related exceptions. This has brevity and reduced nesting advantages over the following:

try {
    FileInputStream fis = new FileInputStream("some.file");
    try {
        // do something useful with fis
    } finally {
        fis.close();
    }
} catch (FileNotFoundException e) {
    return false;
}

And if you're not catching the exception and just letting it propagate, then your suggestion is cleaner - however given the heavy use of checked exceptions in the IO libraries (including from class constructors), catching and handling an exception is common enough, and I think it makes sense to have just one pattern for tutorial purposes instead of showing someone a slightly different pattern each time.

edited Aug 14 '14 at 15:05

answered Aug 13 '14 at 22:44

Darth Android

3,437
18
19

It is polite to attribute this idea to assylias if you're going to write it into an answer. – merlin2011 Aug 13 '14 at 22:50
A good one but it only works in Java 7 which is not always available. And it does not answer the question: why a certain pattern is used all over the official documentation when there is a better one? – SnakE Aug 13 '14 at 23:52
@SnakE See my updated answer for a possible reasoning; merlin2011: assylias' comment was not posted when I started writing my answer, and was not the source of inspiration for mine o.o – Darth Android Aug 14 '14 at 15:07
@DarthAndroid In the same vein I might argue that `fis.close()` also throws a checked exception that you should* probably catch. Add that to both snippets and see which has more brevity and readability. *Arguably you shouldn't but you won't see such a recommendation in official documentation. – SnakE Aug 14 '14 at 16:26

Variants of stream closing pattern in Java

Update

3 Answers3