Factman - A two player numbers game

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 = G

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.

StackExchange Code Review Q#80640, answer score: 7