Hangman program in Java

Question

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<Character> alphabet;
 static Set<Character> 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 number nor a special character
 private void createAlphabetSet(){
 alphabet = new HashSet<Character>(26);
 for(Character c = 'a'; c<='z'; c++){
 alphabet.add(c);
 }
 }
 // loseSequence - when the the user runs out of guesses
 static void loseSequence(){
 for(int i = 0; i < lettersRevealed.length; i++){
 lettersRevealed[i] = true;
 }
 HangmanGUI.drawSecretWord();
 playAgain("Tough luck. The secret word was " + secretWord + ".\nWould you like to play another game of hangman?");
 }
 // playAgain - Allows the user to play another game of hangman
 private static void playAgain(String message){
 int ans = HangmanGUI.playAgainDialog(message);
 if(ans == JOptionPane.YES_OPTION){
 setUpGame();
 }else{
 System.exit(0);
 }
 }
 // readFile - read in wordList
 private String[] readFile(String loc){
 BufferedReader input = null;
 try{
 input = new BufferedReader(new InputStreamReader(this.getClass().getClassLoader().getResourceAsStream(loc)));
 wordList = input.readLine().split(" ");
 }catch(IOException ioException) {
 ioException.printStackTrace();
 }finally{
 try {
 if (input != null) input.close();
 }catch(IOException ioEx){
 ioEx.printStackTrace();
 }
 }
 return wordList;
 }
 // setUpGame - sets up the variables appropriately
 private static void setUpGame(){
 guessesRemaining = 5;
 secretWord = chooseSecretWord(wordList);
 lettersRevealed = new boolean[secretWord.length()];
 Arrays.fill(lettersRevealed, false);
 lettersGuessed = new HashSet<Character>(26); // 26 letters in alphabet
 HangmanGUI.drawSecretWord();
 HangmanGUI.drawLettersGuessed();
 HangmanGUI.drawGuessesRemaining();
 }
 // updateSecretWord - updates which letters of the secret word have been revealed
 static void updateSecretWord(String l){
 List<Integer> changeBool = new ArrayList<Integer>();
 if(secretWord.contains(l)){
 // Searches through secretWord & notes down all letters that equal the user's guess
 for(int i=0; i<secretWord.length(); i++){
 if(secretWord.charAt(i) == l.charAt(0))
 changeBool.add(i);
 }
 // Changes the boolean value for those letters @ their corresponding indexes
 for(Integer idx : changeBool)
 lettersRevealed[idx] = true;
 }else{
 guessesRemaining--;
 HangmanGUI.drawGuessesRemaining();
 }
 }
 // winSequence - when the user has correctly guessed the secret word
 static void winSequence(){
 playAgain("Well done! You guessed " + secretWord + " with " + guessesRemaining + " guesses left!\n" +
 "Would you like to play another game of hangman?");
 }
 }

Hangman GUI

 import java.awt.*;
 import java.awt.event.ActionEvent;
 import java.awt.event.ActionListener;
 import javax.swing.*;
 public class HangmanGUI {
 // GUI
 static JFrame frame;
 static JTextField textField;
 static JLabel guessesRemainingLabel;
 static JLabel lettersGuessedLabel;
 static JLabel secretWordLabel;
 // buildGUI - builds the GUI
 static void buildGUI(){
 SwingUtilities.invokeLater(
 new Runnable(){
 @Override
 public void run(){
 frame = new JFrame("Hangman");
 // JLabels
 guessesRemainingLabel = new JLabel("Guesses remaining: " + String.valueOf(Hangman.guessesRemaining));
 lettersGuessedLabel = new JLabel("Already guessed: ");
 secretWordLabel = new JLabel();
 // TextField & checkButton
 textField = new JTextField();
 JButton checkButton = new JButton("Guess");
 GuessListener guessListener = new GuessListener();
 checkButton.addActionListener(guessListener);
 textField.addActionListener(guessListener);
 // Panel for all the labels
 JPanel labelPanel = new JPanel();
 labelPanel.setLayout(new BoxLayout(labelPanel, BoxLayout.PAGE_AXIS));
 labelPanel.add(guessesRemainingLabel);
 labelPanel.add(lettersGuessedLabel);
 labelPanel.add(secretWordLabel);
 // User panel
 JPanel userPanel = new JPanel(new BorderLayout());
 userPanel.add(BorderLayout.CENTER, textField);
 userPanel.add(BorderLayout.EAST, checkButton);
 labelPanel.add(userPanel);
 // Add everything to frame
 frame.add(BorderLayout.CENTER, labelPanel);
 frame.setSize(250, 100);
 frame.setLocationRelativeTo(null);
 frame.setDefaultCloseOperation(WindowConstants.EXIT_ON_CLOSE);
 frame.setVisible(true);
 }
 }
 );
 }
 // drawGuessesRemaining - Outputs the guesses remaining
 static void drawGuessesRemaining(){
 final String guessesMessage = "Guesses remaining: " + String.valueOf(Hangman.guessesRemaining);
 SwingUtilities.invokeLater(
 new Runnable(){
 @Override
 public void run(){
 guessesRemainingLabel.setText(guessesMessage);
 guessesRemainingLabel.setAlignmentX(Component.LEFT_ALIGNMENT);
 }
 }
 );
 }
 // drawLettersGuessed - Outputs the letters guessed
 static void drawLettersGuessed(){
 StringBuilder lettersBuilder = new StringBuilder();
 for (Character el : Hangman.lettersGuessed) {
 String s = el + " ";
 lettersBuilder.append(s);
 }
 final String letters = lettersBuilder.toString();
 SwingUtilities.invokeLater(
 new Runnable() {
 @Override
 public void run() {
 lettersGuessedLabel.setText("Already guessed: " + letters);
 }
 }
 );
 }
 // drawSecretWord - draws the secret word with dashes & etc for user to use to guess the word with
 static void drawSecretWord(){
 StringBuilder word = new StringBuilder();
 for(int i=0; i<Hangman.lettersRevealed.length; i++){
 if (Hangman.lettersRevealed[i]) {
 String s = Hangman.secretWord.charAt(i) + " ";
 word.append(s);
 } else {
 word.append("_ ");
 }
 }
 final String w = word.toString();
 SwingUtilities.invokeLater(
 new Runnable(){
 @Override
 public void run() {
 secretWordLabel.setText(w);
 }
 }
 );
 }
 //playAgainDialog - shows the confirm w
 static int playAgainDialog(String m){
 return JOptionPane.showConfirmDialog(frame, m, "Play again?", JOptionPane.YES_NO_OPTION, JOptionPane.PLAIN_MESSAGE);
 }
 // GETTERS
 private static String getText(){
 return textField.getText();
 }
 // SETTERS
 static void setText(final String t){
 SwingUtilities.invokeLater(
 new Runnable() {
 @Override
 public void run() {
 textField.setText(t);
 }
 }
 );
 }
 // ActionListener
 private static class GuessListener implements ActionListener {
 @Override
 public void actionPerformed(ActionEvent ev){
 String guess = getText();
 if(Hangman.checkUserGuess(guess)) {
 Hangman.updateSecretWord(guess);
 drawSecretWord();
 if(Hangman.lettersGuessed.size() != 0) // No letters have been guessed by the user at the beginning
 drawLettersGuessed();
 // Checks if the user has won or lost
 if (Hangman.checkIfWon())
 Hangman.winSequence();
 else if (Hangman.guessesRemaining == 0)
 Hangman.loseSequence();
 }
 }
 }
 }

Answer 1

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<Character>(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<Integer> 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

• Calculus5000: Great, thanks for this! I’ve completely contained the GUI stuff in the GUI class now & have made all of its member variables & functions non-static. Should I also make the variables & methods in the main hangman class non-static? If so, how can I go about that as I use a lot of those methods in the GuessListener private class (which is in the HangmanGUI class)?
• tim: yes, those should be non-static as well. And your GuessListener shouldn't do all the things it's doing. Checking if the player won/lost should be part of the main game class, not the input retriever (that should really only get the input, nothing more). If you slim it down a lot, it should be easy to make everything non-static. ps: I rolled back your edit (see here: What you may and may not do after receiving answers)