Win a copy of Transfer Learning for Natural Language Processing (MEAP) this week in the Artificial Intelligence and Machine Learning forum!
  • 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 all forums
this forum made possible by our volunteer staff, including ...
Marshals:
  • Campbell Ritchie
  • Tim Cooke
  • Paul Clapham
  • Devaka Cooray
  • Bear Bibeault
Sheriffs:
  • Junilu Lacar
  • Knute Snortum
  • Liutauras Vilda
Saloon Keepers:
  • Ron McLeod
  • Stephan van Hulst
  • Tim Moores
  • Tim Holloway
  • Piet Souris
Bartenders:
  • salvin francis
  • Carey Brown
  • Frits Walraven

Reading from file - critique my code

 
Greenhorn
Posts: 21
1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I've been trying to follow advice from this amazing forum and from now on my objective isn't only to pass the tests, but also write using good practices. So I'll be posting some exercises to be critiqued:

For this exercise, my changes were:
1 - Avoid writing all the code in the main method
2 - Pay closer attention to exception throwing
3 - Close the stream properly

Just FYI, the "Solution" class was given beforehand by the exercise.

Please tear my code apart!

 
Rancher
Posts: 157
14
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
First of all, I would avoid using these "static" methods that all get called from main. Rather, have the main method instantiate your class and call a method on it, and then have all other methods used not be declared static. For example:


Methods should be declared private unless there is a reason to not declare them private. The method countCommas has no access modifier, so it is protected, not private.

I don't think countCommas should declare that it throws an IOException
You catch IOException here. You could also move the lines above (that may throw IOException) into this try block.

So it can't throw any IOException.

Speaking of exception handling - displaying the stack trace is rarely the correct way of doing so.
The user doesn't want to see a stack trace. It means nothing to them, and it exposes some of the code internals which isn't a good thing.
Rather consider - why would an IOException be thrown and what does it mean for the user? How about FileNotFound exception, what does that mean to the user? How can you tell them what went wrong in a nice way.


I don't like this line.
What is the meaning of 44?
Of course I know what the meaning is because I know ASCII but others might not.
If you have to use a number like this, at least make it a named constant so it shows its meaning clearly. You could also probably say i == ','. Not entirely sure if that'll compile but it's worth trying.


This code would just display a number to the user. What does that number mean? You could say: System.out.println("Number of commas: " + count);


This comment is unnecessary.


Not that there's anything wrong with this, but why not just use a Scanner? It's probably simpler.
Actually you could use a Scanner for reading the file too, instead of a FileInputStream. Scanner is the simplest solution for text-only files.


I would add Javadoc comments on this, explaining the intention of the code, assumptions, etc. Anything that's useful to know about it.

General comment: this method could be split into two. One method to prompt a file, and one to count the commas from the file.


This readLine is entirely unprompted. If the user runs this application, they will be greeted with a blank screen.
Instead, prompt them:
System.out.print("Enter a filename to count the commas of: ");
 
Pablo Aguirre
Greenhorn
Posts: 21
1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Zachary Griggs, Fantastic stuff, thank you so much for the comments, I'll definitely think about all of that for writing code from now on. From your comments I realised I have zero consideration for readability.
 
Pablo Aguirre
Greenhorn
Posts: 21
1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Zachary Griggs wrote:

I don't think countCommas should declare that it throws an IOException
You catch IOException here. You could also move the lines above (that may throw IOException) into this try block.

So it can't throw any IOException.



I'm doing my next exercise and trying to close a method using the finally block and I'm getting an error telling me I should declare the method throws an IOException.

I tried moving the declarations into the try block but then the variable can't be reached. Any thoughts? Here's the link for the image:

https://imgur.com/a/fqD5LIN

Edit:

I fixed it using the code below but not sure if it's correct.

 
Marshal
Posts: 68904
275
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
PA: you managed to lose the [/quote] part of the quoted test, but don't worry: I added it for you. You can now see where the part of ZG's post you are answering ends. And you are right only to keep a small part of the preceding post in the quote

Afraid that isn't the correct way to close resources. Have a look at the Java8 version of the Java™ Tutorials, which shows you the old way to close resouirces.Isn't that horrible. It also shows why many people try to avoid using two resources simultaneously. You start off declaring reader and writer outside the try but the only thing you can assign them to is null, otherwise you need exception handling. So you end up with reader and writer having too wide a scope. And if anything goes wrong in lines 5 and 6, you still have the references pointing to null, so you need the test for nullity. There is are simpler versions of that code available. But even those have more finally than reading code. So, to reduce the mess, Java7 introduced a new way to close resources (see tutorial link above).The tutorial which satya Priya Sundar quoted here says that there is an alternative form of that code in Java9+, which I have probably written wrongly like this:-Note I have had to add extra {} to restrict the scope of reader and writer.
 
Marshal
Posts: 25436
65
Eclipse IDE Firefox Browser MySQL Database
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Campbell Ritchie wrote:Isn't that horrible...



Not to mention that the copy-and-paste editing that usually gets done to write that sort of code can lead to subtle errors. Like line 17 in the excerpt from Campbell's code which will lead to the writer not being closed.
 
Bartender
Posts: 7065
65
Eclipse IDE Firefox Browser MySQL Database VI Editor Java Windows
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Interesting syntax enhancement but it appears that if the constructor for writer throws an exception then reader will not get closed. Can anyone substantiate this?
 
Carey Brown
Bartender
Posts: 7065
65
Eclipse IDE Firefox Browser MySQL Database VI Editor Java Windows
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Good.close() is never called.
 
Sheriff
Posts: 7051
184
Eclipse IDE Postgres Database VI Editor Chrome Java Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
The syntax seems kind of useless because of that second try block.
The line that creates FileReader (13) still has an unhandled exception as does the creation of the BufferedWriter with the new FileWriter inside (17).  Either way, it doesn't seem like a very useful construct.
 
Saloon Keeper
Posts: 3893
154
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Excellent example. This syntax is new to me, but the old syntax works as expected, with Good being closed.
 
Campbell Ritchie
Marshal
Posts: 68904
275
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Paul Clapham wrote:. . . Like line 17 in the excerpt from Campbell's code . . .

I couldn't have made a better error if I had tried And somebody pointed out there is an error in line 3 of the last code block I showed; I am leaving finding that as an exercise for the reader.
 
Campbell Ritchie
Marshal
Posts: 68904
275
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Piet Souris wrote:. . . This syntax is new to me, but the old syntax works as expected . . .

It was new to me, too, until I saw that tutorial link, but the older usage of try with resources appears to be more reliable., with Good being closed
 
Paul Clapham
Marshal
Posts: 25436
65
Eclipse IDE Firefox Browser MySQL Database
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Campbell Ritchie wrote:

Paul Clapham wrote:. . . Like line 17 in the excerpt from Campbell's code . . .

I couldn't have made a better error if I had tried



But the moral of the story is: The more code you write, the more errors you're going to have. So stop writing so much code! (Try-with-resources is an excellent way of getting rid of tiresome error-prone code.)
 
Ranch Foreman
Posts: 95
6
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Carey Brown wrote:Good.close() is never called.


I may can be wrong - but isn't Closeable the wrong interface? Isn't it rather AutoCloseable that's used for try-with-resources, is it?
 
Saloon Keeper
Posts: 11882
253
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Closeable extends AutoCloseable.

When writing a class that is composed of resources that need to be disposed of, it's always good to implement AutoCloseable, but if Closeable is appropriate it should be preferred.

Why Closeable? Its contract explicitly states that the close() operation is idempotent.

When Closeable? The class or its components perform I/O-like operations.

Note that even if Closeable is not appropriate and you want to use AutoCloseable instead (for instance, if your class represents a cryptographic secret and you want to clear its memory contents as soon as you don't need the secret anymore), it's still a good idea to make the close() method idempotent. The difference is that the client of your method won't know for sure that it is.
 
Live ordinary life in an extraordinary way. Details embedded in this tiny ad:
Two software engineers solve most of the world's problems in one K&R sized book
https://coderanch.com/wiki/718759/books/Building-World-Backyard-Paul-Wheaton
    Bookmark Topic Watch Topic
  • New Topic