Ultimatoe — 1. GUI and general game interfaces

For the community challenge, I wrote ultimatoe. For me, the most interesting part is clearly the AI, but what I did so far is just a stupid Monte Carlo tree search, which isn't ready for a review yet (but may come soon).

Before reviewing, please have a look at the README. The text is much shorter than the code.

I'd like you have a look mainly at the GUI. Especially tips making it better without much effort are welcome. It works by transforming game.asString() into graphical representation (i.e., it makes from the character 'X' a field containing "X", which is a funny (and probably stupid) idea. The string representation of the below board would be something like "XXX-X- \n --O O \n...".

I'm leaving out the imports as boring.

Game

```
/** An immutable representation of a game (state). */
@Immutable public interface Game> {
/**
* Return the list of all players, including dummies (like e.g., a player representing "no winner").
*
* The starting player is the first element, other real players follow, dummies are placed last.
*/
ImmutableList> players();

/** Return the zero-based turn (i.e., the number of moves already played). */
int turn();

/**
* Return the player to go, unless the game has already finished.
*
* In case {@link #isFinished()} returns true, the result is undefined.
*/
GamePlayer playerOnTurn();

/** Return the winner. If there's none, a special dummy gets returned. */
GamePlayer winner();

/**
* Return the score for the initial player (the bigger the better for them).
* The result lies between -1 and +1.
*
* Note that there's no heuristics involved, the score reflects the rules only.
*
* If applicable for the given game, the result is
* {@code +1.0}, when the {@link #winner()} is {@code players().get(0)}
* {@code -1.0}, when the {@link #winner()} is {@code players().get(1)}
* {@code 0.0}, otherwise.
*/
doub

Normally, X is the player to go first in Tic-Tac-Toe games. In your implementation, O goes first. This is a bit unusual.

A lot of your interfaces are using generics, but is that really necessary? I find that sometimes adding generics for these kinds of games is a case of YAGNI (You Ain't Gonna Need It). Will you ever be passing another type as the generic parameter?

You seem to rely quite a lot on String objects (in play(String move) for example). A problem with this is that you only want a very very small subset of all possible strings. It is very hard to document how the String should be formatted to be accepted. I believe that it would be a good idea to use a custom GameMove class for this, that will make the usage so much clearer.

In your FieldListener you do a bunch of mathematics with the x and y pos to retrieve the actual button that was clicked. I am not a fan of this. You're using a FieldButton class, you might as well give that a x and y property and then you can retrieve the x and y much easier.

Alternatively, you could create a specific instance of an ActionListener to it and pass x and y values along there.

mainPanel.setLayout(new GridLayout(N_OF_GUI_FIELDS, N_OF_GUI_FIELDS));
for (int i=0; i<N_OF_GUI_FIELDS*N_OF_GUI_FIELDS; ++i) {
    final FieldButton button = new FieldButton();
    button.setPreferredSize(BUTTON_SIZE);
    button.addActionListener(listener);
    buttons.add(button);
    mainPanel.add(button);
}

Here you could easily do that mathematics and call a method that creates a specific listener for that specific button, with its associated x and y values.

You might want to read some previous answers of mine about how to create multiple instances of an ActionListener that shares code.

Speaking of x and y values, I noticed that you don't really use the fact that this game is two-dimensional. Most loops that loop over all buttons is only using one variable, i. I'd recommend making more use of x and y.

Your GUI does not allow switching which AI controls which player at runtime. As you work on the AI aspect, I expect there to be more than two AIs, and the more you add the more of a mess you will have. Deal with this potential mess as soon as possible. Implementing this part in a clean way will allow players to play a bit against your Random AI, a bit against your MonteCarlo AI, and maybe even ask an AI for advice (without having them making a move). That last part is achievable in your code without too much effort as you have the GameActor return a move, instead of making the move (this is good).

boolean isX = b.getText().endsWith("X")

Not a fan of that. I'd perhaps create two instances of ActionListeners here as well.

for (final String s : "auto-X auto-O".split(" ")) {

Looping over an array of String by creating a string and splitting it is.... not ideal.

You could do:

for (final String s : new String[]{ "auto-X", "auto-O" }) {

Or you could extract a method and call it twice.

Your setStateInternal method reads the String representation of a board, and then transforms it into representations for the buttons. Why transform a 2d array to a String only to transform it back into a 2d representation again? (in the form of a value for each button). I'd skip this String entirely here and read the values directly from the game model

Your apply method would be much better named as createWorkerForActor or similar.

I recommend adhering to the Java conventions when it comes to where to place constants and fields. (At the top of the file)

StackExchange Code Review Q#96015, answer score: 6