Java File Handling, what did I do wrong?

Question

Wrote up a basic file handler for a Java Homework assignment, and when I got the assignment back I had some notes about failing to catch a few instances:

• Buffer from file could have been null.
• File was not found
• File stream wasn't closed

Here is the block of code that is used for opening a file:

/**
 * Create a Filestream, Buffer, and a String to store the Buffer.
 */
FileInputStream fin = null;
BufferedReader buffRead = null;
String loadedString = null;

/** Try to open the file from user input */
try
{
    fin = new FileInputStream(programPath + fileToParse);
    buffRead = new BufferedReader(new InputStreamReader(fin));
    loadedString = buffRead.readLine();
    fin.close();
}
/** Catch the error if we can't open the file */
catch(IOException e)
{
    System.err.println("CRITICAL: Unable to open text file!");
    System.err.println("Exiting!");
    System.exit(-1);
}

The one comment I had from him was that fin.close(); needed to be in a finally block, which I did not have at all. But I thought that the way I have created the try/catch it would have prevented an issue with the file not opening.

Let me be clear on a few things: This is not for a current assignment (not trying to get someone to do my own work), I have already created my project and have been graded on it. I did not fully understand my Professor's reasoning myself. Finally, I do not have a lot of Java experience, so I was a little confused why my catch wasn't good enough.

Answer 1

Say buffRead.readLine() throws an exception, will your FileInputStream ever be closed, or will that line be skipped? The purpose of a finally block is that even in exceptional circumastances, the code in the finally block will execute.

Answer 2

But what if your program threw an exception on the second or third line of your try block?

buffRead = new BufferedReader(new InputStreamReader(fin));
loadedString = buffRead.readLine();

By this point, a filehandle has been opened and assigned to fin. You could trap the exception but the filehandle would remain open.

You'll want to move the fin.close() statement to a finally block:

} finally {
    try {
        if (fin != null) {
            fin.close();
        }
    } catch (IOException e2) {
    }
}

Answer 3

• Buffer from file could have been null.

The file may be empty. That is, end-of-file is reach upon opening the file. loadedString = buffRead.readLine() would then have returned null.

Perhaps you should have fixed this by adding something like if (loadedString == null) loadedString = "";

• File was not found

As explained in the documentation of the constructor of FileInputStream(String) it may throw a FileNotFoundException. You do catch this in your IOException clause (since FileNotFoundException is an IOException), so it's fine, but you could perhaps have done:

} catch (FileNotFoundException fnfe) {
    System.err.println("File not fonud!");
} catch (IOException ioex {
    System.err.println("Some other error");
}

• File stream wasn't closed

You do call fin.close() which in normal circumstances closes the file stream. Perhaps he means that it's not always closed. The readLine could potentially throw an IOException in which case the close() is skipped. That's the reason for having it in a finally clause (which makes sure it gets called no matter what happens in the try-block. (*)

---

(*) As @mmyers correctly points out, putting the close() in a finally block will actually not be sufficient since you call System.exit(-1) in the catch-block. If that really is the desired behavior, you could set an error flag in the catch-clause, and exit after the finally-clause if this flag is set.

Answer 4

There are a lot of other errors which may happen other than opening the file.

In the end you may end up with a fin which is defined or not which you have to protect against null pointer errors, and do not forget that closing the file can throw a new exception.

My advice is to capture this in a separate routine and let the IOExceptions fly out of it :

something like

private String readFile() throws IOException {
  String s;
  try {
    fin = new FileInputStream(programPath + fileToParse);
    buffRead = new BufferedReader(new InputStreamReader(fin));
    s = buffRead.readLine();
    fin.close();
  } finally {
    if (fin != null {
       fin.close()
    }
  }
  return s
}

and then where you need it :

try {
   loadedString = readFile();
} catch (IOException e) {
   // handle issue gracefully
}