2
\$\begingroup\$

I am happy to receive all recommendations for improvement. But below I am mostly interested in a review of my handling of the exception thrown by the close() method of RandomAccessFile.

The FileNotFoundException thrown by the opening of a file (in the constructor of RandomAccessFile) is declared as thrown by my method, because the caller can act on it.

But the exception thrown by the close() method on that file cannot be handled the caller, so I repackage it in a RuntimeException.

private boolean includeArticle(File articleFile, Map<String, String> conditionMap)
  throws FileNotFoundException 
{
  if (doesFileQualifyAsArticle(articleFile)) {
    /* This can throw a FileNotFoundException, which the caller should know about. */
    RandomAccessFile raFile = new RandomAccessFile(articleFile, "r");
    if (this.isToInclude(raFile, conditionMap)) {
      return true;
    }
    try {
      raFile.close();
    } catch (IOException e) {
      throw new RuntimeException(e);
    }
  }
  return false;
}
\$\endgroup\$
7
\$\begingroup\$

At least in code that I've seen, it is very common to ignore exceptions when closing a resource. In fact Apache Commons IO has a number of closeQuietly functions just for this purpose.

Also, the work being done with raFile should be wrapped in a try, with the close/closeQuietly being called in the finally. This ensures that close/closeQuietly is called even if an exception occurs while working with the file.

RandomAccessFile raFile = new RandomAccessFile(articleFile, "r");
try {
  if (this.isToInclude(raFile, conditionMap)) {
    return true;
  }
}
finally {
  IOUtils.closeQuietly(raFile);
}

Also, if you are using Java 7, you might want to check out the new try-with-resources statement.

\$\endgroup\$
  • \$\begingroup\$ I am using Java 7. So I will use try-with-resources. Please correct me if I'm wrong, but even with that I need a finally and a closeQuietly() if I don't want to propogate the IOException from the close(), correct? \$\endgroup\$ – John Fitzpatrick Apr 7 '13 at 5:48
  • \$\begingroup\$ In general, it is not a good idea to ignore close exceptions for/after writing. You could get the same exceptions for the same reasons there as for a normal write call. Which can lead to unexpected behavior then. This could happen only very rarely or never in good environments, but does not make it that better. To the question about try-with: You have to catch it. \$\endgroup\$ – tb- Apr 7 '13 at 13:28
1
\$\begingroup\$

From your description, I assume you only do reading. At least, you only mention FileNotFoundException, no write exceptions. If you do only reading, you can most likely do it in this way, with a small improvement:

....
finally {
    try {
        if (raFile != null)
            raFile.close();
    } catch (IOException e) {
        log.warn(e); // not interesting, we are in read only mode
    }
}

Otherwise, you could throw an additional NullPointerException.

If you need it several times, you could use a static utility method:

public static void closeQuietly(Closeable closeable) {
    try {
        if (closeable != null)
            closeable.close();
    } catch (IOException e) {
        log.warn(e); // we do not care
    }
}

(which is the same implementation as for apache commonsio)

If you do writing, you should throw the exception. Because the final close can lead to the same exceptions as every write (remember, we do probably a last flush before the close and write unwritten data). I would suggest to do only:

finally {
    if (closeable != null)
        closeable.close();
}

or rethrow the exception. This is exactly the same what happens with try-with, see for example here: http://docs.oracle.com/javase/tutorial/essential/exceptions/tryResourceClose.html

And do not use abbreviations.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.