4
\$\begingroup\$

It randomly gets a word from an array of Strings. It prints out a '_' char for every letter of the word, displays the ones you have already guessed and displays the ones you have missed.

Some advice would be welcome, such as what I should change, avoid and so on.

Main class:

import java.util.Scanner;
public class Main {
public static void main(String[] args) {

    Game game = new Game();
    Scanner keyboard = new Scanner(System.in);
    String input;
    char guess;

    game.initializeGame();
    do {
        System.out.print("\n-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=\n\nWord: ");
        game.showRow();

        System.out.print("\n\nMisses: ");
        game.showMisses();

        System.out.print("\nGuess: ");
        input = keyboard.next();

        if (input.equalsIgnoreCase("exit"))
            break;
        else if (input.equalsIgnoreCase("new"))
            game.initializeGame();
        else {
            guess = input.charAt(0);
            game.checkRow(guess);

            if (game.isLooser()) {
                System.out
                        .println("\nGame Over!\nYou have missed 8 times!\nThe word was "
                                + game.getSecretWord()
                                + " ! \nThe game will now reset!\n");
                game.initializeGame();
            }

            else if (game.IsWinner()) {
                System.out
                        .println("\nCongratulations!\nYou have guessed the word!\nThe game will now reset!\n");
                game.initializeGame();
            }

        }

    } while (true);

    System.out.println("\nThank you for playing !");
    keyboard.close();
}

}

Game class

import java.util.Random;

public class Game {
Random random = new Random();
StringBuffer misses = new StringBuffer("");

private final String[] wordArray = { "baboons", "beavers", "cats",
        "chickens", "choughs", "dolphins", "eagles", "elephants",
        "flamingoes", "giraffes", "grasshoppers", "hedgehogs", "hornets",
        "kangaroos" };
private String secretWord;
char[] hangman;

public void initializeGame() {
    setWord();
    setRow();
    misses.delete(0, misses.length());
    System.out.println("\nNew game started! Type \"exit\" to stop game or \"new\" to reset !\nOnly the first letter in youre input will be considered a guess.\nYou have 8 guesses !\n");
}

private void setWord() {
    secretWord = wordArray[random.nextInt(wordArray.length)];
}

private void setRow() {
    hangman = new char[secretWord.length()];
    for (int i = 0; i < secretWord.length(); i++) {
        hangman[i] = '_';
    }
}

public void showRow() {
    for (int i = 0; i < hangman.length; i++)
        System.out.print(hangman[i] + " ");
}

public void showMisses() {
    System.out.println(misses);
}

private boolean isCharInWord(char guess) {
    for (int i = 0; i < hangman.length; i++) {
        if (secretWord.charAt(i) == guess)
            return true;
    }
    return false;
}

private void changeRow(char guess) {
    for (int i = 0; i < hangman.length; i++)
        if (secretWord.charAt(i) == guess)
            hangman[i] = secretWord.charAt(i);
}

private void addToMisses(char guess) {
    misses.append(guess + " ");
}

public void checkRow(char guess) {
    if (isCharInWord(guess)) {
        changeRow(guess);
    } else
        addToMisses(guess);

}

public boolean isLooser() {
    if (misses.length() / 2 > 7)
        return true;
    return false;
}

public boolean IsWinner() {
    for (int i = 0; i < hangman.length; i++)
        if (hangman[i] == '_')
            return false;
    return true;
}

public String getSecretWord() {
    return secretWord;
}

}
\$\endgroup\$

2 Answers 2

4
\$\begingroup\$

Remember that any function of the type

def function(arg):
    if condition(arg):
        return True
    return False

can be shortened to:

def function(arg):
    return condition(arg)

You can apply this advice to:

public boolean isLooser() {
    if (misses.length() / 2 > 7)
        return true;
    return false;
}

Also I think that you can simplify the condition to:

misses.length() > 14

The function:

public boolean IsWinner() {
    for (int i = 0; i < hangman.length; i++)
        if (hangman[i] == '_')
            return false;
    return true;
}

Can benefit from built-ins:

public boolean IsWinner() {
    return hangman.indexOf('_') == -1; // '_' never occurs
}

As well as this one:

private boolean isCharInWord(char guess) {
    return secretWord.indexOf(guess) != -1;
}

Brace yourself

Please always use braces, doing so increases readability and reduces stupid-bugs probability.

A bit of generality

setWord is specific to this game, I would do:

public void randomChoice(array) {
    return array[random.nextInt(array.length)];
}

and then in the initialization I would call:

public void initializeGame() {
   secretWord =  randomChoice(wordArray);
   // ...
}

I think my version is more immediate to understand. (By the way, I can see that it is an array, so you can remove that from the name, a simple words would be better).

\$\endgroup\$
7
  • 1
    \$\begingroup\$ You should always use the simplification that you mention with misses.length() / 2 > 7 there, as division is extremely taxing on the processor. Also you missed a semicolon on this line: return hangman.indexOf('_') == -1. \$\endgroup\$ Jul 9, 2015 at 13:57
  • \$\begingroup\$ @EBrown Nice joke about the division :). One division takes a millionth of a second on a PC, my simplification was only for readibility. \$\endgroup\$
    – Caridorc
    Jul 9, 2015 at 13:59
  • 4
    \$\begingroup\$ It's not a joke. Division is much more taxing on the processor than multiplication of an inverse. (I'm basing this off of an AMD instruction set document I have, which states that a 64-bit division operation has a latency of 47, whereas a 64-bit multiplication operation has a latency of 6.) \$\endgroup\$ Jul 9, 2015 at 14:01
  • \$\begingroup\$ @EBrown It can be as taxing as you want, let's suppose a division takes a 1/1000-th of a second (huge time, but let's just suppose) in a program with a user interface, the user won't notice that time-out. You are rigth about low level number-crunching thought. \$\endgroup\$
    – Caridorc
    Jul 9, 2015 at 14:04
  • 1
    \$\begingroup\$ @EBrown excellent that we agree, but it should be clear that writing more complex code to save 0.0001 seconds in a user-interface-driven program is crazy. \$\endgroup\$
    – Caridorc
    Jul 9, 2015 at 14:07
3
\$\begingroup\$
if (misses.length() / 2 > 7) return true;

System.out.println("\nGame Over!\nYou have missed 8 times!...");

The numbers 7 and 8 are not only hard-coded, but they have an important relationship as well. If you decided to change your 7 to 6, you'll need to remember to change your output above too to say You have missed 7 times!. It is better if your Game class has a static final field to mark this:

public class Game {
    private static final int MAX_TRIES = 7;
}

// usage
return misses.length() / 2 > MAX_TRIES; // after refactoring

System.out.println("\nGame Over!\nYou have missed " + (MAX_TRIES + 1) + "times!...");

Your IsWinner() and isLooser() method names can be improved, since a game is technically neither a winner nor loser, but won or lost. Therefore isWon() or isLost() may be better names here.

You also have inconsistent usage of { } braces for your if statements, I will suggest using them throughout. This is mainly because it eliminates the possibility of introducing bugs when you have introduced a second line seemingly within an existing if block, but is actually not.

The indentation of your methods are also weirdly left-aligned, and I'm not sure if this is a side-effect of using the Markdown formatting. Please check the indentation of your code too.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.