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.
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.
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?
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 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.
Those are the largest trousers in the world! Especially when next to this ad:
a bit of art, as a gift, that will fit in a stocking