Hangman program in Java

Here's an implementation of Hangman using Java 6. I've split the code into 2 classes - a logic class & a gui class. Is it ok to have so many static member variables? Please let me know if there are any design improvements that I could make.

Hangman logic - main class

```
import java.awt.*;
import java.io.*;
import java.util.*;
import java.util.List;
import javax.swing.*;

public class Hangman {
static String[] wordList;
static String secretWord;
static Set alphabet;
static Set lettersGuessed; // letters the user has guessed
static boolean[] lettersRevealed; // determines if the letter should be revealed or not
static int guessesRemaining;

public static void main(String[] args){
Hangman hangman = new Hangman();
hangman.createAlphabetSet();
hangman.readFile("words.txt");

HangmanGUI.buildGUI();
setUpGame();
}

// checkIfWon - sees if the user has won the game
static boolean checkIfWon(){
for(boolean isLetterShown : lettersRevealed){
if(!isLetterShown)
return false;
}
return true;
}

// checkUserGuess - get input from the user
static boolean checkUserGuess(String l){
if(l.length() == 1 && !lettersGuessed.contains(l.charAt(0)) && alphabet.contains(l.charAt(0))) {
HangmanGUI.setText(null);
lettersGuessed.add(l.charAt(0));
return true;
}else{
Toolkit.getDefaultToolkit().beep();
}
return false;
}

// chooseSecretWord - selects a word
private static String chooseSecretWord(String[] wordList){
return wordList[(int)(Math.random() * wordList.length)];
}

// createAlphabetSet - Creates the alphabet set that's used to ensure that the user's guess not a nu

Is it ok to have so many static member variables?

No, it's almost always a sign of bad design (and not just because of the static variables, but also the static functions; only utility functions should be static). Another bad sign is that you import javax.swing.* in your logic class.

You classes also do too many things, which makes them very static, hard to read, and hard to write automated tests for.

I would at least create:

• aHangmanGame which contains the word that currently has to be guessed, the guessed letters, the revealed letters, and the remaining guesses. Maybe the alphabet set as well. But it doesn't deal with any user input or the like, it just statically stores the game data and handles the logic. The constructor would only take the word to be guessed, and maybe remaining guesses. Methods might be public boolean guess(Character), public boolean isGameOver(), public boolean isGameWon, String[] getWrongGuessesMade() (or create a Guess class, which then can have a field wrong, and return a list of those classes), etc. All these methods should not be static.

• HangmanMain: the main game loop. Build gui, game, etc; manage play again, etc.

• WordReader: gets the words

• HangmanGUI: pretty much as before, but don't let it be responsible for getting the data it needs. drawGuessesRemaining() for example could look like this: drawGuessesRemaining(int guesses) and then used like this in HangmanMain: // init gui somewhere at the beginning: HangmanGUI hangmanGUI = new HangmanGUI(); // somewhere else: hangmanGUI.drawGuessesRemaining(5);

• I would move the GuessListener to its own class.

Misc

• don't import *, but import all classes explicitly, so a reader knows exactly what you use.

• use private (or public if it makes sense) instead of the default package private.

• use curly brackets even for one line statements

• use JavaDoc style comments for more readable method documentation. Also, some comments could be improved (eg chooseSecretWord - selects a word: a word for what? What happens with the selected word?

• don't hardcode magic numbers such as 5, because it makes it hard to see what the 5 stands for, and it makes it hard to reconfigure the game. Move these to static fields. This is especially bad for lettersGuessed = new HashSet(26), because it is independent of the 26 used in the createAlphabetSet function. So theoretically, I could reconfigure the game to generate a bigger alphabet set (because I want to include ß and ä), raise the guessesRemaining to 28 (because who likes losing?), and then get an exception (a bit of a hypothetical, but I think the idea is clear).

• You don't have to declare the type twice when declaring something. List changeBool = new ArrayList<>() works fine.

• some of your variable names are rather short (s, el, idx, ans, etc). This is only in a small context, so it's not that bad, but I would prefer more expressive names

• your spacing is sometimes off

StackExchange Code Review Q#82440, answer score: 5