Java Code Challenge: Tic-Tac-Toe Solution

Guys, you're awesome! My first coding challenge has been a major success and I hardly managed to read all of the solutions. For those of you who haven't seen the original challenge, the task was to implement a simple Tic-Tac-Toe game in the console, with the goal of delivering the best, clean design possible. Out of the stunning number of 1 submission — big thanks to Robert Brautigam — I have chosen a lucky one to analyze today.

Code Structure

The code structure of the lucky project looks like this:

We can easily infer a few things from the structure itself:

• It's a Tic-Tac-Toe board game

• There are two kinds of players

• Only square boards are supported

• The UI is in the console

That's a pretty expressive package structure. The View interface next to the game's first-class citizens like Game or Board is a little bit surprising to me, but we'll get to it in a moment.

Game.java

In general, the class looks pretty sleek. The main algorithm looks like this:

public void play() {
    while (!board.containsRow() && !board.isFull()) {
        nextPlayer().makeMove();
    }
}

Two players make moves until the board is full or... contains a row? I suppose this method was supposed to check that there are enough players marks in a row:

public void play() {
    while (board.marksInARow() < 3 && !board.isFull()) {
        nextPlayer().makeMove();
    }
}

The next player logic is based on an array of players and is a bit too "brilliant" to me:

private Player nextPlayer() {
    return players[(turn++) % 2];
}

Everybody knows what it's supposed to do and trusts the author that it works, but trying to understand it and reason about its correctness requires too much time and effort. To me, clean code should be obvious, not "brilliant":

private Player nextPlayer() {
    if (currentPlayer == player1) {
        return player2;
    } else {
        return player1;
    }
}

The class also contains two comments, which I, personally, absolutely hate. They add no value for the reader. In this case, one of them can be misleading and another one mislocated:

/**
 * Main logic of the Tic-Tac-Toe game. It is played on a Board by
 * two Players, who make their move in turns until the board contains
 * a full row, or the board is full.
 */

Since the author made the game support other boards than 3x3, we're talking about an m,n,k-game here, not necessarily Tic-Tac-Toe (it's a minor thing, but a game nerd like me does not forgive! :) ). The game shouldn't end when there is a full row; it ends when one player has taken a full row.

/**
 * Construct a game with the given board and players. Player 1 will start the game.
 */

I don't like this comment either. Why is the starting player in the documentation of the constructor, while the choice of the starting player is spread between play() and nextPlayer()? As the first part of the comment doesn't add much value, I'd delete it altogether.

You can see the full class here.

Board.java

The Board interface turned out pretty surprising for me. First, besides the two methods that we've already seen in action, it contains two other methods that return  Views:

/**
 * Get the view for this board for player 1.
 */
View getPlayer1View();

/**
 * Get the view for this board for player 2.
 */
View getPlayer2View();

Knowing the game's rules, I don't know can't there be a single view for both players. I'd just use different marks for each of the players.

Second, the nasty comments are there again. Come on! The naming is so good in this project that there's no need to duplicate things in the comments.

You can see the full interface here.

View.java

This interface name and purpose is still cryptic to me. The comment says that it represents a player's view of the board.

A view can tell you what cells it contains and draw itself on the UI, but not the UI you've seen in the code structure. It's an inner interface at the bottom of the View.

void draw(UI ui);

...

interface UI {
    /**
     * Draw the board. Board is a 2D array, with position (0,0) as first element, x axis
     * in the rows, y in columns.
     */
    void drawBoard(Mark[][] board);

    enum Mark {
        EMPTY, MINE, ENEMYS;
    }
}

That could explain why we have two different views for different players. There are no "absolute" marks like Xs and Os and instead, we're talking about my marks and my opponent's marks.

The View interface also has another inner interface named Cell. Again there are the unneeded comments for well-named methods, but also there are two other things worth mentioning.

First, there are two unused methods:

/**
 * @return True iff the cell is taken by the player using this view.
 */
boolean isMine();

/**
 * @return True iff the cell is taken by the opponent.
 */
boolean isEnemys();

Dead code is bad. It clutters things and there's almost no cost in removing it.

Second, it contains a particularly surprising mark() method:

/**
 * Own this cell. This can only be done if empty. It not, exception is thrown.
 */
void mark();

Does that mean that the player is making moves in the game by talking to Cells, which are parts of the View? And what's with the exception when the cell is not empty? Does the game blow up with an exception just because a player tried to make a bad move? (I tried to check but the game blows up with an NPE for me, just after it begins).

You can see the full interface here.

SquareBoard.java

This class basically controls the whole game using int logic. With just a look on the first few lines, you know that you never want to be tasked with fixing a bug in there.

private final int size;
// The board, 0 - Empty, (1) - Player1, (-1) - Player2
private final int[] board;
// The score (sum) of marks in given row (size horizontal rows, size vertical rows, 2 diagonal rows)
private final int[] boardScore;

public SquareBoard(int size) {
   this.size = size;
   this.board = new int[size*size];
   this.boardScore = new int[2 * size + 2];
}

Maybe it works, but again, it's too "brilliant" for me.

The class also contains two inner classes that implement  View  and View.Cell respectively. As we inferred from the previous interface, the moves in the game are done via a View.Cell.mark() method:

@Override
public void mark() {
    if (!isEmpty()) {
        throw new IllegalStateException("can not mark cell at " + getPosition() + ", it is already taken");
    }
    board[index] = identity;
    boardScore[x] += identity;
    boardScore[size + y] += identity;
    if (x == y) {
        boardScore[2 * size] += identity;
    }
    if (x + y == size - 1) {
        boardScore[2 * size + 1] += identity;
    }
}

It basically makes the View.Cell responsible for calculating some score, deciding if a move in the game is valid, user interaction (in the form of blowing off the application with an exception) and marking the cell as taken. Not bad for a method that takes 15 lines of code.

In my opinion, we should make the Game class more powerful instead:

public void play() {
    while (board.marksInARow() < 3 && !board.isFull()) {
        Position move = currentPlayer.makeMove();

        if (board.isFree(move)) {
            board.place(move, currentPlayer.getMark());
            currentPlayer = nextPlayer();
        } else {
            currentPlayer.invalidMove();
        }
    }
}

Summary

That's it. These were the most interesting classes in the project. If you want to see the rest, you can check out the full source code here. Although this article mostly focused on the things to improve, the code was pretty clean with a particularly good naming. Even though I didn't get the coding concept by just looking at the structure, the code was good enough for me to finally grasp how it works. On the negative side, I'd delete the comments that add no value and make the code more obvious. Last but not least, again, big thanks to Robert Brautigam for participating in the challenge!