Win a copy of Python Continuous Integration and Delivery this week in the Python 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
  • Liutauras Vilda
  • Bear Bibeault
  • Paul Clapham
  • Jeanne Boyarsky
Sheriffs:
  • Devaka Cooray
  • Junilu Lacar
  • Tim Cooke
Saloon Keepers:
  • Tim Moores
  • Ron McLeod
  • Tim Holloway
  • Claude Moore
  • Stephan van Hulst
Bartenders:
  • Winston Gutkowski
  • Carey Brown
  • Frits Walraven

Reviews or feedback of basic java programs  RSS feed

 
Greenhorn
Posts: 25
1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Hello everybody!! I'm new here and new into the world of programming too!!
I was just wondering if it's possible/acceptable to post codes of basic java programs and get reviews/feedback from others.
I just started learning java few weeks ago by myself, reading books and with online courses and my goal is get certificated and hopefully get a entry level java job!

Thanks everyone and hope you all having a great day!
 
author & internet detective
Posts: 39147
724
Eclipse IDE Java VI Editor
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Ansamana,
Welcome to CodeRanch!

Yes! We even a forum called "Code Reviews" for that very purpose.
 
Ansamana Sankarray
Greenhorn
Posts: 25
1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Thanks Jeanne!!

I'm afraid that the code it's too long to post it here, there's any other way to post long codes?
 
Marshal
Posts: 63314
205
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
If the code is that long, why are you calling it basic code? You can try a Github or SourceForge or GitLab reference, but many people are reluctant to go onto other websites to download such code.
 
Ansamana Sankarray
Greenhorn
Posts: 25
1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Thanks Campbell!

I see your point, there are 2 classes on the program, i'll try to post it in 2 different posts!!

Class player


 
Ansamana Sankarray
Greenhorn
Posts: 25
1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Main Class

 
Campbell Ritchie
Marshal
Posts: 63314
205
  • Likes 2
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
That isn't long code; it is quite a small class. Asking for feedback means that is what you get, and it might not be nice to read. Read on at your peril!

Line 1: Only use that package name if you own that website. Have a look in the Java™ Tutorials.
Lines 3‑5: The indentation in that comment is inconsistent. It should look like this:-Note the error writing Player in one place and player in another. It would be better for that comment to be a documentation comment: look at the javadoc tool's homepage and links there.
The line‑end comments in the class body are inappropriate; some of them only tell you what you know already. Those in line 15 and 23 are incorrect because you don't have constructors there. I think you may do well to revise what a constructor is. The comments on public methods should be converted to documentation comments.
The comment in line 10 is the first hint about what the game means. If it is anumber‑guessing game, that should be explained somewhere. It also doesn't let me know whether that is a guess made by the player or the “right” answer chosen by this player. The variable name in line 10 is poor; only in London is the word “fing” used, and it doesn't mean finger. If you mean to say finger, then say finger.
The variable numGuess is confusing. It doesn't tell me whether that is the guess made, or how many guesses have been made already. If the variable names were clear, you wouldn't feel the need for those comments to explain them.
Why is the numberOfPlayers variable in the Player class? I don't think it belongs there.
Because you haven't supplied a constructor, you have no way to initialise the object to a consistent state; I shall let you work out what the default values of its fields will be. I think every class should have a constructor, and try not to create multiple constructors if possible.
Why do you have those setXXX methods? Are you going to change a player's name during a game? What will happen if you change the guess or number of fingers? Do you want those methods at all?
Why haven't you written a toString() method?

I haven't got time to look at the other class now, I am afraid.
 
Ansamana Sankarray
Greenhorn
Posts: 25
1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Thanks for your answer Campbell!!

I will have a proper look tomorrow also I'll check the links.

That isn't long code; it is quite a small class. Asking for feedback means that is what you get, and it might not be nice to read. Read on at your peril!



No problem at all, see what I'm doing wrong it's what I'm looking for
 
Bartender
Posts: 5627
55
Eclipse IDE Firefox Browser Java MySQL Database VI Editor Windows
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
You have way too much redundancy in all your print blocks. Make a single print method, passing in enough parameters to effect the visual output (e.g. who wins) as necessary.
Not fond of 'p' vs 'p1'. Would have preferred 'p1' and 'p2'.
 
Carey Brown
Bartender
Posts: 5627
55
Eclipse IDE Firefox Browser Java MySQL Database VI Editor Windows
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
For the case of you playing against the computer: set player 1 name to "YOU", and player 2 name to "COMPUTER". This can reduce the redundancy of your print blocks.
 
Ansamana Sankarray
Greenhorn
Posts: 25
1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Thanks Carey.
I'll try to figure out a way to reduce it.
 
Carey Brown
Bartender
Posts: 5627
55
Eclipse IDE Firefox Browser Java MySQL Database VI Editor Windows
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
NEVER close a Scanner that has been created with System.in. Some IDE's generate a warning if you don't close a Scanner, but in this case don't do it.

Your Player class should include a field for number of wins.
 
Ansamana Sankarray
Greenhorn
Posts: 25
1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Hello! I'm back,

For Campbell:

Line 1: Only use that package name if you own that website. Have a look in the Java™ Tutorials.
Lines 3‑5: The indentation in that comment is inconsistent. It should look like this:-



Thanks for the clarification, I've read(I'm still) the content of the tutorial, I'll save the link.

The line‑end comments in the class body are inappropriate; some of them only tell you what you know already. Those in line 15 and 23 are incorrect because you don't have constructors there. I think you may do well to revise what a constructor is. The comments on public methods should be converted to documentation comments.  



You're right there, I was confused about what is a constructor and what is a method, I remade the code from zero, including a constructor, the toString() method and few other methods, all the variable names have been changed.
I'm skipping the comments as I need to learn more about them.

 
Ansamana Sankarray
Greenhorn
Posts: 25
1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Carey Brown wrote:You have way too much redundancy in all your print blocks. Make a single print method, passing in enough parameters to effect the visual output (e.g. who wins) as necessary.
Not fond of 'p' vs 'p1'. Would have preferred 'p1' and 'p2'.



I have remade that class from scratch too.



I'ts not finished yet as I still have to include the code for the 2 players game.
For the print blocks I tried to reduce the as much as possible, I'm thinking if it would be a good idea to make a new class called "Result" with a generic print block and then modify it as it needs on the class "GuessAndWin" not sure if I'm thinking in the right direction...
About the scanner, yes Eclipse was notifying that the scanner was not closed, I've searched about it but nothing clear whether it should be closed or not. I'll keep that in mind too. Thanks
 
Campbell Ritchie
Marshal
Posts: 63314
205
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Ansamana Sankarray wrote:. . . I remade the code from zero, including . . . the toString() method  . . .

So why are you only returning name from toString()? Shouldn't you include the fingers and the guess?
 
Campbell Ritchie
Marshal
Posts: 63314
205
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Ansamana Sankarray wrote:. . .  I have remade that class from scratch too. . . . .

Kudos for being prepared to take criticism and make changes
But you need more changes. You still have repeated code, and your main method is much too long. Those print statements look like something which should be a separate method. You can consolidate the ten print statements starting in line 11 into one statement with the + operator. And avoid \n. Maybe use printf and %n instead.

Thanks

That's a pleasure.
 
Carey Brown
Bartender
Posts: 5627
55
Eclipse IDE Firefox Browser Java MySQL Database VI Editor Windows
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
For single player
p1 is "YOU"
p2 is "COMPUTER"
For two player
p1 is inputName()
p2 is inputName()

You should be able to come up with a single method for printing. Something like

Still too much redundancy in your printing. As an example, these lines are repeated several tiimes

You can use p1 & p2 .getName() for both 1 and 2 player games if you set the names in the one player game to "YOU" and "COMPUTER". Do away with the specific "computerPlay" then and just use p2.

 
Campbell Ritchie
Marshal
Posts: 63314
205
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Ansamana Sankarray wrote:. . . the scanner was not closed, I've searched about it but nothing clear . . .

You don't need to search; we can tell you whether you need to close a Scanner:-
  • 1: If you are reading from System.in, you mustn't close it.
  • 1½: If you use Eclipse and it says something about resource leaks, and gives you options for sorting out the problem, choose the option with @SuppressWarnings("resource")
  • 2: If your Scanner is reading from anything else, you must close it. The best way to close it is with try with resources.
  •  
    Ansamana Sankarray
    Greenhorn
    Posts: 25
    1
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Hello Campbell and Carey,

    I made some changes, 3 new methods on class GuessAndWin:

    -Main method starts asking for number of players, if is 1 player 1 introduces his name and then sets the name of the player number 2 to "COMPUTER". If it's 2 both players introduce their names.
    -Method "playGame" it's used either to play against the computer or for 2 players, no more repeating the same code.
    -Method "printMoves" prints the result of finger played and fingers guessed for each player. (Trying to find a better name for this method).
    -Method "printResult" prints the result, winner, tie or both lost and the number of wins of each player.

    Again any feedback about things that are not right, things I'm doing wrong, things that make no sense, are welcome!

    Class Player

     
    Ansamana Sankarray
    Greenhorn
    Posts: 25
    1
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Class GuessAndWin

     
    Carey Brown
    Bartender
    Posts: 5627
    55
    Eclipse IDE Firefox Browser Java MySQL Database VI Editor Windows
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    I'm afraid that your printing has taken a turn for the worse. You use a lot of magic numbers. What is message '1' for instance. I think your redundant code in the beginning was clearer. I don't know if you were trying to improve on the output but just taking your original prints and putting them in a method led me towards something more like this. This also expects Player to keep track of the number of wins that it had. Note that 'result' can be local.
     
    Ansamana Sankarray
    Greenhorn
    Posts: 25
    1
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Thanks for your time reviewing the code Carey, really appreciated.
     
    Creator of Enthuware JWS+ V6
    Posts: 3246
    286
    Android Chrome Eclipse IDE
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Congratulations Ansamana Sankarray,

    Your question has made it to our Journal

    Have a Cow!
     
    Greenhorn
    Posts: 10
    Java TypeScript
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Hello,

    Some observations in addition to what others have posted:

    -Reading the description of GuessAndWin confused me. I thought guessing a number and guessing a finger is two completely separate things and didn't understand the connection and why both are needed. Isn't each player just guessing 2 numbers; an own and a sum?
    -All of the methods in GuessAndWin are static at the moment.
    -In main(), lines 139-140, two new Player objects are instantiated. Their names are then handed over to playGame(), but there, on lines 33-34 two new Player objects are instantiated again. Consider either handing over the Player objects themselves to playGame(), or instantiating them only in playGame(). Perhaps there will be a use for setters in Player again.
    -Some of the printed text could be stored in constants. I see lots of "line breaks" ("-----------------------------------------").

    Keep going!
     
    Ansamana Sankarray
    Greenhorn
    Posts: 25
    1
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator

    Paul Folder wrote:Hello,

    Some observations in addition to what others have posted:

    -Reading the description of GuessAndWin confused me. I thought guessing a number and guessing a finger is two completely separate things and didn't understand the connection and why both are needed. Isn't each player just guessing 2 numbers; an own and a sum?
    -All of the methods in GuessAndWin are static at the moment.
    -In main(), lines 139-140, two new Player objects are instantiated. Their names are then handed over to playGame(), but there, on lines 33-34 two new Player objects are instantiated again. Consider either handing over the Player objects themselves to playGame(), or instantiating them only in playGame(). Perhaps there will be a use for setters in Player again.
    -Some of the printed text could be stored in constants. I see lots of "line breaks" ("-----------------------------------------").

    Keep going!



    Hello Paul, thanks for your review!

    Let's see if I can explain myself:

    - Each player is guessing a number from 0 to 10 (the opponent doesn't know your guess) and then play a number of fingers of 1 hand, 0 to 5, the sum of the number of fingers of both players is the result, the player who guessed correctly the result wins.

    - Yes, they are static, to call the methods without an object, checking the code again I guess that line 139 "GuessAndWin.playGame(numberOfPlayers, pName1, pName2);" could be change just to "playGame(numberOfPlayers, pName1, pName2);" same with all the "GuessAndWin.printMessages();
     My overall knowledge of programming and Java still poor, I don't know if I'm doing wrong there...

    - Lines 129, 130, invokes the constructor Player(String name) to  get the names (p1name and p2name), then lines 23, 24, the constructor Player(String name, int fingersPlayed, int numberGuess) the name is get through p1.getName() and p2.getName() . I did it this way so the names are introduced just once,  number fingers played and number guessed several times until the while loop ends. Again, not sure if this is a good practice

    - I will implement the suggestion made by Carey Brown on few posts above, so no more repeated print methods

    Thanks!
     
    Ansamana Sankarray
    Greenhorn
    Posts: 25
    1
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Hello! I just finished a Spring framework tutorial and I decided to remade that game, just to practice
    If anyone could have a look and give any feedback! Would be really appreciated.
    Here's a link to the github repository
    https://github.com/Ansamana/SuperGuess-2.0

    Thanks!
     
    Campbell Ritchie
    Marshal
    Posts: 63314
    205
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Good to see you back Please post the new codde here.
     
    Ansamana Sankarray
    Greenhorn
    Posts: 25
    1
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    I will post some of them here, basically I have 3 modules, 'core' with the game itself, 'console' to play the game from the console, 'web' to do it from the browser, 'web' module is not finished yet.

    From module core:

    Player.java



    ComputerNumberGeneratorImpl.java



     
    Ansamana Sankarray
    Greenhorn
    Posts: 25
    1
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    MessageGeneratorImpl.java



    GameImpl.java

     
    Ansamana Sankarray
    Greenhorn
    Posts: 25
    1
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    From module console

    Main.java



    ConsoleSuperGuess.java

     
    Listen. That's my theme music. That's how I know I'm a super hero. That, and this tiny ad told me:
    Become a Java guru with IntelliJ IDEA
    https://www.jetbrains.com/idea/
    • Post Reply Bookmark Topic Watch Topic
    • New Topic
    Boost this thread!