Factman - A two player numbers game

Question

I'm not sure if anyone else has ever heard of Factman, or even where the name comes from, but my AP Computer Science teacher calls this project Factman, so that's what I'm going with.

Naming aside, the game is simple. Players take turns choosing from a list of numbers. They get the number added to their score, but the other player get the sum of the factors. The number and all factors are removed from the list and the players must choose from the remaining numbers on all subsequent rounds. When the list is empty, the player with the higher score wins.

package games;

import java.awt.Color;
import java.awt.GridBagConstraints;
import java.awt.GridBagLayout;
import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;
import java.awt.event.KeyEvent;
import java.util.ArrayList;
import javax.swing.Box;
import javax.swing.JCheckBoxMenuItem;
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;
import javax.swing.KeyStroke;
import javax.swing.SwingUtilities;

class Factman_GUI extends JFrame {
    static JPanel panel;
    static JMenuBar menubar;
    static JCheckBoxMenuItem hideBoard;
    static JLabel p1ScoreLabel, p2ScoreLabel, turnIndicator, gameAreaLabel;
    static String userInput;
    static int userSelection = -1;
    static boolean newGameFlag = false;

    public Factman_GUI() {
        initGUI();
    }

    private void initGUI() {
        panel = new JPanel(new GridBagLayout());
        add(panel);

        // Generate the menu at the top of the window
        createMenu();

        //////////////////////////////////////////////////
        // First row, score labels
        // Score values will split all extra space evenly
        //
        GridBagConstraints c = new GridBagConstraints();
        JLabel p1Label = new JLabel("Player 1 Score:  ");
        c.anchor = GridBagConstraints.LINE_START;
        c.gridx = 0;
        c.gridy = 0;
        panel.add(p1Label, c);

        c = new GridBagConstraints();
        p1ScoreLabel = new JLabel("0");
        c.fill = GridBagConstraints.HORIZONTAL;
        c.anchor = GridBagConstraints.LINE_START;
        c.gridx = 1;
        c.gridy = 0;
        c.weightx = 0.5;
        panel.add(p1ScoreLabel, c);

        c = new GridBagConstraints();
        JLabel p2Label = new JLabel("Player 2 Score:  ");
        c.anchor = GridBagConstraints.LINE_START;
        c.gridx = 2;
        c.gridy = 0;
        panel.add(p2Label, c);

        c = new GridBagConstraints();
        p2ScoreLabel = new JLabel("0");
        c.fill = GridBagConstraints.HORIZONTAL;
        c.anchor = GridBagConstraints.LINE_START;
        c.gridx = 3;
        c.gridy = 0;
        c.weightx = 0.5;
        panel.add(p2ScoreLabel, c);

        //////////////////////////////////////////////////
        // Second row, main content area.
        // This spans all 4 columns and 2 rows
        //
        c = new GridBagConstraints();
        JPanel gameArea = new JPanel();
        gameArea.setLayout(new GridBagLayout());
        c.fill = GridBagConstraints.BOTH;
        c.gridx = 0;
        c.gridy = 1;
        c.gridwidth = 4;
        c.gridheight = 2;
        c.weighty = 1;
        panel.add(gameArea, c);

            c = new GridBagConstraints();
            gameAreaLabel = new JLabel("");
            c.anchor = GridBagConstraints.CENTER;
            c.fill = GridBagConstraints.BOTH;
            gameArea.add(gameAreaLabel, c);

        //////////////////////////////////////////////////
        // Third row, input area
        // This row contains another panel with its own layout
        // The first row indicates whose turn it is,
        // the second row takes user input. The text
        // field will take up all extra space
        //
        JPanel inputPanel = new JPanel();
        inputPanel.setLayout(new GridBagLayout());
        c = new GridBagConstraints();
        c.fill = GridBagConstraints.BOTH;
        c.gridx = 0;
        c.gridy = 3;
        c.gridwidth = 4;
        c.weightx = 1;
        panel.add(inputPanel, c);

            c = new GridBagConstraints();
            turnIndicator = new JLabel("It is Player 1's Turn");
            c.gridx = 0;
            c.gridy = 0;
            c.gridwidth = 4;
            inputPanel.add(turnIndicator, c);

            c = new GridBagConstraints();
            JLabel inputLabel = new JLabel("Enter your selection: ");
            c.fill = GridBagConstraints.BOTH;
            c.gridx = 0;
            c.gridy = 1;
            inputPanel.add(inputLabel, c);

            c = new GridBagConstraints();
            JTextField inputField = new JTextField();
            c.fill = GridBagConstraints.HORIZONTAL;
            c.gridx = 1;
            c.gridy = 1;
            c.gridwidth = 3;
            c.weightx = 1;
            inputField.addActionListener(new ActionListener() {
                public void actionPerformed(ActionEvent event) {
                    userInput = inputField.getText();
                    try {
                        userSelection = Integer.parseInt(userInput);
                    } catch (NumberFormatException e) {
                        System.out.println("No number entered...");
                    }
                    System.out.println(userInput);
                    inputField.setText("");
                }
            });
            inputPanel.add(inputField, c);


        // Set basic window properties
        setTitle("Factman Game");
        setSize(600,200);
        setLocationRelativeTo(null);
        setDefaultCloseOperation(EXIT_ON_CLOSE);
    }

    private void createMenu() {
        menubar = new JMenuBar();

        // Create the file menu
        JMenu filem = new JMenu("File");
        filem.setMnemonic(KeyEvent.VK_F);

        JMenuItem newGame = new JMenuItem("New Game");
        newGame.setAccelerator(KeyStroke.getKeyStroke(KeyEvent.VK_N,
            ActionEvent.CTRL_MASK));
        newGame.addActionListener(new ActionListener() {
            @Override
            public void actionPerformed(ActionEvent event) {
                int response = JOptionPane.showConfirmDialog(
                    panel,
                    "You are about to start a new game.\n"+
                    "Your current game will be lost.",
                    "Confirm New Game",
                    JOptionPane.OK_CANCEL_OPTION,
                    JOptionPane.WARNING_MESSAGE);
                if (response == 0) newGameFlag = true;
            }
        });

        JMenuItem quit = new JMenuItem("Exit");
        quit.setAccelerator(KeyStroke.getKeyStroke(KeyEvent.VK_Q,
            ActionEvent.CTRL_MASK));
        quit.addActionListener(new ActionListener() {
            @Override
            public void actionPerformed(ActionEvent event) {
                System.exit(0);
            }
        });

        filem.add(newGame);
        filem.addSeparator();
        filem.add(quit);

        // create the view menu
        JMenu viewm = new JMenu("View");
        viewm.setMnemonic(KeyEvent.VK_V);

        hideBoard = new JCheckBoxMenuItem("Hide Game Board");
        hideBoard.setState(false);        
        viewm.add(hideBoard);

        // Create the help menu
        JMenu helpm = new JMenu("Help");
        helpm.setMnemonic(KeyEvent.VK_H);

        JMenuItem gameInstructions = new JMenuItem("How to Play");
        gameInstructions.addActionListener(new ActionListener() {
            public void actionPerformed(ActionEvent event) {
                JOptionPane.showMessageDialog(
                    panel,
                    "<html>" +
                        "<p>Factman is a pretty simple game once you know the rules.<br>" +
                           "To play, each player will take turns selecting a number<br>" +
                           "from the list. The player will earn the number of points<br>" +
                           "equal to the number they selected. But be careful, if you<br>" +
                           "choose a number not in the list, you loose a turn!</p>" +
                        "<p></p>" +
                        "<p>When a player chooses a number, the other player will gain<br>" +
                           "the number of points for each of the factors in the list.<br>" +
                           "Any number that is used (selected or a factor) is removed<br>" +
                           "from the list.</p>" +
                        "<p></p>" +
                        "<p>The player with the highest score when the list is empty wins.</p>" +
                        "<p></p>" +
                        "<p>Good Luck!</p>" +
                    "</html>",
                    "How to Play",
                    JOptionPane.INFORMATION_MESSAGE);

            }
        });

        helpm.add(gameInstructions);        

        // Populate the menu bar
        menubar.add(filem);
        menubar.add(viewm);
        menubar.add(Box.createHorizontalGlue());
        menubar.add(helpm);

        // Set the menu bar in the panel
        setJMenuBar(menubar);
    }
}

public class Factman_Swing extends Factman_GUI {
    static ArrayList<Integer> gameBoard;
    static int upperBound, factorIndex, p1Score = 0, p2Score = 0;
    static boolean player1 = true;

    public static void main(String args[]) {
        SwingUtilities.invokeLater(new Runnable() {
            public void run() {
                Factman_GUI factman = new Factman_GUI();
                factman.setVisible(true);
            }
        });

        playGame();
    }

    public static void playGame() {
        // set the flag false to prevent a new game when someone wins
        newGameFlag = false;
        // make sure the label text is black
        // gameAreaLabel.setForeground(Color.black);

        // create a popup window to get the upper bound
        upperBound = Integer.parseInt(JOptionPane.showInputDialog(
            panel, "Enter the upper bound for this game", null));
        System.out.println("Upper bound = " + upperBound);

        // generate the arraylist with the given upper limit
        gameBoard = createList(upperBound);
        System.out.println(gameBoard);

        // as long as there are numbers left in the list, keep looping the game
        while(!gameBoard.isEmpty()) {
            // if the new game option was selected, go back to main
            if (newGameFlag) return;

            // show the list in the GUI
            gameAreaLabel.setVisible(!hideBoard.getState());
            gameAreaLabel.setText(gameBoard.toString());

            // indicate whose turn it is in the GUI
            if(player1) turnIndicator.setText("It's Player 1's Turn");
            else        turnIndicator.setText("It's Player 2's Turn");

            // userSelection becomes non-zero when a
            // number is entered in the text field
            if (userSelection >= 0) {
                // save the input and set it back to zero
                // so the loop doesnt fire again
                int selection = userSelection;
                userSelection = -1;
                System.out.println("User selected " + selection);

                // wrap the selection in an Integer object for comparison with the list
                Integer number = new Integer(selection);
                // the player will loose his/her turn if an invalid number is entered
                if (!gameBoard.contains(number)) {
                    JOptionPane.showMessageDialog(
                        panel,
                        "The number you selected is not in the list.\nYou loose a turn",
                        "OOPS",
                        JOptionPane.ERROR_MESSAGE);
                    player1 = !player1;
                    continue;
                }

                // add the selection to the current player's score
                if (player1) p1Score += selection;
                else         p2Score += selection;

                // search for and remove the selection from the list
                removeInt(gameBoard, selection);

                // as long as there are factors, add them to the other
                // players score and remove them from the list
                do {
                    factorIndex = findFactor(gameBoard, selection);
                    if (factorIndex >= 0) {
                        int value = gameBoard.get(factorIndex).intValue();
                        if (player1) p2Score += value;
                        else         p1Score += value;
                        // remove the factor
                        removeInt(gameBoard, value);
                    }
                } while (factorIndex >= 0);    // loop until no factor is found

                // show the scores in the GUI
                p1ScoreLabel.setText(String.valueOf(p1Score));
                p2ScoreLabel.setText(String.valueOf(p2Score));

                // switch players
                player1 = !player1;
            }
        }

        // Show who won
        gameAreaLabel.setForeground(Color.blue);
        if (p1Score > p2Score)       gameAreaLabel.setText("PLAYER 1 WINS!!!!");
        else if (p1Score < p2Score)  gameAreaLabel.setText("PLAYER 2 WINS!!!!");
        else gameAreaLabel.setText("Somehow, you managed to tie.  Nice going.");
    }

    /**
     * Create a list of Integer objects from 1 to limit, inclusive.
     * @param limit the upper bound of the list
     * @return an ArrayList of Integer type 
     */
    public static ArrayList<Integer> createList(int limit) {
        ArrayList<Integer> temp = new ArrayList<Integer>();
        for (int i = 1; i <= limit; i ++) {
            temp.add(new Integer(i));
        }
        return temp;
    }

    /**
     * Search for the specified value in the list and remove the object
     * from the list. The remove method of the ArrayList class removes
     * the object and shifts all of the objects following it to the
     * left one index.
     * @param list  an ArrayList of Integers to search
     * @param value the value to remove from the list
     * @see java.util.ArrayList#remove
     */
    private static void removeInt(ArrayList<Integer> list, int value) {
        // loop through the list until the value of the object matches
        // the specified value, then remove it
        for (int i = 0; i < list.size(); i ++) {
            if (list.get(i).intValue() == value) {
                list.remove(i);
            }
        }
    }

    /**
     * Returns the index of the first factor of the specified number in
     * the specified ArrayList.  If no factor is found, -1 is returned.
     * @param list   an ArrayList of Integers to search
     * @param number the value to find factors of
     * @return the index of the first factor, or -1 if no factors exist
     */
    private static int findFactor(ArrayList<Integer> list, int number) {
        // loop through the list until the end or the specified number
        // this prevents index exceptions
        for (int i = 0; i < list.size() && i < number; i ++) {
            // check if the value divides evenly into the number
            if (number % list.get(i).intValue() == 0) {
                return i;
            }
        }
        // we only get here if no index was found
        return -1;
    }
}

Any thoughts, questions, suggestions will be greatly appreciated. I am fairly new to java and this is my first swing application, so don't be surprised by any funky code.

Answer 1

Large Methods

Your methods are definitely too long. As a rule of thumb, if a method doesn't fit on the screen, it becomes hard to understand and maintain.

Try to extract code to their own methods, so that each method only does one thing.

As an example, your playGame method does:

• gather pre-game settings for the current game (upper bound)
• manage the game loop
• manages the GUI (toggles visibility, displays user info, etc)
• manages game scores
• manages game logic
• displays post-game information (who won)

That's just too much for one method. Extracting code to methods will also reduce your in-code comments if you choose good method names.

OOP

One of the reason that your methods are so long is that you only have two classes (with unclear responsibilities); I would add a lot more classes. It is often easiest to first sketch them out on a piece of paper, here are some general ideas to get you started:

• UserInput: interface and implementing class to get user input. That way, you could later easily exchange the way you get input.
• PrintDebug: an interface and implementing class for all your system.out.print statements. That way, it will be really easy to later disable this, log it to a file, etc
• Game class: here you can put your game logic (and possibly game loop if you do not put that in its own class). This will not contain any information on how to actually display the game. That way, you always know where to look for changing the game logic, and you could easily exchange the display method if you want to.
• GUI: interface and implementing class; this contains the displaying of the game. I would probably put eg the menu in its own GUI class as well.
• Controller: this class combines all the other classes.

Naming

• Factman_Swing and Factman_GUI are not very good names. They both express similar meanings (a gui), and it seems that GUI actually contains more swing elements than swing.
• c, filem, viewm, and helpm are not very good names.

Comments

• your JavaDoc comments are great.
• but your in-code comments are a bit much and sometimes actually hurt readability. eg make sure the label text is black or // indicate whose turn it is in the GUI doesn't tell the reader anything that the code doesn't already. In-code comments should document why you do something, not what you do (good code, method names and JavaDoc comments do that).

Usability

• when I start a new game, PLAYER X WINS!!!!! is displayed instead of the new list, and a new game is not started

Misc

• your fields should be private (and not static, see OOP). If they are needed outside the class, add getters.
• always use curly brackets, even for one-line statements.
• your indentation is sometimes off, which makes your code harder to read.
• Integer number = new Integer(selection); is unnecessary.
• removeInt: you can just use list.removeAll((Integer) value); or list.removeAll(new Integer(value)); instead of implementing removal yourself.

Answer 2

Use enhanced for loops for better performance:

For example, instead of

private static void removeInt(ArrayList<Integer> list, int value) {
        // loop through the list until the value of the object matches
        // the specified value, then remove it
        for (int i = 0; i < list.size(); i ++) {
            if (list.get(i).intValue() == value) {
                list.remove(i);
            }
        }
}

do

private static void removeInt(ArrayList<Integer> list, int value) {
        // loop through the list until the value of the object matches
        // the specified value, then remove it
        for (Integer element:list) {
            if (element == value) {
                list.remove(element);
            }
        }
}

Also note, since Java 5 you don't need to call intValue() as this conversion from Integer to int is taken care of automatically by autoboxing.