Ultimatoe — 2. The model

The game model (Ultimatoe) consists of 9 instances of Tic-Tac-Toe (UltimatoeBoard) and some auxiliary information. As the number of all reachable boards is bound by \$3^9 = 19683\$, they all are pre-generated, which allows fast move evaluation.

Before reviewing, please read the short text of the predecessor question and the README. It's just a few lines.

Since the predecessor question, the code has slightly changed, but the interfaces given there have not. So all the needed stuff is available on this site and additional information on github.

UltimatoeBoard

```
/**
* Representation of a 3x3 board of {@link Ultimatoe}.
*
* As there are just a few thousands reachable states, they all get pre-created with all necessary data
* in order to make moves as fast as possible.
*/
@Immutable final class UltimatoeBoard {
/**
* Create a new board described by the given fields and also recursively create
* all its ancestors unless found in the repository.
*
* @param fields the effectivelly immutable list containing state of the nine fields
* @param repository a map filled with already created boards
*/
private UltimatoeBoard(List fields, Map, UltimatoeBoard> repository) {
checkArgument(fields.size() == AREA);
this.fields = fields;
winner = computeWinner();
int possibilities = 0;
if (winner.isDummy()) {
for (final GamePlayer player : UltimatoeUtils.PLAYERS) {
if (player.isDummy()) continue;
for (int i=0; i, UltimatoeBoard> repository) {
final List childFields = Lists.newArrayList(this.fields);
childFields.set(index, player);
UltimatoeBoard result = repository.get(childFields);
if (result!=null) return result;
result = new UltimatoeBoard(childFields, repository);
repository.put(childFields, result);
return result;
}

/** Create the empty board. Must be called just once. */
private st

Spacing

Your code could use lots of both vertical and horizontal spacing.

You should use vertical spacing to group related lines of code together. It is much easier to read.

Horizontal spacing will also add readability at least if done where you assign or compare variables.

@Override public ImmutableBiMap children() {
    final ImmutableBiMap.Builder result = ImmutableBiMap.builder();
    for (int i=0; i<N_BOARDS; ++i) {
        if (!isPlayable(i)) continue;
        final UltimatoeBoard b = boards[i];
        for (int j=0; j<N_FIELDS_PER_BOARD; ++j) {
            if (!b.isPlayable(j)) continue;
            result.put(play(i, j), UltimatoeUtils.indexesToMoveString(i, j));
        }
    }
    return result.build();
}

is much more readable like so

@Override public ImmutableBiMap children() {

    final ImmutableBiMap.Builder result = ImmutableBiMap.builder();

    for (int i = 0; i < N_BOARDS; ++i) {

        if (!isPlayable(i)) { continue; }

        final UltimatoeBoard b = boards[i];

        for (int j = 0; j < N_FIELDS_PER_BOARD; ++j) {

            if (!b.isPlayable(j)) { continue; }

            result.put(play(i, j), UltimatoeUtils.indexesToMoveString(i, j));
        }
    }

    return result.build();
}

I guess you know my habit to place braces {} arround everything, so I did in this sample.

Constructor

The constructor should be used to construct the object with the minimum needed stuff. IMHO you are doing too much inside the constructor which should be extracted to a separate method.

private UltimatoeBoard(List fields, Map, UltimatoeBoard> repository) {
    checkArgument(fields.size() == AREA);
    this.fields = fields;
    winner = computeWinner();
    int possibilities = 0;
    if (winner.isDummy()) {
        for (final GamePlayer player : UltimatoeUtils.PLAYERS) {
            if (player.isDummy()) continue;
            for (int i=0; i<AREA; ++i) {
                if (!getPlayerOnField(i).isDummy()) continue;
                children[childIndex(i, player)] = getOrCreateChild(i, (UltimatoePlayer) player, repository);
                possibilities |= 1 << i;
            }
        }
    }
    this.possibilities = possibilities;
}

Let us create a method computePossibilities() to assign to the class variable possibilites like so

private int computePossibilities() {

    int possibilities = 0;

    if (!winner.isDummy()) { return possibilities; }

    for (final GamePlayer player : UltimatoeUtils.PLAYERS) {

        if (player.isDummy()) { continue; }

        for (int i = 0; i < AREA; ++i) {

            if (!getPlayerOnField(i).isDummy()) { continue; }

            children[childIndex(i, player)] = getOrCreateChild(i, (UltimatoePlayer) player, repository);
            possibilities |= 1 << i;
        }
    }

    return possibilities;
}

You may have noticed that I have inverted the condition of the first if condition, because this is horizontal space which should be avoided.

Now your constructor looks like so

private UltimatoeBoard(List fields, Map, UltimatoeBoard> repository) {
    checkArgument(fields.size() == AREA);

    this.fields = fields;
    this.winner = computeWinner();
    this.possibilities = computePossibilities();
}

I am not that sure about the name computePossibilities but I have nevertheless named it like this.

Ultimatoe

The constructor could get a little faster by storing calculated results which you calculate often

ToStringHelper(Ultimatoe game) {
        final char[][] result = new char[11][11];
        for (int i=0; i0) sb.append("\n");
            sb.append(result[i]);
        }
        toString = sb.toString();
    }

here in the most inner loop you are calculating 4y1, 4x1 and 3*y0 for each iteration. You should store the result in some variables and use them in the most inner loop.

ToStringHelper(Ultimatoe game) {
    final char[][] result = new char[11][11];
    for (int i = 0; i  0) {
            sb.append("\n");
        }
        sb.append(result[i]);
    }
    toString = sb.toString();
}

now about the StringBuilder part. If you don't initialize a StringBuilder object with a capacity it will have a default value of 16 and each time the length of the StringBuilder exceeds the capacity to 2 * capacity +2.

Here you already know the maximum capacity (11 * 11 + 10) the StringBuilder can reach so you should use this knowledge.

If you append result[0] before the loop and let the loop start at 1 you can omit the if in the loop.

```
ToStringHelper(Ultimatoe game) {
final char[][] result = new char[11][11];
for (int i = 0; i < 11; ++i) {
Arrays.fill(result[i], UltimatoeUtils.BORDER);
}

for (int y1 = 0; y1 < 3; ++y1) {

final int outerIndexY = 4 * y1;
final int majorIndexY = 3 * y1;

for (int x1 = 0; x1 < 3; ++x1) {

final int outerIndexX = 4 * x1;
final int majorInd

StackExchange Code Review Q#96591, answer score: 6