11
\$\begingroup\$

For the community challenge, I wrote ultimatoe. For me, the most interesting part is clearly the AI, but what I did so far is just a stupid Monte Carlo tree search, which isn't ready for a review yet (but may come soon).

Before reviewing, please have a look at the README. The text is much shorter than the code.

I'd like you have a look mainly at the GUI. Especially tips making it better without much effort are welcome. It works by transforming game.asString() into graphical representation (i.e., it makes from the character 'X' a field containing "X", which is a funny (and probably stupid) idea. The string representation of the below board would be something like "XXX*-X-* \n *--O* O \n...".

ultimatoe

I'm leaving out the imports as boring.

Game

/** An immutable representation of a game (state). */
@Immutable public interface Game<G extends Game<G>> {
    /**
     * Return the list of all players, including dummies (like e.g., a player representing "no winner").
     *
     * <p>The starting player is the first element, other real players follow, dummies are placed last.
     */
    ImmutableList<GamePlayer<G>> players();

    /** Return the zero-based turn (i.e., the number of moves already played). */
    int turn();

    /**
     * Return the player to go, unless the game has already finished.
     *
     * <p>In case {@link #isFinished()} returns true, the result is undefined.
     */
    GamePlayer<G> playerOnTurn();

    /** Return the winner. If there's none, a special dummy gets returned. */
    GamePlayer<G> winner();

    /**
     * Return the score for the initial player (the bigger the better for them).
     * The result lies between -1 and +1.
     *
     * <p>Note that there's no heuristics involved, the score reflects the rules only.
     *
     * <p>If applicable for the given game, the result is<ul>
     * <li>{@code +1.0}, when the {@link #winner()} is {@code players().get(0)}
     * <li>{@code -1.0}, when the {@link #winner()} is {@code players().get(1)}
     * <li>{@code 0.0}, otherwise.</ul>
     */
    double score();

    /** Return true if the game has been finished. */
    boolean isFinished();

    /** Return a map of all directly reachable states to the corresponding moves. */
    ImmutableBiMap<G, String> children();

    /**
     * Return a game state resulting from applying a move represented by the argument string.
     * The set of all possible moves may be obtained via {@code children().values()}.
     *
     * @throws IllegalArgumentException if the game has already finished or the move is illegal or malformed.
     */
    G play(String move);

    /**
     * Return a game state resulting from applying a randomly chosen move.
     *
     * @throws IllegalArgumentException if the game has already finished.
     */
    G play(Random random);

    /**
     * Return a unique string representation suitable for automated processing and allowing to recreate the game.
     *
     * <p>Using this method rather than {@link toString} allows the latter
     * to omit some information or add redundant text to get mare human-friendly.
     */
    String asString();
}

GamePlayer

/**
 * A representation of an abstract player (like "black" or "white" in chess).
 *
 * <p>Not to be confused with {@link GameActor}}.
 */
@Immutable public interface GamePlayer<G extends Game<G>> {
    /** Return an int uniquely identifiying {@code this}. */
    int ordinal();

    /**
     * Return true if {@code this} represents a dummy (e.g., no winner) rather than
     * a real player (e.g., "X" or "O" in tic-tac-toe).
     */
    boolean isDummy();
}

GameActor

/** Represents a real player (unlike {@link GamePlayer}) of game {@code G}, e.g., an AI player. */
public interface GameActor<G extends Game<G>> {
    /** Return the initial state of {@code G}, like e.g. an empty tic-tac-toe board. */
    G initialGame();

    /** Return the "best" move for the given game state. */
    String selectMove(G game);

    GameAIParameters parameters();
}

UltimatoeGui

Apart from two constructors and a constant, the GUI uses only interface methods (I hope so). So you should be able to find everything needed above.

/** The GUI for the {@link Ultimatoe} game. */
public final class UltimatoeGui implements GameListener<Ultimatoe> {
    private final class FieldListener implements ActionListener {
        @Override public void actionPerformed(ActionEvent e) {
            if (swingWorker!=null) return; // It's a AI turn.
            final int guiIndex = buttons.indexOf(e.getSource());
            final int guiX = guiIndex % N_OF_GUI_FIELDS;
            final int guiY = guiIndex / N_OF_GUI_FIELDS;
            // Convert the gui coordinates to game cooredinates in range 0..8.
            final int x = guiX - guiX/4;
            final int y = guiY - guiY/4;
            final String move = UltimatoeUtils.coordinatesToMoveString(x, y);
            setState(game.play(move));
        }
    }

    private final class FieldButton extends JButton {
        @Override protected void paintComponent(Graphics g) {
            super.paintComponent(g);
            final String text = getText();
            if (text.isEmpty() || text.charAt(0) != UltimatoeUtils.BORDER) return;
            setForeground(new Color(0x20FFC0C0, true));
            g.fillRect(0, 0, getWidth(), getHeight());
        }
    }

    private UltimatoeGui() {
        final JButton fasterButton = new JButton(new AbstractAction("faster") {
            @Override public void actionPerformed(ActionEvent e) {
                autoplayDelayMillis /= SPEEDUP_FACTOR;
            }
        });

        final ActionListener autoListener = new ActionListener() {
            /** Switch the AI for a player on or off. */
            @Override public void actionPerformed(ActionEvent e) {
                final JToggleButton b = (JToggleButton) e.getSource();
                final boolean isOn = b.getModel().isSelected();
                final boolean isX = b.getText().endsWith("X");
                if (isX) {
                    actors[1] = isOn ? new GameMonteCarloActor<>(INITIAL_GAME) : null;
                } else {
                    actors[0] = isOn ? new GameRandomActor<>(INITIAL_GAME) : null;
                }

                final boolean isFullAuto = actors[0]!=null && actors[1]!=null;
                fasterButton.setEnabled(isFullAuto);
                autoplayDelayMillis = MAX_AUTOPLAY_DELAY_MILLIS;
                autoplay();
            }
        };

        for (final String s : "auto-X auto-O".split(" ")) {
            final JToggleButton autoButton = new JToggleButton(s);
            autoButton.addActionListener(autoListener);
            controlPanel.add(autoButton);
        }

        fasterButton.setEnabled(false);
        controlPanel.add(fasterButton);

        mainPanel.setLayout(new GridLayout(N_OF_GUI_FIELDS, N_OF_GUI_FIELDS));
        for (int i=0; i<N_OF_GUI_FIELDS*N_OF_GUI_FIELDS; ++i) {
            final FieldButton button = new FieldButton();
            button.setPreferredSize(BUTTON_SIZE);
            button.addActionListener(listener);
            buttons.add(button);
            mainPanel.add(button);
        }

        frame.add(controlPanel, BorderLayout.NORTH);
        frame.add(mainPanel);
        frame.pack();
        frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);

        setState(INITIAL_GAME);
        frame.setVisible(true);
        timer.start();
    }

    public static void main(String[] args) {
        new UltimatoeGui();
    }

    /** Store the new state of the game and display it. */
    @Override public void setState(final Ultimatoe game) {
        SwingUtilities.invokeLater(new Runnable() {
            @Override public void run() {
                setStateInternal(game);
                autoplay();
            }
        });
    }

    private void setStateInternal(Ultimatoe game) {
        final String newString = game.asString().replace("\n", "");
        Dout.a(game.asString());
        final String oldString = this.game==null ? null : this.game.toString().replace("\n", "");
        for (int i=0; i<buttons.size(); ++i) {
            final char c = newString.charAt(i);
            if (oldString != null && c == oldString.charAt(i)) continue;
            final JButton b = buttons.get(i);
            b.setForeground(Color.BLACK);
            b.setText("" + c);
            b.setEnabled(c == UltimatoeUtils.PLAYABLE);
            if (c == UltimatoeUtils.BORDER) b.setForeground(Color.DARK_GRAY);
        }

        this.game = game;
        frame.setTitle(title());
    }

    private String title() {
        String result;
        if (game.isFinished()) {
            result = "\"" + UltimatoeUtils.scoreToWinner(game.score()) + "\"" + " has won!";
        } else {
            result = "\"" + game.playerOnTurn().toString() + "\"" + " to go.";
        }
        result = "Turn " + game.turn() + ", " + result;
        result += " --- " + "Ultimatoe";
        return result;
    }

    private void autoplay() {
        if (game.isFinished()) return;
        autoplayInit();
        autoplayFinish();
    }

    /** Start the AI if needed. */
    private void autoplayInit() {
        if (swingWorker!=null) return;
        final GameActor<Ultimatoe> actor = actors[game.playerOnTurn().ordinal()];
        if (actor==null) return;
        swingWorker = apply(actor);
        swingWorker.execute();
    }

    /** Apply the move generated by AI if any. */
    private void autoplayFinish() {
        if (swingWorker==null) return;
        if (!swingWorker.isDone()) return;
        if (System.currentTimeMillis() < lastAutoplayMillis + autoplayDelayMillis) return;
        lastAutoplayMillis = System.currentTimeMillis();
        try {
            final String move = swingWorker.get();
            setState(game.play(move));
        } catch (InterruptedException | ExecutionException e) {
            e.printStackTrace(); //TODO
        } finally {
            swingWorker = null;
        }
    }

    private SwingWorker<String, Void> apply(final GameActor<Ultimatoe> actor) {
        return new SwingWorker<String, Void>() {
            @Override protected String doInBackground() throws Exception {
                return actor.selectMove(game);
            }
        };
    }

    private static final int N_OF_GUI_FIELDS = 11;
    private static final Dimension BUTTON_SIZE = new Dimension(50, 50);
    private static final int MAX_AUTOPLAY_DELAY_MILLIS = 2000;
    private static final double SPEEDUP_FACTOR = 1.2;

    private static final Ultimatoe INITIAL_GAME = Ultimatoe.INITIAL_GAME;

    private final JFrame frame = new JFrame();
    private final JPanel controlPanel = new JPanel();
    private final JPanel mainPanel = new JPanel();
    private final FieldListener listener = new FieldListener();
    private final List<JButton> buttons = Lists.newArrayList();

    private final Timer timer = new Timer(10, new ActionListener() {
        @Override public void actionPerformed(ActionEvent e) {
            autoplay();
        }
    });
    private long lastAutoplayMillis;
    private long autoplayDelayMillis;

    @NonNull private Ultimatoe game;

    /** The worker currently running the AI computing the move, or null. */
    @Nullable private SwingWorker<String, Void> swingWorker;

    @SuppressWarnings("unchecked")
    private final GameActor<Ultimatoe>[] actors = (GameActor<Ultimatoe>[]) new GameActor<?>[2];
}

I'd like to make the code better, but the price (length and effort) should be rather low as for me the interesting part is the AI.


There's a followup question showing the game engine.

\$\endgroup\$
6
+100
\$\begingroup\$

Normally, X is the player to go first in Tic-Tac-Toe games. In your implementation, O goes first. This is a bit unusual.


A lot of your interfaces are using generics, but is that really necessary? I find that sometimes adding generics for these kinds of games is a case of YAGNI (You Ain't Gonna Need It). Will you ever be passing another type as the generic parameter?


You seem to rely quite a lot on String objects (in play(String move) for example). A problem with this is that you only want a very very small subset of all possible strings. It is very hard to document how the String should be formatted to be accepted. I believe that it would be a good idea to use a custom GameMove class for this, that will make the usage so much clearer.


In your FieldListener you do a bunch of mathematics with the x and y pos to retrieve the actual button that was clicked. I am not a fan of this. You're using a FieldButton class, you might as well give that a x and y property and then you can retrieve the x and y much easier.

Alternatively, you could create a specific instance of an ActionListener to it and pass x and y values along there.

mainPanel.setLayout(new GridLayout(N_OF_GUI_FIELDS, N_OF_GUI_FIELDS));
for (int i=0; i<N_OF_GUI_FIELDS*N_OF_GUI_FIELDS; ++i) {
    final FieldButton button = new FieldButton();
    button.setPreferredSize(BUTTON_SIZE);
    button.addActionListener(listener);
    buttons.add(button);
    mainPanel.add(button);
}

Here you could easily do that mathematics and call a method that creates a specific listener for that specific button, with its associated x and y values.

You might want to read some previous answers of mine about how to create multiple instances of an ActionListener that shares code.


Speaking of x and y values, I noticed that you don't really use the fact that this game is two-dimensional. Most loops that loop over all buttons is only using one variable, i. I'd recommend making more use of x and y.


Your GUI does not allow switching which AI controls which player at runtime. As you work on the AI aspect, I expect there to be more than two AIs, and the more you add the more of a mess you will have. Deal with this potential mess as soon as possible. Implementing this part in a clean way will allow players to play a bit against your Random AI, a bit against your MonteCarlo AI, and maybe even ask an AI for advice (without having them making a move). That last part is achievable in your code without too much effort as you have the GameActor return a move, instead of making the move (this is good).


boolean isX = b.getText().endsWith("X")

Not a fan of that. I'd perhaps create two instances of ActionListeners here as well.


for (final String s : "auto-X auto-O".split(" ")) {

Looping over an array of String by creating a string and splitting it is.... not ideal.

You could do:

for (final String s : new String[]{ "auto-X", "auto-O" }) {

Or you could extract a method and call it twice.


Your setStateInternal method reads the String representation of a board, and then transforms it into representations for the buttons. Why transform a 2d array to a String only to transform it back into a 2d representation again? (in the form of a value for each button). I'd skip this String entirely here and read the values directly from the game model


Your apply method would be much better named as createWorkerForActor or similar.


I recommend adhering to the Java conventions when it comes to where to place constants and fields. (At the top of the file)

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.