• 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:
  • Campbell Ritchie
  • Ron McLeod
  • Paul Clapham
  • Jeanne Boyarsky
  • Liutauras Vilda
Sheriffs:
  • Rob Spoor
  • Bear Bibeault
  • Tim Cooke
Saloon Keepers:
  • Tim Moores
  • Stephan van Hulst
  • Tim Holloway
  • Carey Brown
  • Piet Souris
Bartenders:
  • Frits Walraven
  • Himai Minh

Is this "guess the word program" (hangman) correct ?

 
Greenhorn
Posts: 1
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
I've written this guess the word game program. It seems to work fine, but I feel there's something wrong about it. Could you please review the code and see if there's something wrong with or are there any
optimizations I can make to it. Thank you in advance.

 
Saloon Keeper
Posts: 8220
71
Eclipse IDE Firefox Browser MySQL Database VI Editor Java Windows
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Math.random() has been replaced by the Random class which has a cleaner interface.


Don't put everything in main().
 
Rancher
Posts: 4307
38
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Continually creating a new String from guessedLetters looks wrong.  Create it only as needed.
 
Carey Brown
Saloon Keeper
Posts: 8220
71
Eclipse IDE Firefox Browser MySQL Database VI Editor Java Windows
  • Likes 1
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Some minor suggestions.
Make things that don't change into constants, e.g. INPUT, RAND, WORDS.
You could use StringBuilder instead of a char[]. Builder is very similar to a String but is muttable so you can add and delete directly.
Make the loop that checks to see if you want to play again separate from the actual play loop.
Make getRandomWord() method which allows future enhancement to get words from file for example.

 
Marshal
Posts: 72913
330
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
What's the point of indexOf(...) > −1 when you can write contains(...)? The inequality might seem clever, but it is potentially more error‑prone than a simple method call.
 
Sheriff
Posts: 16201
270
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
This would be a nice little exercise in refactoring and adding unit tests. I'll post something in the Refactoring forum if you'd like to follow along.

Here's the refactoring thread: https://coderanch.com/t/743010/engineering/Refactoring-main -- everyone is welcome to follow along and contribute ideas.
 
Junilu Lacar
Sheriff
Posts: 16201
270
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
One thing I've discovered while putting your program under test is that it accepts '*' as input and incorrectly reports it as "already in the word."

Here's the test that confirmed that behavior:

Here's the result of running the test (failure)

> Task :test FAILED
...
expected: 0
but was : 2
 
Junilu Lacar
Sheriff
Posts: 16201
270
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

Carey Brown wrote:Some minor suggestions.
Make things that don't change into constants, e.g. INPUT, RAND, WORDS.
You could use StringBuilder instead of a char[]. Builder is very similar to a String but is muttable so you can add and delete directly.
Make the loop that checks to see if you want to play again separate from the actual play loop.
Make getRandomWord() method which allows future enhancement to get words from file for example.


It's interesting how similar yet different some of these choices are to the ones I've made in my refactoring attempt. The extraction to getRandomWord() is one thing that I did also but for an entirely different reason. In my case, the motivation was to create a seam that I could use to make the code more testable.

Another thing I noticed is around the Scanner object being used as the means of input. In my refactoring, I didn't extract it to a constant but rather, I extracted it to another method that I could also use as a seam to bring the means of input under control of the test. This couldn't be achieved by the change that you made to it by extracting to a constant and initializing it on class loading.

I did find that I eventually wanted to have the main() method look like what you have in order to separate the "play again?" logic from the "play once" logic. See my progress in the thread in the Refactoring forum if you want more details and understand the rationale behind the changes I made while refactoring this code. My first main objective was to clarify that big noisy section of code that makes up the bulk of the logic of the "play once" part of the program.
 
lowercase baba
Posts: 12986
66
Chrome Java Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
the best way to tell if your program is correct is to compare it against your specifications for what it is supposed to do.  We don't have those specs, so we can't actually tell you if it is correct or not.

and there are almost always optimizations you can make, but a better questions is are there any optimizations you should make.  And the answer to that again goes back to your specs, and what they say about efficiency/speed.  it's almost always better to write clean, understandable, maintainable code that super-efficient code.
 
Junilu Lacar
Sheriff
Posts: 16201
270
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

fred rosenberger wrote:We don't have those specs, so we can't actually tell you if it is correct or not.


True but we can make some pretty good educated guesses based on how the code is written. Ironically, some of those clues come from the comments, which are normally a code smell that indicates a lack of clarity and expressiveness in the code itself.

For example, this code:

The comment and the message being displayed would lead me to guess that the intended behavior is not in line with the actual behavior. It seems that if the word has multiple instances of the same letter, *all* of the instances are expected to be revealed. However, as it was written, the code only reveals one instance of the letter for each guess even if there are multiple instances of that letter in the word. In my testing of the code, I specified "foo" as the word and guessing 'o' will only revealed the first one. I had to guess 'o' a second time to reveal the second 'o'. That behavior is verified by this failing test:

This fails both assertions:

Multiple Failures (2 failures)
org.opentest4j.AssertionFailedError:
expected: 1
but was : 3
org.opentest4j.AssertionFailedError:
expected: 2
but was : 0

This failure shows that the program as written will not display multiple instances of the same letter the first time it is guess but will in fact require the player to guess the same letter as many times as there are instances of it in the word.
reply
    Bookmark Topic Watch Topic
  • New Topic