3
\$\begingroup\$

I would like to improve code readability and reduce the complexity of my code to improve the maintainability of the source code.

If you have any tips, please say so. I was also wondering if it would be a good feature to add a 2-player option to this game. Currently, the game runs off of a word list in which it randomly selects a word.

package com.game;

import java.awt.BorderLayout;
import java.awt.Font;
import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;
import java.awt.event.KeyEvent;

import javax.swing.BorderFactory;
import javax.swing.BoxLayout;
import javax.swing.ImageIcon;
import javax.swing.JFrame;
import javax.swing.JLabel;
import javax.swing.JMenu;
import javax.swing.JMenuBar;
import javax.swing.JMenuItem;
import javax.swing.JOptionPane;
import javax.swing.JPanel;
import javax.swing.JTextField;

class GameStructure {
    private String[] wordList = { "computer", "java", "activity", "alaska",
            "appearance", "javatar", "automobile", "falafel", "birthday",
            "canada", "central", "character", "chicken", "chosen", "cutting",
            "daily", "darkness", "shawarma", "disappear", "driving", "effort",
            "establish", "exact", "establishment", "fifteen", "football",
            "foreign", "frequently", "frighten", "function", "gradually",
            "hurried", "identity", "importance", "impossible", "invented",
            "italian", "journey", "lincoln", "london", "massage", "minerals",
            "outer", "paint", "particles", "personal", "physical", "progress",
            "quarter", "recognise", "replace", "rhythm", "situation",
            "slightly", "steady", "stepped", "strike", "successful", "sudden",
            "terrible", "traffic", "unusual", "volume", "yesterday" };
    private JTextField tf;
    private JLabel jlLetsUsed;
    static String letter;
    static int[] wordLength = new int[64];
    static int level = (int) (Math.random() * 64);// random word
    static JFrame frame = new JFrame();
    static ImageIcon bg = new ImageIcon("hangman1.png");
    static ImageIcon logo = new ImageIcon("logo.png");
    static ImageIcon ic = new ImageIcon("hangmanIcon.png");
    JLabel img = new JLabel(bg, JLabel.CENTER);

    public void window() {
        bg = new ImageIcon("hangman1.png");
        img.setIcon(bg);
        JMenuBar menuBar = new JMenuBar();
        JMenu menu = new JMenu("File");
        menu.setMnemonic(KeyEvent.VK_A);
        menuBar.add(menu);
        JMenuItem menuItem = new JMenuItem("Developer", KeyEvent.VK_T);
        menu.add(menuItem);
        menuItem.addActionListener(new ActionListener() {
            @Override
            public void actionPerformed(ActionEvent e) {// right click key
                JOptionPane.showMessageDialog(frame, "Developer: Joe Eid"
                        + "\n" + "\n" + "The Javatar", "Developer",
                        JOptionPane.INFORMATION_MESSAGE, logo);
            }// end actionPerformed method
        });
        JMenuItem menuItem2 = new JMenuItem("Instructions", KeyEvent.VK_T);
        menu.add(menuItem2);
        menuItem2.addActionListener(new ActionListener() {
            @Override
            public void actionPerformed(ActionEvent e) {// right click key
                JOptionPane
                        .showMessageDialog(
                                frame,
                                " Hangman is a guessing game where the word"
                                        + "\n"
                                        + " to guess is represented by dashes. The player"
                                        + "\n"
                                        + " is given the option to enter a letter. If the letter"
                                        + "\n"
                                        + " guessed is contained in the word, the letter will"
                                        + "\n"
                                        + " replace the dash in its approprate placement."
                                        + "\n"
                                        + " You can not exceed 6 wrong guesses or else you"
                                        + "\n"
                                        + " lose. Words are selected randomly.",
                                "Instructions",
                                JOptionPane.INFORMATION_MESSAGE, logo);
            }// end actionPerformed method
        });
        JMenuItem menuItem3 = new JMenuItem("Restart", KeyEvent.VK_T);
        menu.add(menuItem3);
        menuItem3.addActionListener(new ActionListener() {
            @Override
            public void actionPerformed(ActionEvent e) {// right click key
                GameStructure restart = new GameStructure();
                level = (int) (Math.random() * 64);// random word
                restart.window();
            }// end actionPerformed method
        });
        JMenuItem menuItem4 = new JMenuItem("Exit", KeyEvent.VK_T);
        menu.add(menuItem4);
        menuItem4.addActionListener(new ActionListener() {
            @Override
            public void actionPerformed(ActionEvent e) {// right click key
                System.exit(0);
            }// end actionPerformed method
        });

        JFrame gameFrame = new JFrame();
        JPanel bottomRight = new JPanel();
        JPanel bottomLeft = new JPanel();
        JPanel top = new JPanel();
        JPanel bottom = new JPanel();
        JPanel imgPane = new JPanel();
        JPanel panel1 = new JPanel();
        bottom.setLayout(new BoxLayout(bottom, BoxLayout.X_AXIS));
        imgPane.setLayout(new BorderLayout());
        panel1.setLayout(new BorderLayout());
        panel1.setOpaque(false);// !!
        top.setBorder(BorderFactory.createTitledBorder(""));
        bottom.setBorder(BorderFactory.createTitledBorder(""));
        tf = new JTextField(1);
        JLabel jl = new JLabel("Enter a letter", JLabel.CENTER);
        jlLetsUsed = new JLabel("Letters used:  ", JLabel.CENTER);
        final JLabel jlLines = new JLabel("__ ", JLabel.CENTER);
        jl.setFont(new Font("Rockwell", Font.PLAIN, 20));
        tf.setFont(new Font("Rockwell", Font.PLAIN, 20));
        jlLetsUsed.setFont(new Font("Rockwell", Font.PLAIN, 20));
        jlLines.setFont(new Font("Rockewell", Font.PLAIN, 20));
        imgPane.add(img);// center
        top.add(jl);// top center
        top.add(tf);// top center
        bottomLeft.add(jlLetsUsed);// bottom left position
        bottomRight.add(jlLines);// bottom right position
        bottom.add(bottomLeft);// bottom
        bottom.add(bottomRight);// bottom
        panel1.add(imgPane, BorderLayout.CENTER);// background image (center)
        panel1.add(top, BorderLayout.NORTH);// text field and jlabel (top)
        panel1.add(bottom, BorderLayout.SOUTH);// blank spaces and letters used
        gameFrame.setJMenuBar(menuBar);
        gameFrame.setTitle("Hangman");
        gameFrame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        gameFrame.setIconImage(new ImageIcon("logo.png").getImage());
        gameFrame.setResizable(false);
        gameFrame.add(panel1);
        gameFrame.setSize(800, 500);
        gameFrame.setLocationRelativeTo(null);
        gameFrame.setVisible(true);

        int j = 0;
        String line = "";
        for (j = 0; j < 64; j++) {
            wordLength[j] = wordList[j].length();// gets length of words
        }// end for
        int m = 0;
        // creates line first then put into .setText
        while (m < wordLength[level]) {
            line += "__ ";
            m++;
        }// end for
        jlLines.setText(line);

        tf.addActionListener(new ActionListener() {
            int wrong = 0;
            int right = 0;

            @Override
            public void actionPerformed(ActionEvent e) {// enter key
                JTextField tf = (JTextField) e.getSource();
                letter = tf.getText();
                tf.setText("");
                // tf.requestFocus();
                jlLetsUsed.setText(jlLetsUsed.getText() + letter + " ");// sets
                                                                        // jlabel
                                                                        // text
                                                                        // to
                                                                        // users
                                                                        // entered
                                                                        // letter
                char[] jlabelText = jlLines.getText().toCharArray();// converts
                                                                    // string to
                                                                    // character
                                                                    // array
                                                                    // (array is
                                                                    // length of
                                                                    // string)
                char userEnteredChar = letter.charAt(0);
                // System.out.println(wordList[level]);
                if (!wordList[level].contains(letter)) {
                    wrong++;
                    if (wrong == 1) {
                        bg = new ImageIcon("hangman2.png");
                        img.setIcon(bg);
                    }
                    if (wrong == 2) {
                        bg = new ImageIcon("hangman3.png");
                        img.setIcon(bg);
                    }
                    if (wrong == 3) {
                        bg = new ImageIcon("hangman4.png");
                        img.setIcon(bg);
                    }
                    if (wrong == 4) {
                        bg = new ImageIcon("hangman5.png");
                        img.setIcon(bg);
                    }
                    if (wrong == 5) {
                        bg = new ImageIcon("hangman6.png");
                        img.setIcon(bg);
                    }
                    if (wrong == 6) {
                        bg = new ImageIcon("hangman7.png");
                        img.setIcon(bg);
                    }
                    if (wrong == 6) {
                        JOptionPane
                                .showMessageDialog(frame, "He's dead." + "\n"
                                        + "Press 'OK' to restart." + "\n"
                                        + "The word was " + wordList[level]
                                        + ".", "You Lost",
                                        JOptionPane.INFORMATION_MESSAGE, ic);
                        GameStructure restart = new GameStructure();
                        level = (int) (Math.random() * 64);// generate new
                                                            // random word
                        restart.window();
                    }
                    return;
                }
                int i = 0;
                for (i = 0; i < wordList[level].length(); i++) {
                    if (wordList[level].charAt(i) == userEnteredChar) {
                        jlabelText[3 * i] = ' ';
                        jlabelText[3 * i + 1] = userEnteredChar;
                        right++;
                    }// end if
                }// end for
                jlLines.setText(String.valueOf(jlabelText));
                if (jlabelText.length / 3 == right) {
                    JOptionPane.showMessageDialog(frame, "You got the word!"
                            + "\n" + "Press 'OK' for new word", "Good Job",
                            JOptionPane.INFORMATION_MESSAGE, logo);
                    GameStructure restart = new GameStructure();
                    level = (int) (Math.random() * 64);// generate new
                                                        // random word
                    restart.window();
                }
            }// end actionPerformed method
        });
    }// end window method
}

public class GameMain {
    public static void main(String[] args) {
        GameStructure game = new GameStructure();
        game.window();
    }
}
\$\endgroup\$
4
  • \$\begingroup\$ Is there a reason that you keep posting questions asking us to review similar code that you have had reviewed before? \$\endgroup\$
    – syb0rg
    Dec 27, 2013 at 17:47
  • 1
    \$\begingroup\$ the others weren't a complete game, this is the completed game \$\endgroup\$
    – Malachi
    Dec 27, 2013 at 17:51
  • \$\begingroup\$ @Malachi Agreed, but I can see some points that I would fix which I have mentioned before to the OP. \$\endgroup\$
    – syb0rg
    Dec 27, 2013 at 18:02
  • 1
    \$\begingroup\$ there were some things in my code that I didn't change, mostly because I wasn't sure the code needed the changes at that specific step, etc \$\endgroup\$
    – Malachi
    Dec 27, 2013 at 18:06

1 Answer 1

3
\$\begingroup\$

Review these general concepts for a Hangman game. Notice how LazySloth13's hangman game and your hangman game are not reusable. That is, there is no way to take your vision of hangman and get the vision from LazySloth13, or vice-versa. Yet they are both hangman, and a simple version at that.

Object-Oriented Programming has reusability as a core goal. A concept that helps achieve that goal is the Open-Closed Principle, along with other SOLID practices. Both versions of hangman violate most of these principles in similar ways.

Most predominately, the biggest violation is that there is no separation between the game's logic and the game's presentation. What if you wanted to port your game to the Web? A mobile device? A custom piece of hardware? As it stands, a lot of code would have to be re-written.

Let's take a closer look at how the code violates the SOLID principles.

Single Responsibility

Each class should be limited to a single reason for changing it. In the given code, if I wanted to change the number of guesses or the list of words, there is only a single class. This tight coupling means that it is possible to affect the code for guesses by changing the code for the list of words. Ideally, we'd want this to be impossible.

This points to the first change: create a HangmanDictionary class that is responsible for all aspects of the game's dictionary:

  • Set the dictionary source (could be an array, an input stream, a database, or HTTP connection).
  • Load a dictionary (for a given language).
  • Return a randomly selected word.

That's it. A dictionary doesn't know about guessing words. The mechanism behind guessing words is part of the rules.

Open-Closed Principle

Once a class is finished (i.e., it performs the single responsibility required), it should be closed from further modifications. In the code, it is impossible to change the font without changing the class itself:

    jl.setFont(new Font("Rockwell", Font.PLAIN, 20));
    tf.setFont(new Font("Rockwell", Font.PLAIN, 20));
    jlLetsUsed.setFont(new Font("Rockwell", Font.PLAIN, 20));
    jlLines.setFont(new Font("Rockewell", Font.PLAIN, 20));

This is due to the tight coupling between the logic and the view, which points to the second change: create a class that is responsible for displaying the results of the guesses, called HangmanView. In it you might see:

protected Font getTextFont() {
  return new Font( "Rockwell", Font.PLAIN, 20 );
}

If I wanted to change the font, now I could create a subclass of HangmanView and override a single method getTextFont(). Your original code would remain untouched, thereby ensuring to a great extent that my code would not introduce bugs in your code. (My code could still be buggy, but that's a different issue.)

Liskov Substitution

From CS 3443,

objects in a program should be replaceable with instances of their subtypes without altering the correctness of that program.

I cannot substitute my own subclass of GameStructure. This is because there is only one method to override: window(), which contains the entirety of the program.

The third change, which should be reasonably apparent by this time, is to separate the functionality of the window() method into various classes, each with its own single responsibility. What if I wanted to change how the restart functionality worked? Currently:

GameStructure restart = new GameStructure();
                level = (int) (Math.random() * 64);
                restart.window();

Note that it should not be required to create a new instance of the entire game just to reset it. Resetting "hangman" means telling the view to reset, telling the rules to reset (e.g., the number of guesses), and then picking a new random word. The above code should be included in a method such as:

public void reset() {
  getView().reset();
  getRules().setGuessWord( getDictionary().getRandomWord() );

  // Setting the new word to guess could call reset as it makes
  // little sense, in Hangman, to pick a new word but keep the
  // current tally of incorrect guesses.
  getRules().reset();
}

Now, if I wanted to change the way resetting the works, I can simply override the reset() method. (For example, what if I wanted to make a difficulty level that, in English, chooses words without the letters R S T L N E?)

Interface Segregation

There's not much to say here except that there is no code that a client class can reuse. My answer to a similar question has details on how to separate the interfaces.

Dependency Inversion

Martin Fowler identified three types of dependency injection:

  • Interface - clients must implement a specific interface.
  • Setter - expose setter methods for injecting dependencies.
  • Constructor - clients inject dependencies upon instantiation.

I, personally, feel that classes should "just work" upon instantiation and tend to opt for setter injection. This implies a fourth change: use class-scoped variables that can be changed while the program is running.

Assuming the addition of a HangmanDictionary class, I should see:

public class HangmanDictionary {
  // Objects should "just work", so give some initially valid data.
  private String[] words = { "computer", ... };

  public void load( File file ) {
    load( new FileInputStream( file ) );
  }

  public void load( InputStream i ) { 
    String wordList[];

    // ... reads the words from an input stream

    // Now replace the words used for the game.
    setWords( wordList );
  }

  protected void setWords( String[] words ) {
    assert words != null;

    if( words.length > 0 ) {
      this.words = words;
    }
  }
}

Now, at any point, the user can opt to change the dictionary for the game. Note that this would allow for switching the language, for example.

Specific Comments

There are a few comments I'd like to make about the code.

if (wrong == 1) {
  bg = new ImageIcon("hangman2.png");
  img.setIcon(bg);
}

You can eliminate all the tests for wrong == # with:

  bg = new ImageIcon("hangman" + wrong + ".png");
  img.setIcon(bg);

Reduce the numbering; rename hangman2.png to hangman1.png, for example.

The following code is, again, a violation of some of the principles described.

if (wrong == 6) { // ...

Should be:

if( getRules().exceedsGuesses( wrong ) ) { // ...

Or, at the very least:

if( wrong == getMaxGuesses() ) { // ...

But this is not as flexible as using a rules class.

Lastly, add a separate method to set up the user interface.

JFrame gameFrame = new JFrame();
// ... code ...
JPanel panel1 = new JPanel();
// ... code ...
gameFrame.setVisible(true);

Separate all of that code into another method, or, even better, a HangmanView class.

\$\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.