Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Last week I asked to review my "Guess a number" game in Java here

I learned so much that I decided to try a little harder game and put into practice all the amazing feedback I received there. Would you please review and give me some feedback?

My main concern is if i'm doing ok using so many methods instead of a big block of code in the main function. I've found it was nice to create several methods in the previous project, but here it was difficult. Perhaps I'm having a serious design issue?

If the game looks fine, what would be the next simple game challenge I should attempt? Is it a good idea to try to port this to swing? Or would be better to port it to a web application? or android? I want to keep learning while challenging myself to something new. I know I need to learn how to handle exceptions better or how to cast / downcast into subclasses (where can I learn this or how can I practice this in a project? a card game?)

I'm new to GitHub, but I created my second repository today to upload this code, perhaps its easier to look at the code here

This is the main class:

package game;

import java.util.HashSet;
import java.util.Scanner;
import java.util.Set;

public class HangmanGame {
    private Scanner input;
    private String playerName;
    private String wordToGuess;
    private boolean gameIsRunning;
    private int lives;
    private char letter;
    private Set<Character> lettersToGuess;
    private Set<Character> lettersGuessed;
    private Set<Character> lettersWrong;

    public static void main(String[] args) {
        HangmanGame game = new HangmanGame();
        game.run();
    }

    private void run() {
        input = new Scanner(System.in);
        playerName = getPlayerName(input);
        wordToGuess = getRandomWordToGuess();
        gameIsRunning = true;
        lives = 5;
        lettersToGuess = new HashSet<Character>();
        lettersGuessed = new HashSet<Character>();
        lettersWrong = new HashSet<Character>();

        fillLettersToGuess();
        // Put first and last letter into lettersGuessed set.
        lettersGuessed.add(wordToGuess.charAt(0));
        lettersGuessed.add(wordToGuess.charAt(wordToGuess.length() - 1));

        System.out.println("I've picked a word for you to guess.");
        while (gameIsRunning) {
            showHangmanState();
            showWordToGuess(wordToGuess, lettersGuessed);
            System.out.println(playerName + ", guess a letter!");
            showWrongLetters();
            pickLetter();
                if (isLetterInWord(letter, lettersToGuess)) {
                    addLetterToLettersGuessed(letter, lettersGuessed);
            } else {
                wrongLetter(letter, lettersWrong);
                lifeLost();
                System.out.println("Your letter is not in the word.");
                System.out.println("Lives remaining: " + lives);
            }
            showWordToGuess(wordToGuess, lettersGuessed);
            if (isGameOver()) {
                gameIsRunning = false;
                System.out.println("You " + ((lives == 0) ? "lost!" : "won!"));
            }
        }
        input.close();
    }

    /*
     * Method to ask for the player's name
     */
    private String getPlayerName(Scanner keyboard) {
        System.out.println("Hi there! What's your name?");
        String playerName = keyboard.nextLine().trim();
        System.out.println("Hi " + playerName + ", let's play!");
        return playerName;
    }

    /*
     * Returns a random word to guess and play.
     */
    private String getRandomWordToGuess() {
        // Load a word from dictionary. To do: Implement a dictionary
        String[] wordList = { "hello", "test", "elephant", "car", "table",
                "stack", "help", "someone", "yellow", "purple" };
        int min = 0;
        int max = wordList.length - 1;
        int wordToGuessPosition = getRandomNumber(min, max);
        return wordList[wordToGuessPosition];
    }

    /*
     * Returns a random number. To do: Move it to a helper class.
     */
    private int getRandomNumber(int min, int max) {
        return min + (int) (Math.random() * ((max - min) + 1));
    }

    /*
     * Shows the word to guess.
     */
    private void showWordToGuess(String wordToGuess,
            Set<Character> lettersGuessed) {

        for (int i = 0; i < wordToGuess.length(); i++) {

            // Checks lettersGuessed to know what to reveal to the user
            if (lettersGuessed.contains(wordToGuess.charAt(i))) {
                System.out.print(wordToGuess.charAt(i));
            } else {
                System.out.print("*");
            }
        }
        // Hardcoded newline. Is there any fancier way to do this?
        System.out.println();
    }

    /*
     * Checks if the letter guessed is in the lettersToGuess.
     */
    private boolean isLetterInWord(char letter, Set<Character> lettersToGuess) {
        return lettersToGuess.contains(letter);
    }

    /*
     * Adds the guessed letter to the set that contains the guessed letters.
     */
    private void addLetterToLettersGuessed(char letter,
            Set<Character> lettersGuessed) {
        lettersGuessed.add(letter);
    }

    /*
     * Adds the wrong letter to the set that contains wrong letters.
     */
    private void wrongLetter(char letter, Set<Character> lettersWrong) {
        lettersWrong.add(letter);
    }

    /*
     * We lose 1 life per wrong letter. We will use this method to handle the
     * draw to display.
     */
    private void lifeLost() {
        lives--;
    }

    /*
     * Fills lettersToGuess from wordToGuess
     */
    private void fillLettersToGuess() {
        for (Character c : wordToGuess.toCharArray()) {
            lettersToGuess.add(c);
        }
    }

    /*
     * Show Hangman state. To do: draw hangman in his 5 stages.
     */
    private void showHangmanState() {
        System.out.println("+++++++++++++++++++++++++++++++++");
        System.out.println("+++++Hangman draw goes here.+++++");
        System.out.println("+++++Hangman draw goes here.+++++");
        System.out.println("+++++Hangman draw goes here.+++++");
        System.out.println("+++++Hangman draw goes here.+++++");
        System.out.println("+++++++++++++++++++++++++++++++++");
        System.out.println("Word to guess:");
    }

    /*
     * We check if the game is over (win or lose)
     */
    private boolean isGameOver() {
        return ((lives == 0) || (lettersGuessed.size() == lettersToGuess.size()));
    }

    /*
     * Show wrong letters (if any)
     */
    private void showWrongLetters() {
        if (lettersWrong.size() > 0) {
            System.out.print("Wrong letters: ");
            for (Character c : lettersWrong) {
                System.out.print(c + " ");
            }
            System.out.println();
        }
    }

    /*
     * Ask the user to type a letter and verify if it isn't in the game already
     */
    private void pickLetter() {
        do {
            letter = input.nextLine().trim().toLowerCase().charAt(0);
            if (lettersWrong.contains(letter)
                    || lettersGuessed.contains(letter)) {
                System.out.println("The letter " + letter
                        + " is already in the game!.");
                System.out.println(playerName + ", guess a new letter!");
            }
        } while (lettersWrong.contains(letter)
                || lettersGuessed.contains(letter));
    }
}
share|improve this question

2 Answers 2

up vote 8 down vote accepted

Nice improvement from the initial version. Good job! Here are a couple of brief notes.

  1. I'm very wary of having any method named run() outside of classes which implement the Runnable interface. This isn't something you'd know about as a beginner, but basically run() has a very specific connotation which regards using multiple threads. I might rename this method to play() or something of the sort.
  2. It looks like you have a lot of functionality tacked on that is just needless overhead. For example, you have a method addLetterToLettersGuessed() which only has one line of code: lettersGuessed.add(letter). There's no need for you to have a wrapper method here. The name of your variable and the method you invoke from it state what you're doing very succinctly. Because you've chosen your variable names well, the code is somewhat self-documenting. Over-architecting your solution can be as big of a problem as having all of your code just slapped in the main() method. (This comment applies to a chunk of your methods below showWordToGuess()).
  3. If you're going to have big block comments over every method, consider formatting them with JavaDoc. It's never too early to learn how to properly document your code.

To address your specific questions:

Is it a good idea to try to port this to swing? Or would be better to port it to a web application? or android?

You should definitely go with Swing before trying to integrate Java code into a web-app. If you're interested in making Android apps, though, you can download the Android SDK and start toying around with that API. I'm sure there are tutorials out there for that. Setting up your environment might be a pain, though (your programs have to run in an emulator or be deployed to an actual phone).

I know I need to learn how to handle exceptions better or how to cast / downcast into subclasses (where can I learn this or how can I practice this in a project? a card game?)

Casting is actually a kind of "all else failed" tactic in Java programming. Ideally, you should never have to explicitly cast anything. I think you're referring to the concept of polymorphism rather than just casting. A good way to learn about polymorphism is with some game or project involving different kinds of animals. You would have an abstract class or interface named Animal, and below this you might have mammals and birds and all sorts of things. Turns out the world of biology is naturally suited towards hierarchy.

Exceptions you learn naturally when you start actually trying to deal with errors rather than allowing your application to crash when it doesn't go down the "happy path". I think most beginners first experience this when they're trying to get a user to input integers and they have to deal with a user maliciously typing in sdfksdjf. As you try to make your applications more robust, Exception-handling skills will develop.


For your next iteration...

I challenge you to do some file handling! Download this file to your workspace and figure out how to read the words into your program and have it "think up" a random word from it. :)

share|improve this answer
    
Thank you! I improved the game and removed almost 50 lines of code, already updated it in GitHub. I've been trying to do your challenge, but I'm having issues to "think up" a VALID word. Any suggestion? –  facundop Apr 18 at 18:51
    
@facundop What do you mean? All the words on that list should be valid Hangman words. –  asteri Apr 18 at 19:39
    
I thought you asked me to think up a new word! I believe I've done it right, should I open a new thread here? The code is here ! –  facundop Apr 19 at 16:14
    
@facundop Post it in a new question. :) –  asteri Apr 19 at 21:24

Just a nitpick on your random word choice routine, which is:

/*
 * Returns a random word to guess and play.
 */
private String getRandomWordToGuess() {
    // Load a word from dictionary. To do: Implement a dictionary
    String[] wordList = { "hello", "test", "elephant", "car", "table",
            "stack", "help", "someone", "yellow", "purple" };
    int min = 0;
    int max = wordList.length - 1;
    int wordToGuessPosition = getRandomNumber(min, max);
    return wordList[wordToGuessPosition];
}

/*
 * Returns a random number. To do: Move it to a helper class.
 */
private int getRandomNumber(int min, int max) {
    return min + (int) (Math.random() * ((max - min) + 1));
}

Subtracting 1 to obtain max, then adding 1 in getRandomNumber() is awkward. I suggest that you embrace the concept of inclusive-exclusive ranges. Consider:

With that in mind, I'd prefer:

private int getRandomNumber(int lbIncl, int ubExcl) {
    return lbIncl + (int)(Math.random() * (ubExcl - lbIncl));
}

But better still, I'd decompose the problem differently. Random.nextInt() is a better way to accomplish getRandomNumber(). Your real goal is to uniformly sample an array. Furthermore, as you noted in your to-do comment, you want to be able to use a proper word list. By the Single-Responsibility Principle, producing the word list and picking a word from the list should be separate functions. Therefore, I suggest a function:

/**
 * Uniformly sample one element from the pool.
 */
private static <T> T randomElement(T[] pool) {
    int i = (new Random()).nextInt(pool.length);
    return pool[i];
}
share|improve this answer
    
Thank you! I had no idea of anything you said, but after some research it makes sense. Thank you for the tips! –  facundop Apr 18 at 18:28

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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