Callback-oriented Tic-Tac-Toe

Question

I have written a simple implementation of the game Tic-Tac-Toe designed to allow "pluggable" player control, meaning I can easily swap out the controlling logic for players without modifying the game itself.

I have a few issues with my existing code:

1. I don't like the while loop for processing, however I find making move recursive (next calls the agent which calls move which calls next and so on) rather confusing. It also breaks the logic checking at the end of move, as as the "recursive stack" unwinds the logic runs and the result methods of the agent are called repeatedly. I'm not sure what the best way to fix this is; I see the merits of having the while loop (allows me to easily control the timing and steps etc), but I also see the merits of the recursive method as it means I can just call engine.next() once and it would run all of the logic.

2. I don't like the "magic" going on in getWinValues. I understand why it works but I don't like the looks of the "magic" size -y - 1. There probably isn't much I can do about this.

3. I think the toString method could be improved on the board class, as it seems overly verbose for what it is doing.

Result.java

public enum Result {
    LOSE,
    DRAW,
    WIN;
}

Player.java

public enum Player {
    X(0, 1),
    O(1, 0);

    private int id;
    private int opponentId;

    Player(int id, int opponentId) {
        this.id = id;
        this.opponentId = opponentId;
    }

    public Player getOpponent() {
        for (Player player : values()) {
            if (player.id == opponentId) {
                return player;
            }
        }

        return null;
    }
}

Board.java

public class Board {
    private int size;
    private Player[][] board;

    public Board(int size) {
        this.size = size;

        board = new Player[size][size];
    }

    public Board(Board source) {
        this(source.size);

        for (int y = 0; y < size; y++) {
            System.arraycopy(source.board[y], 0, board[y], 0, size);
        }
    }

    public int getSize() {
        return size;
    }

    private int[][] getCellValues(int size) {
        int[][] cells = new int[size][size];
        int cellValue = 1;

        for (int y = 0; y < size; y++) {
            for (int x = 0; x < size; x++) {
                cells[y][x] = cellValue;

                cellValue *= 2;
            }
        }

        return cells;
    }

    private int[] getWinValues(int size) {
        int winCount = (size * 2) + 2;
        int[] wins = new int[winCount];
        int[][] cellValues = getCellValues(size);

        for (int y = 0; y < size; y++) {
            wins[winCount - 2] += cellValues[y][y];
            wins[winCount - 1] += cellValues[y][size - y - 1];

            for (int x = 0; x < size; x++) {
                wins[y] += cellValues[y][x];
                wins[y + size] += cellValues[x][y];
            }
        }

        return wins;
    }

    private int getPlayerValue(Player player) {
        int value = 0;
        int[][] cellValues = getCellValues(size);

        for (int y = 0; y < size; y++) {
            for (int x = 0; x < size; x++) {
                if (board[y][x] == player) {
                    value += cellValues[y][x];
                }
            }
        }

        return value;
    }

    public boolean isWin(Player player) {
        int[] winValues = getWinValues(size);
        int playerValue = getPlayerValue(player);

        for (int winValue : winValues) {
            if ((playerValue & winValue) == winValue) {
                return true;
            }
        }

        return false;
    }

    public boolean isFull() {
        boolean full = true;

        for (Player[] row : board) {
            for (Player player : row) {
                full &= player != null;
            }
        }

        return full;
    }

    public Player get(int x, int y) {
        if (x < 0 || y < 0 || x >= size || y >= size) {
            return null;
        }

        return board[y][x];
    }

    public boolean set(int x, int y, Player player) {
        if (x < 0 || y < 0 || x >= size || y >= size) {
            return false;
        }

        board[y][x] = player;

        return true;
    }

    public Board copy() {
        return new Board(this);
    }

    @Override
    public String toString() {
        StringBuilder builder = new StringBuilder();

        for (int y = 0; y < size; y++) {
            StringBuilder rowBuilder = new StringBuilder();

            for (int x = 0; x < size; x++) {
                Player player = board[y][x];
                String playerString = player == null ? " " : player.name();

                rowBuilder.append(' ').append(playerString).append(' ');

                if (x <= size - 2) {
                    rowBuilder.append('|');
                } else {
                    rowBuilder.append('\n');
                }
            }

            builder.append(rowBuilder.toString());

            if (y <= size - 2) {
                for (int x = 0; x < rowBuilder.length(); x++) {
                    builder.append('-');
                }

                builder.append('\n');
            }
        }

        return builder.toString();
    }
}

Engine.java

public class Engine {
    private Board board;

    private Agent playerX;
    private Agent playerO;

    private Player currentPlayer;

    public Engine(int size, Agent playerX, Agent playerO, Player currentPlayer) {
        board = new Board(size);

        this.playerX = playerX;
        this.playerO = playerO;

        this.currentPlayer = currentPlayer;
    }

    public Engine(int size, Agent playerX, Agent playerO) {
        this(size, playerX, playerO, Player.X);
    }

    public Board getBoard() {
        return board.copy();
    }

    public Result getResult(Player player) {
        boolean winSame = board.isWin(player);
        boolean winOpponent = board.isWin(player.getOpponent());

        if ((winSame && winOpponent) || board.isFull()) {
            return Result.DRAW;
        }

        return winSame ? Result.WIN : Result.LOSE;
    }

    public boolean isOver() {
        if (board.isFull()) {
            return true;
        }

        return getResult(Player.X) != Result.LOSE || getResult(Player.O) != Result.LOSE;
    }

    private Agent getAgent(Player player) {
        if (player == Player.X) {
            return playerX;
        } else if (player == Player.O) {
            return playerO;
        }

        return null;
    }

    public void next() {
        if (isOver()) {
            return;
        }

        Player player = currentPlayer;
        Player opponent = currentPlayer.getOpponent();

        currentPlayer = opponent;

        Agent agent = getAgent(player);

        if (agent == null) {
            return;
        }

        agent.makeMove(player, board.copy(), this);

        if (isOver()) {
            Result result = getResult(player);
            Agent otherAgent = getAgent(opponent);

            if (otherAgent == null) {
                return;
            }

            if (result == Result.WIN) {
                agent.winGame(player, board.copy(), this);
                otherAgent.loseGame(opponent, board.copy(), this);
            } else if (result == Result.DRAW) {
                agent.drawGame(player, board.copy(), this);
                otherAgent.drawGame(opponent, board.copy(), this);
            } else if (result == Result.LOSE) {
                agent.loseGame(player, board.copy(), this);
                otherAgent.winGame(opponent, board.copy(), this);
            }
        }
    }

    public boolean move(Player player, int x, int y) {
        if (isOver() || board.get(x, y) != null) {
            return false;
        }

        if (!board.set(x, y, player)) {
            return false;
        }

        return true;
    }
}

Agent.java

public interface Agent {
    void makeMove(Player player, Board board, Engine engine);
    void winGame(Player player, Board board, Engine engine);
    void drawGame(Player player, Board board, Engine engine);
    void loseGame(Player player, Board board, Engine engine);
}

Main.java

public class Main {
    private static class RandomAgent implements Agent {

        private static int random(Board board) {
            return (int) Math.floor(Math.random() * board.getSize());
        }

        @Override
        public void makeMove(Player player, Board board, Engine engine) {
            System.out.println("makeMove(" + player + ", board, engine)");

            boolean success;

            do {
                success = engine.move(player, random(board), random(board));
            } while (!success);
        }

        @Override
        public void winGame(Player player, Board board, Engine engine) {
            System.out.println("winGame(" + player + ", board, engine)");
        }

        @Override
        public void drawGame(Player player, Board board, Engine engine) {
            System.out.println("drawGame(" + player + ", board, engine)");
        }

        @Override
        public void loseGame(Player player, Board board, Engine engine) {
            System.out.println("loseGame(" + player + ", board, engine)");
        }
    }

    public static void main(String[] args) {
        Engine engine = new Engine(3, new RandomAgent(), new RandomAgent());

        while (!engine.isOver()) {
            engine.next();
        }

        System.out.println(engine.getBoard());
    }
}

And a quick example of the output you should expect from running the code:

makeMove(X, board, engine)
makeMove(O, board, engine)
makeMove(X, board, engine)
makeMove(O, board, engine)
makeMove(X, board, engine)
makeMove(O, board, engine)
makeMove(X, board, engine)
winGame(X, board, engine)
loseGame(O, board, engine)
 X | O |   
-----------
 O | X |   
-----------
 O | X | X

Logic Issue

So I noticed a logic issue after posting, which I will not correct in the main code as per this post.

The issue is that the getResult method treats a full board as a draw, even if one of the players has won.

Example:

 X | X | X
----------- 
 O | O | X 
-----------
 X | O | O

getResult(Player.X) -> Result.DRAW

It can be corrected using by replacing the draw section of getResult with this:

if ((winSame && winOpponent) || (!winSame && !winOpponent && board.isFull())) {
    return Result.DRAW;
}

Follow-up

I have posted a follow-up to this question implementing some of the suggestions made by @mdfst13 and @janos, which can be found here.

Answer 1

Take game logic out of main

I don't like the while loop for processing, however I find making move recursive (next calls the agent which calls move which calls next and so on) rather confusing. It also breaks the logic checking at the end of move, as as the "recursive stack" unwinds the logic runs and the result methods of the agent are called repeatedly. I'm not sure what the best way to fix this is; I see the merits of having the while loop (allows me to easily control the timing and steps etc), but I also see the merits of the recursive method as it means I can just call engine.next() once and it would run all of the logic.

How about a third alternative? Rather than putting the while loop in main, put it in a new method run on Engine. So rather than

        while (!engine.isOver()) {
            engine.next();
        }

You'd just say

        engine.run();

Then in that method you could just say

        while (!isOver()) {
            next();
        }

Or you could get rid of isOver and put that logic in the loop in run. You also should be able to do without checking isOver in next. The part at the beginning is unnecessary if you always call it from run. The part at the end can be put outside the while loop in run. Or inside the loop:

         while (!board.full()) {
             Agent agent = getAgent(player);
             agent.makeMove(currentPlayer, board.copy(), this)

             if (board.isWin(currentPlayer)) {
                 Player opponent = currentPlayer.getOpponent();
                 agent.winGame(player, board.copy(), this);
                 getAgent(opponent).loseGame(opponent, board.copy(), this);
                 return;
             }

             currentPlayer = currentPlayer.getOpponent();
         }

         Player opponent = currentPlayer.getOpponent();
         getAgent(currentPlayer).drawGame(currentPlayer, board.copy(), this);
         getAgent(opponent).drawGame(opponent, board.copy(), this);

Now you don't check multiple things just to get one. You make a move and then check for a win by that player. If the board is full without a win, you fall out and declare a draw. You don't need to check for a loss, as you can only lose after the other player moves.

You get rid of both isOver and next. This removes duplicated logic, as you had to make those more generic than they should need to be.

Don't handle the impossible

In next, you have

        if (agent == null) {
            return;
        }

But that should never happen. If it does happen, then it will keep happening (because you are looping without changing anything). So the program will just run forever.

It would be better if the program crashed in that situation. So just leave out these checks. You'll attempt to dereference the null and the program will crash. Then you can figure out why. As is, the code won't even tell you that it is broken.

Answer 2

Improving Player

It's a good practice to make final everything that you can, such as the id and opponentId fields.

However, on closer look, there's not much need for these fields at all. Since there are only two players, getOpponent can decide what to return with a simple condition: Am I X? If yes, the opponent is O, otherwise I'm O so the opponent is X.

Following this logic, Player can be simplified to this:

public enum Player {
    X,
    O;

    public Player getOpponent() {
        return this == X ? O : X;
    }
}

Modeling

It looks a bit unnatural that a Board stores its state in a Player[][]. In this Tic-Tac-Toe game, there are really only 2 players, not a NxN matrix of players.

For modeling the state of the board, a Cell[][] would be more natural, where Cell could be defined as:

public enum Cell {
    EMPTY,
    X,
    O
}

Formatting text

The toString() method is not a great place for sophisticated text formatting. I recommend moving such logic to a dedicated format method. The toString() method is designed for low-level diagnostic messages.