Callback-oriented Tic-Tac-Toe

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:

-
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.

-
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.

-
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 >= size) {
r

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.

StackExchange Code Review Q#125349, answer score: 4