The variable fr only has scope within the try block. It is out of scope in the finally block. You need to declare it before the try block:
FileReader fr = null;
try {
fr = new FileReader(file);
BufferedReader br = new BufferedReader(fr);
String line = null;
} catch (FileNotFoundException e) {
e.printStackTrace();
} finally {
if (fr != null) {
try {
fr.close();
} catch (IOException e) {
// This is unrecoverable. Just report it and move on
e.printStackTrace();
}
}
}
This is quite a common pattern of code, so it's good to remember it for future similar situations.
Consider throwing IOException from this method - printing track traces isn't very helpful to callers, and you wouldn't need the nested try catch around fr.close()
The variable fr only has scope within the try block. It is out of scope in the finally block. You need to declare it before the try block:
FileReader fr = null;
try {
fr = new FileReader(file);
BufferedReader br = new BufferedReader(fr);
String line = null;
} catch (FileNotFoundException e) {
e.printStackTrace();
} finally {
if (fr != null) {
try {
fr.close();
} catch (IOException e) {
// This is unrecoverable. Just report it and move on
e.printStackTrace();
}
}
}
This is quite a common pattern of code, so it's good to remember it for future similar situations.
Consider throwing IOException from this method - printing track traces isn't very helpful to callers, and you wouldn't need the nested try catch around fr.close()
Now finally block is not needed,
try (FileReader fr = new FileReader(file);
BufferedReader br = new BufferedReader(fr);){
String line = null;
}
} catch(FileNotFoundException fnf) {
fnf.printStackTrace();
}
now automatically close your readers
This is the correct idom (and it works fine):
InputStream in = null;
OutputStream out = null;
try {
in = new FileInputStream(inputFileName);
out = new FileOutputStream(outputFileName);
copy(in, out);
finally {
close(in);
close(out);
}
public static void close(Closeable c) {
if (c == null) return;
try {
c.close();
} catch (IOException e) {
//log the exception
}
}
The reason this works fine is that the exception thrown before you got to finally will be thrown after your finally code finishes, provided that your finally code doesn't itself throw an exception or otherwise terminate abnormally.
Edit: As of Java 7 (and Android SDK 19 - KitKat) there is now a Try with resources syntax to make this cleaner. How to deal with that is addressed in this question.
You could implement a utility method:
public final class IOUtil {
private IOUtil() {}
public static void closeQuietly(Closeable... closeables) {
for (Closeable c : closeables) {
if (c != null) try {
c.close();
} catch(Exception ex) {}
}
}
}
Then your code would be reduced to:
try {
copy(in, out);
} finally {
IOUtil.closeQuietly(in, out);
}
Additional
I imagine there'll be a method like this in a 3rd party open-source library. However, my preference is to avoid unnecessary library dependencies unless I'm using a large portion of its functionality. Hence I tend to implement simple utility methods like this myself.
The reason, which as I recall Bloch explains, is that the interesting Exception is the original one thrown from within the try block. Any Exception thrown within the finally block will hide the original Exception.
Imagine that the ur-Exception is because the File is on a network and the network is down. That's the more informative Exception that you want to throw. But that problem also affects the close() , and you don't want the 2nd exception to "get in the way".
Note this explanation was in V1 of his book...
Added The following code proves the point:
try {
throw new Exception("in try, here is the ur-Exception");
}
finally {
throw new Exception("in finally");
}
The second, "in finally" Exception, is the one you will see, hiding the original Exception.
For argument’s sake, I can’t imagine how this operation could fail, and what could go wrong if it fails.
Therefore, I would do nothing except logging in production. And make sure that the app runs into a breakpoint when running on a developers machine. If this ever fails, I want to know about it, and why it happened, so I can figure out more correctly how to handle it.
The cleanest approach would be to do it using the try-with-resources statement as shown below:
private static void writetoFiles(String error) throws IOException {
//...
try (FileWriter updateerrorcode = new FileWriter("errorcode.txt")) {
updateerrorcode.write(error);
}
//...
}
Do not catch an exception in a method if it can not handle it:
If the method, writetoFiles can not handle the exception, it should throw the same so that the calling method can handle it appropriately.
Use a try-with-resource statement:
private static void writetoFiles(String error) {
try {
File file = new File("errorcode.txt");
if (!file.exists()) {
file.createNewFile();
} else {
try (FileWriter updateerrorcode = new FileWriter("errorcode.txt")) {
updateerrorcode.write(error);
}
}
} catch (IOException e) {
// TODO: Handle error condition
}
}
To point out a separate issue...I think your logic is wrong in your example. If the output file doesn't exist, all your code does is create the file. Only if the file already exists does it write the error text to it. I expect that you want to write the text in either case. If this is true, you don't need the createNewFile call at all, as the FileWriter class will create the file if it doesn't already exist. So I think what you really want is this:
private static void writetoFiles(String error) {
try (FileWriter updateerrorcode = new FileWriter("errorcode.txt")) {
updateerrorcode.write(error);
} catch (IOException e) {
// TODO: Handle error condition
}
}
This will cause the writer to be properly closed in both the normal execution case and the error throw case. I assume that in your actual code, you'll do something with that IOException when it is caught. I can't know what you want to do there, so I won't propose anything.
If you want to strictly use a finally block, you can do this instead:
FileWriter updateerrorcode = new FileWriter("errorcode.txt");
try {
updateerrorcode.write(error);
}
catch (IOException e) {
// TODO: Handle error condition
}
finally {
updateerrorcode.close();
}
This is the only option you would have had in earlier versions of Java, prior to the addition of the try-with-resource construct. In this second method, you might want to catch an error from close(), but in all of my 25+ years of experience with Java, I don't recall a close() call on a file failing. I guess you'd get that if you were out of disk space on your target volume and so close() couldn't flush the stream's write buffer. This issue is a distinct advantage of the newer method...failure to close the file won't affect the throw/catch of an exception thrown by the write() call.
Every situation is a little bit different, but it's best to always close in a finally block OR use java's try with resources syntax, which is effectively the same thing.
The risk you run by not closing in the finally block is that you end up with an unclosed stream object after the catch gets triggered. Also, if your stream is buffered, you might need that final close to flush the buffer to ensure that the write is fully complete. It may be a critical error to complete without that final close.
Not closing the file may mean the stream contents don't get flushed in a timely way. This is a concern in the case that the code completes normally and doesn't throw an exception.
If you don't close a FileOutputStream, then its finalize method will try to close it. However, finalize doesn't get called immediately, it is not guaranteed to get called at all, see this question. It would be better not to rely on this.
If JDK7's try-with-resources is an option then use it:
try (OutputStream out = new FileOutputStream(file);) {
out.write(data);
}
Otherwise, for JDK6, make sure the exception thrown on close can't mask any exception thrown in the try block:
OutputStream out = new FileOutputStream(file);
try {
out.write(data);
} finally {
try {
out.close();
} catch (IOException e) {
Log.e("Exception", "File write failed: " + e.toString());
}
}
It may be verbose but it makes sure the file gets closed.
Note that the following is only applicable for Java 6 and earlier. For Java 7 and later, you should switch to using try-with-resources ... as described in other answers.
If you are trying to catch and report all exceptions at source (in Java 6 or earlier), a better solution is this:
ObjectOutputStream oos = null;
try {
oos = new ObjectOutputStream(new FileOutputStream(file));
oos.writeObject(shapes);
oos.flush();
} catch (FileNotFoundException ex) {
// complain to user
} catch (IOException ex) {
// notify user
} finally {
if (oos != null) {
try {
oos.close();
} catch (IOException ex) {
// ignore ... any significant errors should already have been
// reported via an IOException from the final flush.
}
}
}
Notes:
- The standard Java wrapper streams, readers and writers all propagate
closeandflushto their wrapped streams, etc. So you only need to close or flush the outermost wrapper. - The purpose of flushing explicitly at the end of the try block is so that the (real) handler for
IOExceptiongets to see any write failures1. - When you do a close or flush on an output stream, there is a "once in a blue moon" chance that an exception will be thrown due to disc errors or file system full. You should not squash this exception!.
If you often have to "close a possibly null stream ignoring IOExceptions", then you could write yourself a helper method like this:
public void closeQuietly(Closeable closeable) {
if (closeable != null) {
try {
closeable.close();
} catch (IOException ex) {
// ignore
}
}
}
then you can replace the previous finally block with:
} finally {
closeQuietly(oos);
}
Another answer points out that a closeQuietly method is already available in an Apache Commons library ... if you don't mind adding a dependency to your project for a 10 line method.
But be careful that you only use closeQuietly on streams where IO exceptions really are irrelevant.
UPDATE : closeQuietly is deprecated in version 2.6 of the Apache Commons API. Java 7+ try-with-resources makes it redundant.
On the issue of flush() versus close() that people were asking about in comments:
The standard "filter" and "buffered" output streams and writers have an API contract that states that
close()causes all buffered output to be flushed. You should find that all other (standard) output classes that do output buffering will behave the same way. So, for a standard class it is redundant to callflush()immediately beforeclose().For custom and 3rd-party classes, you need to investigate (e.g. read the javadoc, look at the code), but any
close()method that doesn't flush buffered data is arguably broken.Finally, there is the issue of what
flush()actually does. What the javadoc says is this (forOutputStream...)If the intended destination of this stream is an abstraction provided by the underlying operating system, for example a file, then flushing the stream guarantees only that bytes previously written to the stream are passed to the operating system for writing; it does not guarantee that they are actually written to a physical device such as a disk drive.
So ... if you hope / imagine that calling
flush()guarantees that your data will persist, you are wrong! (If you need to do that kind of thing, look at theFileChannel.forcemethod ...)
Current best practice for try/catch/finally involving objects that are closeable (e.g. Files) is to use Java 7's try-with-resource statement, e.g.:
try (FileReader reader = new FileReader("ex.txt")) {
System.out.println((char)reader.read());
} catch (IOException ioe) {
ioe.printStackTrace();
}
In this case, the FileReader is automatically closed at the end of the try statement, without the need to close it in an explicit finally block. There are a few examples here:
http://ppkwok.blogspot.com/2012/11/java-cafe-2-try-with-resources.html
The official Java description is at:
http://docs.oracle.com/javase/7/docs/technotes/guides/language/try-with-resources.html