Problem
I have the following code in Java:
HangmanFunctions class:
import java.util.ArrayList;
import java.util.Random;
public class HangmanFunctions {
private static final String[] WORDS = {"jazz", "buzz", "hajj", "fuzz", "jinx",
"jazzy", "fuzzy", "faffs", "fizzy", "jiffs",
"jazzed", "buzzed", "jazzes", "faffed", "fizzed",
"jazzing", "buzzing", "jazzier", "faffing", "fuzzing"};
private String word;
private ArrayList<Character> hiddenWord = new ArrayList<Character>();
private ArrayList<Character> lettersGuessed = new ArrayList<Character>();
public HangmanFunctions() {
setHiddenWord();
}
public ArrayList<Character> getLettersGuessed() {
return lettersGuessed;
}
private void setWord() {
word = WORDS[new Random().nextInt(19)];
}
public String getWord() {
return word;
}
private void setHiddenWord() {
setWord();
for (char letter: word.toCharArray()) {
hiddenWord.add('-');
}
}
public ArrayList<Character> getHiddenWord() {
return hiddenWord;
}
private ArrayList<Integer> getIndexesOf(char letter) {
ArrayList<Integer> instances = new ArrayList<Integer>();
for (int i = word.indexOf(letter); i >= 0; i = word.indexOf(letter, i + 1)) {
instances.add(i);
}
return instances;
}
public void revealLetter(char letter) {
for (int i: getIndexesOf(letter)) {
hiddenWord.set(i, word.charAt(i));
}
}
public void addGuess(char letter) {
lettersGuessed.add(letter);
}
private boolean isGuessed(char letter) {
return lettersGuessed.indexOf(letter) != -1;
}
public Results getResult(char letter) {
if (!isGuessed(letter)) {
if (getIndexesOf(letter).size() > 0) {
return Results.CORRECT;
}
else {
return Results.INCORRECT;
}
}
else {
return Results.ALREADY_GUESSED;
}
}
}
HangmanPanel class:
import java.awt.Dimension;
import java.awt.Font;
import java.awt.Graphics;
import java.awt.GridBagConstraints;
import java.awt.GridBagLayout;
import java.awt.Insets;
import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;
import java.util.ArrayList;
import javax.swing.JLabel;
import javax.swing.JOptionPane;
import javax.swing.JPanel;
import javax.swing.JTextField;
public class HangmanPanel extends JPanel {
private HangmanFunctions functions;
private JLabel word;
private JLabel lettersGuessed;
private JTextField inputBox;
private Font largeFont = new Font("sansserif", Font.PLAIN, 50);
private int hangmanPos = 0;
public HangmanPanel(HangmanFunctions functions) {
this.functions = functions;
setLayout(new GridBagLayout());
GridBagConstraints c = new GridBagConstraints();
c.insets = new Insets(10, 10, 10, 350);
word = new JLabel(toHiddenWord(functions.getHiddenWord()));
word.setFont(largeFont);
c.gridy = 0;
add(word, c);
lettersGuessed = new JLabel(getLettersGuessedString());
c.gridy = 1;
add(lettersGuessed, c);
inputBox = new JTextField();
inputBox.setPreferredSize(new Dimension(25, 25));
inputBox.addActionListener(new ActionListener() {
public void actionPerformed(ActionEvent e) {
newGuess(e.getActionCommand().charAt(0));
}
});
c.gridy = 2;
add(inputBox, c);
}
private String toHiddenWord(ArrayList<Character> word) {
String hiddenWord = " ";
for (int i = 0; i < word.size(); i++) {
hiddenWord += (word.get(i) + " ");
}
return hiddenWord;
}
private String getLettersGuessedString() {
String lettersGuessedString = "Letters Guessed: ";
for (char i: functions.getLettersGuessed()) {
lettersGuessedString += (i + ", ");
}
return lettersGuessedString;
}
private void newGuess(char letter) {
Results result = functions.getResult(letter);
if (result == Results.CORRECT) {
JOptionPane.showMessageDialog(null, "Correct.", "Hangman", JOptionPane.PLAIN_MESSAGE);
functions.addGuess(letter);
functions.revealLetter(letter);
}
else if (result == Results.INCORRECT) {
JOptionPane.showMessageDialog(null, "Incorrect.", "Hangman", JOptionPane.PLAIN_MESSAGE);
functions.addGuess(letter);
hangmanPos++;
repaint();
}
else {
JOptionPane.showMessageDialog(null, "Already Guessed.", "Hangman", JOptionPane.PLAIN_MESSAGE);
}
inputBox.setText("");
lettersGuessed.setText(getLettersGuessedString());
word.setText(toHiddenWord(functions.getHiddenWord()));
if (functions.getHiddenWord().indexOf('-') == -1) {
JOptionPane.showMessageDialog(null, "Congratulations, " + functions.getWord() + " was my word", "Hangman", JOptionPane.PLAIN_MESSAGE);
}
}
@Override
public void paintComponent(Graphics g) {
g.drawLine(350, 0, 350, 500);
switch (hangmanPos) {
case 9:
g.drawLine(500, 250, 525, 275);
JOptionPane.showMessageDialog(null, "Oops! You've been hanged! My word was " + functions.getWord(), "Hangman", JOptionPane.PLAIN_MESSAGE);
case 8:
g.drawLine(500, 250, 475, 275);
case 7:
g.drawLine(500, 225, 525, 225);
case 6:
g.drawLine(500, 225, 475, 225);
case 5:
g.drawLine(500, 200, 500, 250);
case 4:
g.drawOval(475, 150, 50, 50);
case 3:
g.drawLine(500, 100, 500, 150);
case 2:
g.drawLine(600, 100, 500, 100);
case 1:
g.drawLine(600, 350, 600, 100);
case 0:
g.drawLine(450, 350, 600, 350);
}
}
}
Results enum:
public enum Results {
CORRECT, INCORRECT, ALREADY_GUESSED
}
Frame class:
import javax.swing.JFrame;
public class Frame extends JFrame {
public Frame() {
super("Hangman");
setSize(700, 500);
setResizable(false);
setVisible(true);
add(new HangmanPanel(new HangmanFunctions()));
}
public static void main(String[] args) {
new Frame();
}
}
In theory, this code works, so I decided it was okay to post it here. However, when ran, it looks a bit like this:
So, I’m assuming there’s a lot of bad practices used in my code. Can anyone identify them?
Solution
Overview
Separate the game’s entry point, model, the view, and the controller into different classes:
- HangmanGame – The main entry point to the application.
- HangmanModel – Contains information about the word being guessed.
- HangmanView – Displays the results of the model.
- HangmanController – Updates the model based on events from the view.
Identify the responsibilities and behaviours of the classes.
HangmanGame
Responsibilities:
- Creates instances of the required classes (see also Spring)
- Exits the application.
- Creates a new game.
HangmanModel
Responsibilities:
- Load words.
- Select a word to guess.
- Retrieves positions that a letter is found (or throws an exception).
- Tracks incorrect guesses.
HangmanView
Responsibilities:
- Creates a new UI for the view.
- Updates the drawing of the hangman.
- Updates the letters being displayed.
- Notifies listeners of guesses.
- Notifies listeners of when the user requests to quit.
- Animates the hanging.
HangmanController
Responsibilities:
- Listens for events from the view.
- Tells the view when the game is over.
- Tells the view when another guess is made.
- Tells the view where to put the letters.
Implementation
Once you’ve identified the classes, their responsibilities, and their collaborators, think about how you would implement the actions described above. For example:
public class HangmanModel {
public void load( File dictionary ) { ... }
public int[] guess( char ch ) { ... }
public void badGuess() { ... }
public void pickWord() { ... }
}
public class HangmanGame {
public void initialize() { ... }
public void quit() { ... }
public void newGame() { ... }
}
public class HangmanView {
public void initialize() { ... }
public void drawHangman( HangmanStatus status ) { ... }
public void drawWord( String word[] ) { ... }
public void notifyGuess() { ... }
public void notifyQuit() { ... }
public void animateHanging() { ... }
}
Once you are satisfied that you have thought about most of the possible actions that each class will need to make the game, then consider the attributes each class needs and how the implementation will work.
Procedural Programming vs. OOP
The HangmanFunctions class is not object-oriented. When writing object-oriented code try to think in terms of the objects in the system. For example, you could further split HangmanView into two classes: HangmanUI and AnimatedHangman. The HangmanUI would represent the user interface (ASCII text in DOS vs. Swing vs. AWT) wherein the user interacts with the game. The AnimatedHangman would abstract drawing and animating the hangman figure in the game.
A set of functions is not an object-oriented class.