Playing a game throughout the house

Question

Target

I wrote this program in a few free hours when I was on an excursion as "youth leader". I had created a game to explore the house, with a 40-field mechanic, using dice for advancing and question-based setbacks.

The dice and questions were managed outside the program. De-facto this was just to show progressions of the different teams on a screen.

The names of the team-members have been changed to protect the innocent. I tried to keep the Game configurable aside from the teams.

That said, let's jump into the code:

Code

Main-class:

/**
 * Class for containing the main method. Will only start up the Screen and keep
 * running until screen terminates (or rather signals execution stop)
 * 
 * @author vogel612
 *
 */
public class Program {

    public static void main(String[] args) {
        Screen screen = new Screen();
        screen.startGame();
        while (screen.isRunning()) {
        }
    }
}

I doubt there's much to say here. I am slightly unhappy about the program exit construction, but I doubt there's much to change here...

Display

import java.awt.Color;
import java.awt.Dimension;
import java.awt.EventQueue;
import java.awt.FlowLayout;
import java.awt.Toolkit;
import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;
import java.awt.event.WindowEvent;
import java.util.Arrays;

import javax.swing.JButton;
import javax.swing.JComboBox;
import javax.swing.JFrame;
import javax.swing.JLabel;
import javax.swing.JPanel;
import javax.swing.JTextField;
import javax.swing.SwingUtilities;
import javax.swing.border.LineBorder;

/**
 * Screen Class: Contains... everything frankly. Teams their colors a few settings and handlers for buttons.
 * While that maybe isn't the best idea, the simplicity of the problem allows this.
 * 
 * @author vogel612
 *
 */
public class Screen extends JFrame {

    private static final long serialVersionUID = 8208211925302053925L;

    private static final Color teamOneColor = new Color(0.3f, 0.0f, 0.3f, 0.8f);
    private static final Color teamTwoColor = new Color(0.0f, 0.3f, 0.3f, 0.8f);
    private static final Color noTeamColor = new Color(0.8f, 0.8f, 0.8f);
    private static final Color black = new Color(0.0f, 0.0f, 0.0f);

    private enum Team {
        Team_1(teamOneColor, "Male 1 & Female 1"), Team_2(teamTwoColor, "Male 2 & Female 2");

        private final Color teamColor;
        private int position;
        private final String teamName;

        Team(Color color, String name) {
            this.teamName = name;
            this.teamColor = color;
            this.position = 0;
        }

        public int getPosition() {
            return position;
        }

        public Color getColor() {
            return teamColor;
        }

        public boolean advance(int by) {
            if (by == 0 || by < -1) {
                throw new IllegalArgumentException();
            }
            if (position + by > FIELD_COUNT) {
                position = FIELD_COUNT;
                return true;
            }
            this.position += by;
            return false; // no game end
        }

        public String getTeamName() {
            return teamName;
        }
    }

    private static final int FIELD_COUNT = 40;
    private static final int FIELD_WIDTH = 50;
    private static final int FIELD_HEIGHT = 50;
    private static final Dimension defaultFieldDimension = new Dimension(
            FIELD_WIDTH, FIELD_HEIGHT);
    private static final Dimension defaultWindowDimension = new Dimension(
            ((int) Math.sqrt(FIELD_COUNT) + 3) * FIELD_WIDTH,
            ((int) Math.sqrt(FIELD_COUNT) + 3) * FIELD_HEIGHT);

    private final JComboBox<Team> teamInput = new JComboBox<>(Team.values());
    private final JTextField textInput = new JTextField(1);
    private final JButton submit = new JButton("OK");

    private final EventQueue events = Toolkit.getDefaultToolkit()
            .getSystemEventQueue();

    private boolean isRunning = false;

    private JPanel[] fields = new JPanel[FIELD_COUNT + 1];

    public Screen() {
        this.setDefaultCloseOperation(EXIT_ON_CLOSE);
        this.setTitle("Hausspiel Frankenhofen");
        this.setPreferredSize(defaultWindowDimension);
        this.setSize(defaultWindowDimension);
        this.setLayout(new FlowLayout(FlowLayout.LEFT));
    }

    public void startGame() {
        initialize();
        this.setVisible(true);
        this.isRunning = true;
        repaintPositions();
    }

    private void repaintPositions() {
        Arrays.stream(fields).forEach(f -> f.setBackground(noTeamColor));
        Arrays.stream(Team.values()).forEach(
                t -> fields[t.getPosition()].setBackground(t.getColor()));

        this.repaint();
    }

    private void initialize() {
        createFields();
    }

    private void createFields() {
        for (int i = 0; i <= FIELD_COUNT; i++) {
            JPanel field = new JPanel();
            field.add(new JLabel((i == 0) ? "Start" : i + ""));
            field.setName(i + "");
            field.setSize(defaultFieldDimension);
            field.setPreferredSize(defaultFieldDimension);
            field.setBackground(noTeamColor);
            field.setBorder(new LineBorder(black));
            this.add(field);
            fields[i] = field;
        }

        Arrays.stream(Team.values()).forEach(t -> {
            JPanel legend = new JPanel() {
                private static final long serialVersionUID = -2035379018013186886L;

                {
                    JLabel teamLabel = new JLabel(t + ": " + t.getTeamName());
                    setBackground(t.getColor());
                    add(teamLabel);
                }
            };
            this.add(legend);
        });

        submit.addActionListener(new ActionListener() {
            @Override
            public void actionPerformed(ActionEvent e) {
                if (textInput.getText().isEmpty()) {
                    return;
                }
                int advanceBy;
                try {
                    advanceBy = Integer.parseInt(textInput.getText());
                } catch (NumberFormatException ex) {
                    return;
                }

                Team team = (Team) teamInput.getSelectedItem();
                if (team.advance(advanceBy)) {
                    showWinAlert(team);
                }
                repaintPositions();
                textInput.setText("");
            }

            private void showWinAlert(Team team) {
                JFrame winAlert = new JFrame("Gewonnen");
                winAlert.setSize(new Dimension(300, 150));
                winAlert.setLayout(new FlowLayout());
                winAlert.add(new JLabel("Sieger: " + team.getTeamName()));
                JButton dismiss = new JButton("OK");
                dismiss.addActionListener(a -> {
                    events.postEvent(new WindowEvent(winAlert,
                            WindowEvent.WINDOW_CLOSING));
                });
                winAlert.add(dismiss);
                SwingUtilities.invokeLater(() -> {
                    winAlert.setVisible(true);
                });
            }
        });

        this.add(submit);
        this.add(textInput);
        this.add(teamInput);

        this.doLayout();
    }

    public boolean isRunning() {
        return isRunning;
    }
}

As mentioned in the javadoc, this class seems to be quite the god-object. It contains everything related to the game, that's not stored outside of the program. I am not interested in reviews concerning splitting this class up.

What's by far more interesting to me is the construction around the submit action listener.

The program is designed for java 1.8

I'd be especially interested in my use of lambdas.

In action:

Here's a small screenshot of how this looks on my laptop:

Answer 1

The validation of the text input is not great:

• The validation logic is sort of split between the action listener (ensure number) and the Team enum (ensure non-zero and not -1). It would be better to have all the validation logic in one place

• Throwing IllegalArgumentException in case of 0 or < -1 input doesn't seem very useful: nothing happens on the UI and a stack trace is printed on the console. It would be better to handle this the same way as non-numeric input (= do nothing)

• These validation rules seem a bit magical:

  • -1 is ok
  • 0 is not
  • -2 and below are not
  • all ok above 0

  It would be good to at least have some comments to explain these rules.

The code would be simpler if the text input was a pulldown, then you wouldn't need to parse to integer nor validate.

Somewhat related to this, the advance method violates the single responsibility principle: it advances a team, and also checks the game end condition. It would be better if it did just one thing.

---

The main method can be simplified:

public static void main(String[] args) {
    new Screen().startGame();
}

And you can get rid of Screen.isRunning: the main program will continue running as long as the screen is running, no need for the manual extra checks.

---

In this loop:

for (int i = 0; i <= FIELD_COUNT; i++) {
    // ...
    fields[i] = field;
}

The connection between the size of fields and the FIELD_COUNT constant is semantic: this code snippet doesn't guarantee that fields has enough capacity to contain FIELD_COUNT elements. The implementation will be more robust if you iterate until fields.length instead.

On a related note, unless you specifically need an array, it's recommend to use a List instead. It would eliminate such worries as index overflows in this loop, and all other potential uses of fields. The iteration in repaintPositions for example would also become more straightforward, using fields.stream() instead of Arrays.stream(fields). Finally, the fields member can be final.

---

Some lambda expressions are unnecessarily enclosed in {...}:

dismiss.addActionListener(a -> {
    events.postEvent(new WindowEvent(winAlert,
            WindowEvent.WINDOW_CLOSING));
});

// ...

SwingUtilities.invokeLater(() -> {
    winAlert.setVisible(true);
});

These could be simply:

dismiss.addActionListener(a -> events.postEvent(new WindowEvent(winAlert,
        WindowEvent.WINDOW_CLOSING)));

// ...

SwingUtilities.invokeLater(() -> winAlert.setVisible(true));

---

I think the most significant property of a team is the name, and the color is secondary. So I would put the constructor params in that order, and to be consistent, the member field declarations too.

---

public void startGame() {
    initialize();
    this.setVisible(true);
    repaintPositions();
}

Be consistent: either use this. prefix everywhere or nowhere (when not necessary).

---

In terms of usability, it would be easier if I could control the UI either with keyboard only or mouse only. In its current form I need both. You could change the text field to a pulldown just like you did for team, then it will be possible to control with mouse only, and it would also simplify input validation. For example, the game is intended to values of a dice, 1-6, but the UI doesn't prevent you from entering anything.

---

Lambda expressions are awesome, but there's no need to use them everywhere. For example when you create the the JLabels for the teams, actually it's slightly shorter to write with a good old-fashioned for-each loop, without creating a stream from Team.values. It might not matter at all which way you write. I just want to make the point that just because you can write something with a lambda, doesn't necessarily mean you should.

Answer 2

You can get rid of the busy-wait loop since Swing keeps another thread alive. It was hogging my cpu doing nothing.

You should have a good model/view separation from the start. When I do a small project like yours, I always start by doing a version where the GUI is just some output text to the console. But I am aware that I will move to a real GUI, so I modularize my code accordingly.

I think you are really abusing enum. It makes more sense to just have a normal Team class and then you can create as many Teams as you want.

Also I would not store the positions in the Team themselves, but in a Board class. When switching from a console based GUI to a Swing based GUI, all you have to do is changed the input method and the paint method. You should define some abstrac GUI interface with those two methods.

private static class Board {
    private final int fieldCount;
    private final LinkedHashMap<Team, Integer> teamsPositions = new LinkedHashMap<>();

    public Board(int fieldCount, List<Team> teams) {
        this.fieldCount = fieldCount;
        teams.forEach(team -> teamsPositions.put(team, 0));
    }

    // Should give more thought on the return value.  A team wins when they go over the max?
    public boolean move(Team team, int increment) {
        int oldPosition = teamsPositions.get(team);
        int position = oldPosition + increment;
        if (position >= fieldCount) {
            return false;
        } else {
            teamsPositions.put(team, position);
            return true;
        }
    }

    public int getFieldCount() {
        return fieldCount;
    }

    public LinkedHashMap<Team, Integer> getTeamsPositions() {
        return teamsPositions;
    }

}

---

private static class Team {
    private final Color teamColor;
    private final String teamName;

    Team(Color color, String name) {
        this.teamName = name;
        this.teamColor = color;
    }

    public Color getColor() {
        return teamColor;
    }

    public String getTeamName() {
        return teamName;
    }
}

On the functionality side, you should probably auto-toggle which team is active. But I don't know the rules and maybe sometimes some team skip their move.

An minor suggestion related to lambdas:

teams.stream().map(team -> 
        new JPanel() {
            private static final long serialVersionUID = -2035379018013186886L;
            {
                JLabel teamLabel = new JLabel(team + ": " + team.getTeamName());
                setBackground(team.getColor());
                add(teamLabel);
            }
        })
        .forEach(legendPanel -> Screen.this.add(legendPanel));

But it would actually be cleaner to write a static method to create a JPanel from a Team (not just for the lambda) and then the streaming simplifies to:

teams.stream()
        .map(Screen::createTeamLegendPanel)
        .forEach(legendPanel -> Screen.this.add(legendPanel));