Handling exceptions of FileInputStream and XSSFWorkbook

Question

Can you tear my code apart with code review comments? Below is a simple method which reads an XLSX file and does some stuff with it. I use FileInputStream and XSSFWorkbook to do the processing. The code block which I am concerned about is the exception handling (I have been told that it's buggy/not ideal).

Note: I am not allowed to use try-with-resources for some reasons.

/**Process XLSX file.
 * @param filePath Path of file to process
 * @throws Exception
 */
private void processXlsxFile (String filePath) throws IOException {
    String xlsxFileContent = "";
    FileInputStream fis = null;
    XSSFWorkbook workbook = null;
    try {
        File file = new File(filePath);
        fis = new FileInputStream(file);
        workbook = new XSSFWorkbook(fis);
        XSSFSheet sheet = workbook.getSheetAt(0);
        //Do stuff and build 'xlsxFileContent'.
    } catch (FileNotFoundException fe) {
        logger.error(fe.getMessage());
    } catch (IOException ie) {
        logger.error(ie.getMessage());
    } finally {
        try {
            if (workbook != null) {
                workbook.close();
            }
        } catch (IOException ex) {
            logger.error("Exception while closing XSSFWorkbook: " + ex.getMessage());
        }
        try {
            if (fis != null) {
                fis.close();
            }
        } catch (IOException ex) {
            logger.error("Exception while closing FileInputStream: " + ex.getMessage());
        }
    }
}

Answer 1

"throws" declaration

You declare your method to throw IOException, but you catch exceptions inside your method and don't re-throw them, so I see no way how your method might throw it to the outside, meaning that this declaration does not match your actual code.

Informing your caller about failures

If some code calls your method, it does it for some reason, and expects that the job (processing the XLSX file) has been done when the method returns, so that the caller can faithfully continue. The caller needs an information if the call wasn't successful, and your method fails to provide that information.

Nowadays, the preferred way of communicating failure to a caller is by throwing an exception. Your code only produces a log output, which is not accessible to the caller, so it will "happily" continue although most probably the next step will fail as well, not having a processed XLSX file.

So, as a general rule, whenever you exit a method, and it has not done its job, make sure that your caller gets an exception, typically by

• simply letting an exception bubble up that your code got from its inner calls (with the nice feature that this happens automatically, with zero lines of code from your side),
• or by throwing an exception that you newly created to describe the failure reason,
• or by throwing an exception "wrapped around" an original one, so you can provide more context.

The main question is:

• If a strange situation X happens inside my method (e.g. getting an exception), will it (from a caller's point of view) still have done its job?

Most of the time, the answer to this question will be "no", and then your code should throw an exception to its caller. Even not being able to close a file can be a show-stopper, as then the file might be unfinished in the file system, unable to be opened from another piece of code, etc.

Excessive exception handling

About 20 lines of code are about exception handling. That in itself is a code smell, often a result of a misconception about the best use of exceptions. Well-done exception handling allows to concentrate on the "happy path" and have the failure paths handled by the run time system.

Logging

If you throw an exception, don't log the situation. Someone up the caller stack will eventually catch the exception and log it.

If the exception you got does not contain enough context information to describe the situation, then catch it, and throw a new one, containing the missing information, and providing the original exception as its cause ("wrapping" the exception).

By the way, your log messages do not name the file where the problem occurred, and you cannot rely on the IOException to provide this information in its message. So, an administrator reading the log will have a hard time to find out where to look in the file system.

Suggestion

My method would look roughly like this:

private void processXlsxFile (String filePath) throws IOException {
    // This whole outer try/catch is only meant to provide a better exception
    // object, one that names the file.
    // It's optional, up to your discretion (or coding standards)
    try {
        String xlsxFileContent = "";
        FileInputStream fis = null;
        XSSFWorkbook workbook = null;
        try {
            File file = new File(filePath);
            fis = new FileInputStream(file);
            workbook = new XSSFWorkbook(fis);
            XSSFSheet sheet = workbook.getSheetAt(0);
            // Do stuff and build 'xlsxFileContent'.
        } finally {
            // do your best to close the workbook and file
            if (workbook != null) {
                workbook.close();
            }
            // no need to catch exceptions at workbook.close()
            // if it couldn't be closed, closing the file underneath it
            // will probably fail as well, and not improve anything.
            if (fis != null) {
                fis.close();
            }
        }
    } catch (IOException e) {
        // Maybe, other exceptions can arise as well, so feel free to 
        // catch (Exception e) instead.
        // Make sure that your resulting exception names the file.
        throw new IOException("Exception when processing " + filePath, e);
    }
}

One final remark:

Your original code catches both, FileNotFoundException and IOException, and does the same in both cases. That's completely redundant. FileNotFoundException is a subclass of IOException, so a catch (IOException e) will receive any FileNotFoundException as well.