• Post Reply Bookmark Topic Watch Topic
  • New Topic

Interested in is there an objectively better/cleaner way to do this. (Code Review)  RSS feed

 
Greenhorn
Posts: 26
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
So I am a newbie Dev in training and was curious if this code is "good." While I am new and I shouldn't necessarily worry about writing the best code now, I simply want to build good habits early on.

So Critique Me!

EDIT: Totally forgot purpose of program:
PURPOSE: User enters sentence. Program checks if the sentence has a pariod at the end. If it doesn't then the program rejects the sentence and asks until it gets a sentence with a period.

NOTE: Any way I can prohibit users from entering numbers or spaces?


 
lowercase baba
Bartender
Posts: 12565
49
Chrome Java Linux
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I added code tags to your post, to format it better and make it more readable. Next time, you can highlight all your code and click the button above that says "code".

My two comments are:
1) there are not enough comments in this code.
2) the code is not properly formatted.

Those two reasons alone are enough to make me stop looking.

Even in my junk, throw-away methods, at minimum I put in a comment saying what the method is basically expected to do.

I guarantee Campbell R. will tell you there is too much code in your main method.
 
Marshal
Posts: 56600
172
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Welcome to the Ranch

No, I am afraid it isn't. For a start you should restrict your main method to a single statement.
 
Campbell Ritchie
Marshal
Posts: 56600
172
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
fred rosenberger wrote: . . . I guarantee Campbell R. will tell you there is too much code in your main method.
Damn! 33 seconds faster and I would have had that before your post, Fred
 
Campbell Ritchie
Marshal
Posts: 56600
172
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Incorrect format of class name.
No explanation of why you are using such old‑fashioned code for keyboard entry. Potentially confusing use of different modes for input and output.
Incorrect format for local variable names.
Print instructions which print numbers with no explanation.
Dangerous use of == false. Confusing assignments to boolean.

 
Bartender
Posts: 2087
44
Firefox Browser IntelliJ IDE Java Linux Spring
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
StringIndexOutOfBoundsException in case of an empty String.
Why not use String.endsWith method?
 
Shaman Dasuta
Greenhorn
Posts: 26
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Edited Version



Couple of Clarifications:

Thanks fred for the code tags! Got the commenting a bit better (totally forgot that).
What is "proper formatting." Specificity would help.

Campbell
Anything wrong with JOptionPane? Or is there a better way?
local variable names? whats wrong with them?

This link was great: main method. Y do through all that work? Im guessing its better for shippable/high level code.

You're providing good feedback but just not any examples of the "right/correct" way to do it. Frankly I'm too new to know that its wrong. So any examples/links would be great.

Pawel! Thanks! Looked up and used String Ends with and that solves the empty String issue as well!

What do you guys think? Thanks for the help so far.

EDIT: Unfortunately if you do everything right(Enter a period or a sentence with a period It still shows the Invalid Entry Message but stops the loop.
 
Shaman Dasuta
Greenhorn
Posts: 26
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Campbell Ritchie wrote:Welcome to the Ranch

No, I am afraid it isn't. For a start you should restrict your main method to a single statement.


Thanks!

Also is the afraid not regarding prohibiting users from input numbers and or odd characters?
 
Shaman Dasuta
Greenhorn
Posts: 26
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Slightly updated to fix any issues:

 
Campbell Ritchie
Marshal
Posts: 56600
172
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Not much, I am afraid.
Those comments do not add to the legibility or comprehensibility of the program. They mostly don't actually tell us anything we don't know already. I think Fred meant to add documentation comments to the public members of that class and to the class itself. You can start reading about documentation comments here. The section starting PURPOSE should be rewritten as a documentation comment.
Your indentation is only slightly better, though it is easier to understand the program now you have the {s on lines by themselves. Note we have some suggestions about formatting.
Class name better, but local variable names no better.
The public constructor is not much of an improvement, since it is no different from the default constructor. You can read about default constructors in the Java® Language Specification (=JLS), though the JLS can be difficult to read. Note the next section of the JLS explains why you might have done better to make that constructor private, and that section is easy to read.
The line about ...endsWith(".") is a great improvement
Still using the error‑prone and unstylish == false. It now appears twice
Never if (b == true)... or if (b == false)...
Always if (b)... or if (!b)...
I would use a Scanner for keyboard input.

And most of that code needs to come out of the main method, as you will have seen from the FAQ.
 
Campbell Ritchie
Marshal
Posts: 56600
172
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Shaman Dasuta wrote: . . . Y do through all that work? . . .
Because you want to be able to write a program with more than ten lines and it still be legible.

And it's not “Y” but “Why”, please.


EDIT: Unfortunately if you do everything right(Enter a period or a sentence with a period It still shows the Invalid Entry Message but stops the loop.
I tried your code unchanged and it worked nicely. Beware of option pane dialogues; they can appear behind other windows, so you may have to minimise everything else on screen to find them.
Class name should be EndOfSentence with O rather than o, please.
 
Bartender
Posts: 1801
28
Chrome Eclipse IDE Firefox Browser jQuery Linux MySQL Database Netbeans IDE
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Regarding the variable names, boolean variables typically start with "is", so you should use "isCorrect" or "isValid".


is equivalent to:

while being more readable and it avoids the possibility of accidently writing "if (isCorrect = true)" which will drive you crazy trying to locate that bug.

Ditto for


 
J. Kevin Robbins
Bartender
Posts: 1801
28
Chrome Eclipse IDE Firefox Browser jQuery Linux MySQL Database Netbeans IDE
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Shaman Dasuta wrote:Slightly updated to fix any issues:

Variables should always start with lowercase and be descriptive. What if the user enters a number? or nothing? or a single word?

I would call it "userInput".
 
Paweł Baczyński
Bartender
Posts: 2087
44
Firefox Browser IntelliJ IDE Java Linux Spring
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
One more thing to consider if you still want to use JOptionPane.
What does showInputDialog() return if the user presses cancel or exit button?
 
Shaman Dasuta
Greenhorn
Posts: 26
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Paweł Baczyński wrote:One more thing to consider if you still want to use JOptionPane.
What does showInputDialog() return if the user presses cancel or exit button?


Good Catch. If I press cancel or exit Sentence is a null value so no good.

Sentence to userInput is a great idea. Ill do it.

Only reason I'm using JOptionPane is for its GUI. I wanted a user friendly way to enter a sentence and get feedback. I know with scanner I can just add sentences to the console window but how can I get a dialog box with text entry from it?

Also in Eclipse IDE (what I am coding in). How can I rename a Class? If I right click there is no rename option.

Also Is there a good link that explains the private, public, and method constructors? I looked at this link but Im looking for something that explains it in better detail: http://www.coderanch.com/how-to/java/MainIsAPain
 
Paweł Baczyński
Bartender
Posts: 2087
44
Firefox Browser IntelliJ IDE Java Linux Spring
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Shaman Dasuta wrote:Also in Eclipse IDE (what I am coding in). How can I rename a Class? If I right click there is no rename option.

Refactor -> Rename
 
  • Post Reply Bookmark Topic Watch Topic
  • New Topic
Boost this thread!