Handling exceptions of FileInputStream and XSSFWorkbook

[Ramaram Shankar]

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.

Code:

/**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());
        }
    }
}

[Campbell Ritchie]

Welcome to the Ranch

I shall add you to our “general” and IO fora.
Your documentation comment should say a lot more. It doesn't describe what the method does, nor under what circumstances the Exception would be thrown.

/**
 * ...
 * @throws Exception if XYZ occurs
 */
// ...

There is a discrepancy between the declared exceptions (line 5 throws....), the documentation comment, and what exceptions the method actually throws. I don't think it will throw any checked exceptions because you have caught all the likely ones.
You are using legacy code; File has been regarded as legacy code since Java7 in 2009.
Why are you catching two different exception types and doing the same action for each? You only need catch (IOException ie).
Your use of finally is clunky, with lots of repeated code, but will work correctly. At least I think it will; somebody will tell you soon enough if I am mistaken I think you should push whoever says not to use try‑with‑resources into the canal. I couldn't find any information about XSSFWorkbook; does that need to be closed separately?
Do you ever use xlsxFileContent? Is it necessary to initialise it in line 6?

[Paul Clapham]

Campbell Ritchie wrote:Your documentation comment should say a lot more. It doesn't describe what the method does, nor under what circumstances the Exception would be thrown.

As far as I can see, that method doesn't throw any checked exceptions. They are all caught and summarily dispatched to the logger. So declaring that it throws an exception is misleading.

Or perhaps it should throw an exception to indicate to the caller that the processing actually didn't do anything. That's something that code review can't cover without knowing the context in which the method is called.

[Campbell Ritchie]

Please consider letting your user seek the file with a file chooser. That should reduce the risk of exceptions, but it will mean having to use the legacy class File.

[Campbell Ritchie]

Paul Clapham wrote:. . . that method doesn't throw any checked exceptions. . . .

Yes, I said so too.

[Paul Clapham]

Campbell Ritchie wrote:I couldn't find any information about XSSFWorkbook; does that need to be closed separately?

I looked in the documentation and found it does have a close() method, but it looks like you only have to call it if you open it using a particular thing which I've never heard of. Which means that you should really call close(), I suppose. But fortunately it implements AutoCloseable so try-with-resources is the answer to that.

And yes, if it isn't already clear, not using try-with-resources is officially a code smell.