Extremely Object Oriented Tic-Tac-Toe in Java

Question

Please criticize the classes GameJudgeImpl and GameInfo so that I could make them clearer.

public interface GameJudge {
    public GameInfo gameInfo();
}

public enum GameResult {
    UNKNOWN, DRAW, PLAYER_WINS, OPPONENT_WINS
}

public enum Cell {
    EMPTY, PLAYER, OPPONENT
}

↓

public class GameInfo {
    private final GameResult gameResult;
    private final List<Matrix.Position> cellsOnFire;

    public static GameInfo unknownResult() {
        return new GameInfo(GameResult.UNKNOWN, new ArrayList<Matrix.Position>());
    }

    public static GameInfo drawResult() {
        return new GameInfo(GameResult.DRAW, new ArrayList<Matrix.Position>());
    }

    public GameInfo(GameResult gameResult, List<Matrix.Position> cellsOnFire) {
        this.gameResult = gameResult;
        this.cellsOnFire = cellsOnFire;
    }

    public GameResult gameResult() {
        return gameResult;
    }

    public List<Matrix.Position> cellsOnFire() {
        return cellsOnFire;
    }

    public boolean resultIsKnown() {
        return gameResult != GameResult.UNKNOWN;
    }
}

↓

public class GameJudgeImpl implements GameJudge {
    private final Matrix<Cell> gameBoard;
    private final int gameBoardDimension;

    public GameJudgeImpl(Matrix<Cell> gameBoard) {
        this.gameBoard = gameBoard;
        this.gameBoardDimension = gameBoard.rows;
    }

    @Override
    public GameInfo gameInfo() {
        GameInfo gameInfo = checkAllRowsColumns();
        if (gameInfo.resultIsKnown()) {
            return gameInfo;
        }
        gameInfo = checkDiagonals();
        if (gameInfo.resultIsKnown()) {
            return gameInfo;
        }
        if (gameBoard.contains(Cell.EMPTY)) {
            return GameInfo.unknownResult();
        }
        return GameInfo.drawResult();
    }

    private GameInfo checkAllRowsColumns() {
        for (int i = 0; i < gameBoardDimension; ++i) {
            GameInfo gameInfo = checkRowColumn(i);
            if (gameInfo.resultIsKnown()) {
                return gameInfo;
            }
        }
        return GameInfo.unknownResult();
    }

    private GameInfo checkRowColumn(int index) {
        GameInfo rowGameInfo = checkRow(index);
        if (rowGameInfo.resultIsKnown()) {
            return rowGameInfo;
        } else {
            return checkColumn(index);
        }
    }

    private GameInfo checkRow(int row) {
        return gameInfoByCellsPositions(cellsPositionsOnRow(row));
    }

    private List<Matrix.Position> cellsPositionsOnRow(int row) {
        List<Matrix.Position> cells = new ArrayList<Matrix.Position>(gameBoardDimension);
        for (int column = 0; column < gameBoardDimension; ++column) {
            cells.add(new Matrix.Position(row, column));
        }
        return cells;
    }

    private GameInfo gameInfoByCellsPositions(List<Matrix.Position> cellsPositions) {
        Matrix.Position firstCellOnLinePosition = cellsPositions.get(0);
        Cell firstCellOnLine = gameBoard.get(firstCellOnLinePosition);
        if (firstCellOnLine == Cell.EMPTY) {
            return GameInfo.unknownResult();
        }
        for (int i = 1; i < gameBoardDimension; ++i) {
            Matrix.Position currentPosition = cellsPositions.get(i);
            Cell currentCell = gameBoard.get(currentPosition);
            if (firstCellOnLine != currentCell) {
                return GameInfo.unknownResult();
            }
        }
        return new GameInfo(cellToResult(firstCellOnLine), cellsPositions);
    }

    private GameResult cellToResult(Cell cell) {
        if (cell == Cell.PLAYER) {
            return GameResult.PLAYER_WINS;
        } else if (cell == Cell.OPPONENT) {
            return GameResult.OPPONENT_WINS;
        }
        throw new IllegalArgumentException("Input cell must be not empty!");
    }

    private GameInfo checkColumn(int column) {
        return gameInfoByCellsPositions(cellsPositionsOnColumn(column));
    }

    private List<Matrix.Position> cellsPositionsOnColumn(int column) {
        List<Matrix.Position> cells = new ArrayList<Matrix.Position>(gameBoardDimension);
        for (int row = 0; row < gameBoardDimension; ++row) {
            cells.add(new Matrix.Position(row, column));
        }
        return cells;
    }

    private GameInfo checkDiagonals() {
        GameInfo leftUpperDiagonalGameInfo = checkLeftUpperDiagonal();
        if (leftUpperDiagonalGameInfo.resultIsKnown()) {
            return leftUpperDiagonalGameInfo;
        } else {
            return checkRightUpperDiagonal();
        }
    }

    private GameInfo checkLeftUpperDiagonal() {
        return gameInfoByCellsPositions(cellsPositionsOnLeftUpperDiagonal());
    }

    private List<Matrix.Position> cellsPositionsOnLeftUpperDiagonal() {
        List<Matrix.Position> positions = new ArrayList<Matrix.Position>(gameBoardDimension);
        for (int i = 0; i < gameBoardDimension; ++i) {
            positions.add(new Matrix.Position(i, i));
        }
        return positions;
    }

    private GameInfo checkRightUpperDiagonal() {
        return gameInfoByCellsPositions(cellsPositionsOnRightUpperDiagonal());
    }

    private List<Matrix.Position> cellsPositionsOnRightUpperDiagonal() {
        List<Matrix.Position> positions = new ArrayList<Matrix.Position>(gameBoardDimension);
        for (int i = 0; i < gameBoardDimension; ++i) {
            positions.add(new Matrix.Position(i, gameBoardDimension - i - 1));
        }
        return positions;
    }
}

Answer 1

1. Interface, GameJudge, doesn't seem to make sense.

   • the method gameInfo(), doesn't have an action verb so it's hard to determine what exactly it is doing. getGameInfo() perhaps?

   • An interface is a contract. So if another class were to implement this interface, would the return type, GameInfo, and the method gameInfo() make sense? i.e. for chess. If not, perhaps you'd want to rename your interface to something more specific to tic-tac-toe.

2. The Enum, GameResult, looks pretty good but needs some tweaking.

   • Since this is the result of the game, suggest that it only have 3 states: Player wins, Opponent wins, and Draw.

   • The value of Unknown leads me to believe that you will be using it to check the game's state. If that is the case, rename the class to GameState

3. The Enum, Cell, is fine. It will contain the state of the Cell.

---

A few things jump out of me are:

The use of static methods

• In general, static methods make your code harder to test. See (Mikso Hevery). Since you always want to have tests with your code, try to avoid static methods until you have a better grasp of when to use them and when not to. This will help you with your OO design skills.

• In this case, you're are using it to return a new instance (factory pattern) of GameInfo. The problem is that the static methods are in the same class you are returning. You would be better off creating a separate class that owns the responsibility of creating GameInfo objects.

• I would argue that for any single game, you should only have a single GameInfo object. So you probably don't even need that factory method since you can create a new object during the start of the game and then return the same one when the game is finished.

A large number of private variables

• When you see a single public method and a large number of private methods, it could be an indication of that class having too much responsibility. Investigate your private methods and see if you can find common patterns/usage. If so, you can move those methods to the new class so that it can handle the responsibility.

For example, you have a lot of methods that start with 'check'. Could you make a class called, BoardChecker?

Keep repeating the process and breakdown your class into other classes when applicable.

Too many return statements

A large number of return statements can be confusing (and hard to debug if the method is large). Try to only have one or two return statements.

See method, gameInfo()

Naming Convention

What is Matrix.Position anyway? This seems very strange to me. Please ollow Java naming conventions for your class names.