Simple Hangman game in Java

Question

I'm teaching myself Java and I had some basic questions on how well I am writing my code for readability. I feel like sometimes I'm just writing code that works, and I'm not that proud of myself when I have finished. I'm not asking you to rewrite my code; I'm just asking for some pointers on how optimize my code for readability.

import javax.swing.*;
import java.awt.event.*;
import java.awt.*;

public class Hangman implements ActionListener{
    JFrame frame;
    JTextField userInput;
    JLabel textContents;
    String word = "horse";
        int correctGuesses;
    int incorrectGuesses;
    String guessesLeft;
    boolean lose = false;
    StringBuilder wordsGuessedCorrectly;
    JLabel letters;


    Hangman() {
        incorrectGuesses = 0;
        correctGuesses = 0;
        guessesLeft = "You have " + (6 - incorrectGuesses) +" chances left to guess a " + word.length() + " letter word";
        wordsGuessedCorrectly = new StringBuilder();

        frame = new JFrame("A Hangman Game");
        frame.setSize(300,125);
        frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        frame.setLayout(new GridLayout(3, 1));

        userInput = new JTextField();
        userInput.addActionListener(this);
        frame.add(userInput);

        textContents = new JLabel();
        textContents.setText(guessesLeft);
        textContents.setHorizontalAlignment(JLabel.CENTER);
        textContents.setBorder(BorderFactory.createLineBorder(Color.BLUE));
        frame.add(textContents);

        letters = new JLabel("The letters you guess correctly go down here!");
        letters.setHorizontalAlignment(JLabel.CENTER);
        letters.setBorder(BorderFactory.createLineBorder(Color.RED));
        frame.add(letters);

        frame.setVisible(true);


    }

    String seperateLetter(String a, int x, int y) {
        return a.substring(x, y);
    }



    void testLetter(String a) {
        if (word.contains(a) && wordsGuessedCorrectly.toString().contains(a) == false && correctGuesses != (word.length() - 1)) {
            correctGuesses++;
            wordsGuessedCorrectly.append(a);
            letters.setText(wordsGuessedCorrectly.toString());
            textContents.setText("Correct Guess!");
        }
        else if (word.contains(a) && wordsGuessedCorrectly.toString().contains(a)) {
            textContents.setText("You've already guessed this letter!");
        }
        else if (word.contains(a) && wordsGuessedCorrectly.toString().contains(a) == false && correctGuesses == (word.length() - 1)) {
            lose = true;
            textContents.setText("You Win!");
            wordsGuessedCorrectly.append(a);
            letters.setText(wordsGuessedCorrectly.toString());
        }
        else if (word.contains(a) == false && incorrectGuesses == 5){
            textContents.setText("You lose!");
            lose = true;
        }
        else {
            incorrectGuesses++;
            textContents.setText("Incorrect Guess! You have " + (6 - incorrectGuesses)  + " left.");
        }
    }

    public void actionPerformed(ActionEvent ae) {
        if (lose == false) {
            testLetter(seperateLetter(userInput.getText(), 0 , 1));
            userInput.setText("");
        }
    }

    public static void main(String[] args) {
        SwingUtilities.invokeLater(new Runnable() {
            public void run() {
                new Hangman();
            }
        }); 
    }
}

Answer 1

Just quickly, here's some feedback:

• I think your set up code is fine (where you're creating the JFrames etc), it's very boiler platey, but that's java.

• But you shouldn't put that logic in the constructor. Put it in an init() method, called after construction.

• Do use scope declarations, ie. make that Hangman() constructorpublic Hangman(). Same with void testLetter is this method meant to be public or private? Make it explicit so that others (and yourself, in six months!) know how the code is meant to be used.

• Consider separating your Swing/GUI logic, and your game logic to different classes. ie, something like HangmanApp (your main class) contains a HangmanGui and a HangmanLogic.

• You have magic numbers all through your code. For example this line is particularly bad: textContents.setText("Incorrect Guess! You have " + (6 - incorrectGuesses) + " left."); What if you want to change the number of guesses? Ideally these configurations would be set in a properties file, but for a small application setting them at the top of the class is fine.

• Your if else branching in testLetter is scary. As it currently is, I can't immediately see what each branch is for. Comments would help on each branch, but I would move 'test if the letter fits' logic, and 'what should I do if letter is correct/incorrect' logic, into their own private methods.

eg. something like:

void testLetter(String a) {


     if (evaluateGuess(a)){   //returns true if guess is correct
         handleCorrectGuess(a); 
     }
     else (
         handleIncorrectGuess(a); 
     }

}

• You don't need to use a == comparison on booleans. Use a descriptive variable name and ! operator instead. ie. if (!gameLost){... or if(gameStillInProgress){...

Answer 2

TestLetter should accept a char instead of a String

void testLetter(String a) {

You really want to test if a single character is inside the search word, than a full blown string. Testing if a single character is inside a string can be done with indexOf, and then comparing if its larger or equals to 0

void testLetter(char a) {

Weird variable naming

StringBuilder wordsGuessedCorrectly

This should actually be called charsGuessedCorrectly, because it contains a list of characters the user already used. Even better would be changing the type to a Set<Character> so you can directly use its contains method without turning the whole object to a string.

Don't call setVisible from the constructor

Calling setVisible fro the constructor makes extending your class hard, because any exception thrown from within a sub classes constructor will mean the object is left in a inconsistent state. Better would be removing this line form the constructor and adding it to the main method.

Answer 3

In your test letter method you repeat the majority of the checks you make in your if statement, I think this hurts the readability of your code the most. One example of this is your first if statement and the second else if statement only vary by the last equality check. This equality check could instead occur in an if statement inside if the first check.

Generally, I think it is a good idea to limit the number of conditionals in an if statement. This won't always be true but it is one quick improvement you could make in your code.