• Post Reply Bookmark Topic Watch Topic
  • New Topic
programming forums Java Mobile Certification Databases Caching Books Engineering Micro Controllers OS Languages Paradigms IDEs Build Tools Frameworks Application Servers Open Source This Site Careers Other Pie Elite all forums
this forum made possible by our volunteer staff, including ...
Marshals:
  • Tim Cooke
  • Campbell Ritchie
  • paul wheaton
  • Ron McLeod
  • Devaka Cooray
Sheriffs:
  • Jeanne Boyarsky
  • Liutauras Vilda
  • Paul Clapham
Saloon Keepers:
  • Tim Holloway
  • Carey Brown
  • Piet Souris
Bartenders:

Handling exceptions of FileInputStream and XSSFWorkbook

 
Greenhorn
Posts: 5
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
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:


 
Marshal
Posts: 80652
476
  • Likes 1
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
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.
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?
 
Sheriff
Posts: 28399
100
Eclipse IDE Firefox Browser MySQL Database
  • Likes 1
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

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
Marshal
Posts: 80652
476
  • Likes 1
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
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
Marshal
Posts: 80652
476
  • Likes 1
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

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

Yes, I said so too.
 
Paul Clapham
Sheriff
Posts: 28399
100
Eclipse IDE Firefox Browser MySQL Database
  • Likes 1
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

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.
 
With a little knowledge, a cast iron skillet is non-stick and lasts a lifetime.
reply
    Bookmark Topic Watch Topic
  • New Topic