patternjavaMinor
Hanging men efficiently
Viewed 0 times
efficientlymenhanging
Problem
I can't seem to get the formatting right, so here is the pastebin if needed:
http://pastebin.com/g6VmdYbM
```
public class Hangman {
///////////////////////////////////
// Global Variables
///////////////////////////////////
private static String[] correctPhrase = new String[5];
private static String[] currentGuessPhrase = new String[5];
private static char[] currentGuesses = new char[26];
private static int totalWrong = 0;
private static int totalGuesses = 0;
private static int count;
private static char guess;
///////////////////////////////////
// Methods
///////////////////////////////////
public static void makePhrase() {
//Clear guesses
for (int x = 0; x < currentGuesses.length; x++){
currentGuesses[x] = ' ';
}
totalWrong = 0;
totalGuesses = 0;
//Preset Phrases (Must be 5 words)
String[] phraseOne = { "this", "is", "a", "sample", "phrase" };
String[] phraseTwo = { "another", "phrase", "is", "right", "here" };
String[] phraseThree = { "finally", "this", "is", "the", "last" };
//Random words for selection
String[] wordBank = { "a", "ate", "apple", "banana", "bored", "bear",
"cat", "cow", "carried", "died", "during", "deer", "elephant",
"flame", "fire", "fruit", "forgave", "forged", "fears", "goat",
"good", "game", "gave", "greeted", "glory", "ham", "hairy",
"heaven", "horrible", "I", "illegal", "important", "jammed",
"juice", "kangaroo", "liar", "loved", "money", "miracles",
"monday", "named", "never", "noun", "now", "nor", "orange",
"obligated", "person", "people", "peeled", "quit", "retired",
"rain", "saved", "sunny", "soaring", "salmon", "sealed",
"today", "tomorrow", "trained", "the", "umbrella", "up",
"under", "violent", "violin", "when", "while", "year", "zoo" };
//Get phrase type
Scanner in = new Scanner(System.in);
System.out.println("\n(1) Random Words (2) Presets (3)
http://pastebin.com/g6VmdYbM
```
public class Hangman {
///////////////////////////////////
// Global Variables
///////////////////////////////////
private static String[] correctPhrase = new String[5];
private static String[] currentGuessPhrase = new String[5];
private static char[] currentGuesses = new char[26];
private static int totalWrong = 0;
private static int totalGuesses = 0;
private static int count;
private static char guess;
///////////////////////////////////
// Methods
///////////////////////////////////
public static void makePhrase() {
//Clear guesses
for (int x = 0; x < currentGuesses.length; x++){
currentGuesses[x] = ' ';
}
totalWrong = 0;
totalGuesses = 0;
//Preset Phrases (Must be 5 words)
String[] phraseOne = { "this", "is", "a", "sample", "phrase" };
String[] phraseTwo = { "another", "phrase", "is", "right", "here" };
String[] phraseThree = { "finally", "this", "is", "the", "last" };
//Random words for selection
String[] wordBank = { "a", "ate", "apple", "banana", "bored", "bear",
"cat", "cow", "carried", "died", "during", "deer", "elephant",
"flame", "fire", "fruit", "forgave", "forged", "fears", "goat",
"good", "game", "gave", "greeted", "glory", "ham", "hairy",
"heaven", "horrible", "I", "illegal", "important", "jammed",
"juice", "kangaroo", "liar", "loved", "money", "miracles",
"monday", "named", "never", "noun", "now", "nor", "orange",
"obligated", "person", "people", "peeled", "quit", "retired",
"rain", "saved", "sunny", "soaring", "salmon", "sealed",
"today", "tomorrow", "trained", "the", "umbrella", "up",
"under", "violent", "violin", "when", "while", "year", "zoo" };
//Get phrase type
Scanner in = new Scanner(System.in);
System.out.println("\n(1) Random Words (2) Presets (3)
Solution
So, going through your code I am struck by a few things....
Suggestion, merge the logic of checkGuess()
- static variables are seldom the 'right' way to do things in Object Oriented languages like Java. Other than for constants, it is a red flag that something is wrong.
- you have a problem with generating random numbers... e.g.
Math.round(Math.random() * 2)will, in fact, generate values of 0, 1, and 2, but it will generate the value '1' twice as many times as it generates the values 0 or 2. The 'right way' for doing random numbers is here on StackOverflow. In this case, you want to create aRandom randgen = new Random()instance, and use thenextInt()instance method to get random numbers:randgen.nextInt(range)or, in your 0,1,2 example, you wantrandgen.nextInt(3);
- The code
correctPhrase[x] = wordBank[(int) Math.round(Math.random() * 61)];is ugly. The61is a 'magic number' and would be best to replace withwordBank.length. The normal mechanism for this line of code is:correctPhrase[x] = wordBank[randgen.nextInt(wordBank.length)];(using the same randgen instance we created above.
- you do not convert the 'custom' input phrase to lowercase, which could result in issues, and you should validate that it only has 5 words... otherwise in the next piece of code you could gt an indexout-of-bounds exception when creating the underscore version.
getGuess()appears to be fine.
- checkGuess() is ugly because of the static variables again, and, in fact, the code is essentially a subset of the updateGuess() method.
checkGuess() can be completely removed.... I will describe how, in a moment.
- the drawBoard()
method has lots of code and constant repetition. I would find a way to put a 'blank' board in an array of values (one per line), and then copy that array for each 'totalWrong, but adjust the lines where needed. Then, you can just loop through the lines that are relevant for your 'totalWrong' state (i.e. remove most of the code duplication).
- the goAgain()
method suddenly has GUI components... odd...
- the main()
method appears to be straight-forward enough, but would change a lot if you do what I suggest below:
Suggestion, merge the logic of checkGuess()
in to updateGuess():
public static boolean guess(char guess) {
// Define char array from currentGuess for alerting
char[][] currentGuessArray = new char[currentGuessPhrase.length][];
for (int x = 0; x < currentGuessPhrase.length; x++) {
currentGuessArray[x] = currentGuessPhrase[x].toCharArray();
}
boolean goodguess = false;
//Assign valid values of guess to currentGuessArray
for (int x = 0; x < correctPhrase.length; x++) {
for (int a = 0; a < correctPhrase[x].length(); a++) {
if (correctPhrase[x].charAt(a) == guess) {
currentGuessArray[x][a] = guess;
goodguess = true;
}
}
}
// Convert chars back to string array
for (int x = 0; x < currentGuessArray.length; x++) {
currentGuessPhrase[x] = new String(currentGuessArray[x]);
}
return goodguess;
}
You can then adapt the main method to:
if (!guess(guess)) {
totalWrong++;
}
EDIT
Why static fields are ugly .... (and yes, I probably should have been more descriptive about this, but it is a big topic to answer)
Static fields (not final/constant) are used to store program state. This is a bad pattern in OOP because it means your state is not encapsulated. Consider, for the moment, that a 'better' way to solve your problem would be:
public static final void main (String[] args) {
do {
String phrase = getPhrase();
Hangman game = new HangMan(phrase);
game.play();
} while (goAgain());
}
Then, you have a Hangman` class that encapsulates the state for that instance of the game only:public class Hangman {
private final String[] correctPhrase = new String[5];
private final String[] currentGuessPhrase = new String[5];
private final char[] currentGuesses = new char[26];
private int totalWrong = 0;
private int totalGuesses = 0;
... and lots of code to make the game work....
}Code Snippets
public static boolean guess(char guess) {
// Define char array from currentGuess for alerting
char[][] currentGuessArray = new char[currentGuessPhrase.length][];
for (int x = 0; x < currentGuessPhrase.length; x++) {
currentGuessArray[x] = currentGuessPhrase[x].toCharArray();
}
boolean goodguess = false;
//Assign valid values of guess to currentGuessArray
for (int x = 0; x < correctPhrase.length; x++) {
for (int a = 0; a < correctPhrase[x].length(); a++) {
if (correctPhrase[x].charAt(a) == guess) {
currentGuessArray[x][a] = guess;
goodguess = true;
}
}
}
// Convert chars back to string array
for (int x = 0; x < currentGuessArray.length; x++) {
currentGuessPhrase[x] = new String(currentGuessArray[x]);
}
return goodguess;
}if (!guess(guess)) {
totalWrong++;
}public static final void main (String[] args) {
do {
String phrase = getPhrase();
Hangman game = new HangMan(phrase);
game.play();
} while (goAgain());
}public class Hangman {
private final String[] correctPhrase = new String[5];
private final String[] currentGuessPhrase = new String[5];
private final char[] currentGuesses = new char[26];
private int totalWrong = 0;
private int totalGuesses = 0;
... and lots of code to make the game work....
}Context
StackExchange Code Review Q#36084, answer score: 6
Revisions (0)
No revisions yet.