4
\$\begingroup\$

A followup to this question, thanks to @mdfst13 and @janos for the feedback.

I have made some changes to my code, mainly the following:

  1. Replaced the next method with a loop and run method, as suggested by @mdfst13. Also removed redundant "null" checks.

  2. Added a Cell enum as suggested by @janos, replacing the 2D array of players in the board class with a 2D array of cells.

  3. Moved complex logic out of Board#toString() and into it's own class.

  4. Created a controller class to allow cleaner interfacing from agents - this also prevents agents from making any "unauthorised" moves.

I no longer have any major issues with my code personally, however I would appreciate feedback on improving the code to be more robust and just generally "well written" - following the idioms of the language.

I know there are some "redundant" calls in the code (e.g. the controller copies the board even though the board getter in the engine copies it), however I believe they should remain as they provide clarity over the intent of the actual code.

One or two things I've noticed that could be improved:

  1. I'm not entirely sure on the way I've designed the board renderer - should it be a final class that cannot be instantiated? I'm not sure if there would be any advantages of making the static methods into instance methods and allowing the renderer to be instantiated, as there are no "settings" for the renderer and is no data for the renderer to store.

  2. Copied from my previous question:

    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.

Result.java

public enum Result {
    LOSE,
    DRAW,
    WIN;
}

Player.java

public enum Player {
    X,
    O;

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

Cell.java

public enum Cell {
    EMPTY(' ', null),
    X('X', Player.X),
    O('O', Player.O);

    private char value;
    private Player player;

    Cell(char value, Player player) {
        this.value = value;
        this.player = player;
    }

    public char getValue() {
        return value;
    }

    public Player getPlayer() {
        return player;
    }

    public static Cell getCell(Player player) {
        for (Cell cell : values()) {
            if (cell.getPlayer() == player) {
                return cell;
            }
        }

        return EMPTY;
    }
}

Board.java

import java.util.Arrays;

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

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

        board = new Cell[size][size];

        for (Cell[] row : board) {
            Arrays.fill(row, Cell.EMPTY);
        }
    }

    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].getPlayer() == 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 (Cell[] row : board) {
            for (Cell cell : row) {
                full &= cell != Cell.EMPTY;
            }
        }

        return full;
    }

    public Cell 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, Cell cell) {
        if (x < 0 || y < 0 || x >= size || y >= size) {
            return false;
        }

        board[y][x] = cell;

        return true;
    }

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

BoardRenderer.java

public final class BoardRenderer {
    private BoardRenderer() {

    }

    private static String getRow(Board board, int row) {
        StringBuilder builder = new StringBuilder();

        for (int x = 0; x < board.getSize(); x++) {
            Cell cell = board.get(x, row);

            if (x != 0) {
                builder.append('|');
            }

            builder.append(' ').append(cell.getValue()).append(' ');
        }

        return builder.toString();
    }

    public static String render(Board board) {
        int size = board.getSize();
        StringBuilder builder = new StringBuilder();

        for (int y = 0; y < size; y++) {
            String row = getRow(board, y);

            if (y != 0) {
                builder.append('\n');

                for (int x = 0; x < row.length(); x++) {
                    builder.append('-');
                }

                builder.append('\n');
            }

            builder.append(row);
        }

        return builder.toString();
    }
}

Engine.java

public class Engine {
    private Board board;

    private Agent playerX;
    private Agent playerO;

    private Player startingPlayer;

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

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

        this.startingPlayer = startingPlayer;
    }

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

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

    public boolean hasEnded() {
        return board.isFull() || board.isWin(startingPlayer) || board.isWin(startingPlayer.getOpponent());
    }

    public Result getResult(Player player) {
        if (!hasEnded()) {
            return null;
        }

        boolean winSame = board.isWin(player);
        boolean winOpponent = board.isWin(player.getOpponent());

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

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

    public Player getWinner() {
        if (!hasEnded()) {
            return null;
        }

        if (getResult(startingPlayer) == Result.WIN) {
            return startingPlayer;
        }

        Player opponent = startingPlayer.getOpponent();

        if (getResult(opponent) == Result.WIN) {
            return opponent;
        }

        return null;
    }

    public boolean isDraw() {
        return getResult(startingPlayer) == Result.DRAW && getResult(startingPlayer.getOpponent()) == Result.DRAW;
    }

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

        throw new UnsupportedOperationException("invalid player " + player);
    }

    public void run() {
        int moveCount = 0;
        int maxMoveCount = board.getSize() * board.getSize();

        Player player = startingPlayer;
        Agent playerAgent = getAgent(player);

        while (!board.isFull() && moveCount < maxMoveCount) {
            playerAgent.makeMove(player, new Controller(this, player));

            if (board.isWin(player)) {
                break;
            }

            moveCount++;

            player = player.getOpponent();
            playerAgent = getAgent(player);
        }

        Player opponent = player.getOpponent();
        Agent opponentAgent = getAgent(opponent);

        Result playerResult = getResult(player);
        Result opponentResult = getResult(opponent);

        playerAgent.gameEnded(player, playerResult);
        opponentAgent.gameEnded(opponent, opponentResult);
    }

    public boolean move(Player player, int x, int y) {
        return !hasEnded() && board.get(x, y) == Cell.EMPTY && board.set(x, y, Cell.getCell(player));
    }
}

Agent.java

public interface Agent {
    void makeMove(Player player, Controller controller);
    void gameEnded(Player player, Result result);
}

Controller.java

public class Controller {
    private Engine engine;
    private Player player;
    private boolean moved = false;

    public Controller(Engine engine, Player player) {
        this.engine = engine;
        this.player = player;
    }

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

    public boolean move(int x, int y) {
        if (moved) {
            throw new UnsupportedOperationException("controller already moved");
        }

        if (engine.move(player, x, y)) {
            moved = true;
        }

        return moved;
    }
}

Main.java

public class Main {
    private static class RandomAgent implements Agent {

        private static int random(Controller controller) {
            return (int) Math.floor(Math.random() * controller.getBoard().getSize());
        }

        @Override
        public void makeMove(Player player, Controller controller) {
            System.out.println("makeMove(player = " + player + ", controller)");

            boolean success;

            do {
                success = controller.move(random(controller), random(controller));
            } while (!success);
        }

        @Override
        public void gameEnded(Player player, Result result) {
            System.out.println("gameEnded(player = " + player + ", result = " + result + ")");
        }
    }

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

        long start = System.currentTimeMillis();

        engine.run();

        long end = System.currentTimeMillis();

        System.out.println(BoardRenderer.render(engine.getBoard()));
        System.out.println("The game took " + (end - start) + "ms");
        System.out.println("The winner was: " + (engine.isDraw() ? "Nobody" : engine.getWinner()));
    }
}

And some example output:

makeMove(player = X, controller)
makeMove(player = O, controller)
makeMove(player = X, controller)
makeMove(player = O, controller)
makeMove(player = X, controller)
makeMove(player = O, controller)
makeMove(player = X, controller)
makeMove(player = O, controller)
gameEnded(player = O, result = WIN)
gameEnded(player = X, result = LOSE)
 X | X | O 
-----------
   | X | O 
-----------
 X | O | O 
The game took 3ms
The winner was: O
\$\endgroup\$
1
\$\begingroup\$

Make final what you can

Make final what you can, for example the fields of Cell:

private final char value;
private final Player player;

And the fields of Board:

private final int size;
private final Cell[][] board;

And so on.

Refining Cell.getCell

Instead of looping over cell values, you could simplify as in Player:

public static Cell getCell(Player player) {
    return player == Player.X ? X : O;
}

Or you could take a more conservative approach:

  • Use an assert to document that the player parameter must never be null
  • Crash with a big bang if the player parameter has an unexpected value

Something like this:

public static Cell getCell(Player player) {
    assert player != null;

    if (player == Player.X) {
        return X;
    }
    if (player == Player.O) {
        return O;
    }
    throw new IllegalArgumentException("player must be either X or O");
}

A bit ugly fill + overwrite

It's a bit ugly that when the copy constructor calls the original, the cells are filled with empty, only to be overwritten immediately. It would be slightly better to not call the other constructor from the copy constructor.

Another readability improvement would be using the .clone() method of arrays instead of the tedious System.arraycopy(...).

public Board(Board source) {
    this.size = source.size;
    this.board = new Cell[size][];

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

Naming

The Board class contains the cells in a field named board. This is a bit confusing. It would be more natural to call the cells cells.

Repeated calculations

If I'm looking at it right, getCellValues and getWinValues will always return the same values. As these methods are called repeatedly, this is wasteful. It would be better to pre-calculate their returned values once, and simply reuse.

The size parameter of these methods also looks pointless.

Flag variables

Flag variables are often unnecessary and cause more problems than they solve. Consider full in Board.isFull:

public boolean isFull() {
    boolean full = true;

    for (Cell[] row : board) {
        for (Cell cell : row) {
            full &= cell != Cell.EMPTY;
        }
    }

    return full;
}

As soon as you find a non-empty cell, you could simply return. The result will be simpler, shorter, and faster too:

public boolean isFull() {
    for (Cell[] row : board) {
        for (Cell cell : row) {
            if (cell == Cell.EMPTY) {
                return false;
            }
        }
    }

    return true;
}

Prefer to fail with a big bang

The Board.get and Board.set methods have this check:

if (x < 0 || y < 0 || x >= size || y >= size) {

In case of such invalid x or y parameters, the methods return something, quietly ignoring the fact that something's awfully wrong in the caller.

In such situations, in case of non-sense input, instead of returning normally (also known as "garbage in, garbage out"), it's better to crash with a big bang, for example:

throw new IllegalArgumentException("invalid input: x and y must be within 0 and size");

Prefer one way to do things

Board has two ways to make a copy: using a copy constructor, and the copy method. This can cause problems:

  • Users of the class may question which method is preferred.
  • The more public methods, the more maintenance overhead.

If you have give one way to do something, you can preempt boring questions, and you have fewer methods to maintain.

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