Tic Tac Towards FX

I'm planning on taking on this community challenge. However, before my tics get tactical, I'd appreciate some feedback on the simpler implementation.

Screenshots:



Factoring in all the logic/edge conditionals was unexpectedly challenging and leaves me wondering if:

• This is done efficiently? Get the feeling that some of this is done in a roundabout way.

• Does it break any conventions?

• How are the variable and method names? I always particularly welcome suggestions on this names are hard.

• A few of my variables are static, I'd like to know if any are when they don't necessarily have to/shouldn't be.

-
The location of variables. I know this can be opinion based, but I always have several in mind and I can't know which is more understandable to someone else.

I'm planning on refactoring most of the main class as a board class for the challenge, so only take this in terms of Tic-Tac-Toe.

TicTacToeSquare

import javafx.scene.control.Button;

public class TicTacToeSquare {
    private boolean filled;
    static boolean turnTracker;
    private Button square = new Button();
    private final int SQUARE_SIZE = 100;

    TicTacToeSquare() {
        square.setMinHeight(SQUARE_SIZE);
        square.setMinWidth(SQUARE_SIZE);
        square.setOnAction(e -> {
            if (square.getText().isEmpty()) {
                square.setText(turnTracker ? "O" : "X");
                square.setStyle(
                    turnTracker ? "-fx-text-fill: gold;" : "-fx-text-fill: darkred;");
                filled = true;
                turnTracker = turnTracker ? false : true;
                TicTacToe.evaluateBoard();
            }
        });
    }

    public Button button() {
        return square;
    }

    public boolean equals(TicTacToeSquare target) {
        return filled && square.getText().equals(target.button().getText());
    }

    public void clear() {
        filled = false;
        square.setText("");
        square.setDisable(false);
    }
}

Tic

My main concern is missing separation of model and view. Using

enum Player {X, O}

the model could be something like

class Model {
    private final Player[] board = new Player[9];
    private int boardTracker;

    public Player playerOnTurn() {
        return boardTracker % 2 == 0 ? Player.X : Player.O;
    }

    public boolean isFilled(int index) {
        return board[index] != null;
    }

    boolean playOn(int index) {
        if (board[index] != null) {
            return false;
        }
        board[index] = playerOnTurn();
    }

    ... replaces evaluateBoard
    @Nullable public Player getWinner() {
        ...
    }

   ... all methods necessary for evaluation
   ... NO GUI HERE!
}

and the GUI would just draw it.
TicTacToeSquare

static boolean turnTracker;

That's ugly and prevents you from creating multiple boards. And multiple boards is exactly what you need for the challenge. There's a simple way how to get rid of all static variables:

private final boolean turnTracker[];

TicTacToeSquare(boolean turnTracker[]) {
     this.turnTracker = turnTracker;
     ...
}

and use turnTracker[0] instead of turnTracker. Create a single instance (later: a single instance per board) and pass it to the constructor. You may argue that using new boolean[1] is ugly like hell and you're right. It's just a poor man's MutableBoolean (AtomicBoolean would do, too). The good part is the general principle called Dependency Injection: Create your instances so that they get everything they need.

The ugly part goes away when you create a model. The idea stays the same.

private Button square = new Button();

Shouldn't this be final? I configured my eclipse to always add the modified to all fields which don't get changed. It makes it clearer (and also helps making classes thread-safe).

Don't call you Button square. The whole thing is TicTacToeSquare and the button should be simply button. Just like you call it in this getter

public Button button() {
    return square;
}

In general: Either is "button" the better name, or "square" is. Without a good reason you should use the same name for the field and for the getter (apart from a "get" prefix; that's another story).

public boolean equals(TicTacToeSquare target) {
    return filled && square.getText().equals(target.button().getText());
}

Please don't. Whatever this method does, it should in no case be called equals. It's just too confusing.

turnTracker = turnTracker ? false : true;

Too complicated since ! has been invented.
TicTacToe

private static TicTacToeSquare[] board = new TicTacToeSquare[9];
private static int boardTracker;

As already said, this two together should be simply

private final Model model = new Model();

Later you'll create an UltimateModel containing 9 Models and pass one of them to the constructor.

private static StringProperty xPlayer = new SimpleStringProperty("X player");

Assuming this is a constant, static is fine. A constant should be static final.

public static void evaluateBoard() {
    for (int i = 0, j = 0; i < 3; i++) {
        // Horizontal
        if(checkSet(j++, j++, j++)) {
            return;
        }

Why not checkSet(3 i, 3 i + 1, 3 * i + 2)? What you did works in Java as it ensures operand evaluation order, but this doesn't make it less ugly.

Actually, such a method should always return a result. That's the more important thing: Compute something. The next part is use the computed thing. Doing both is a violation of Separation of Concerns and makes the code not usable for anything else, including the challenge.
Answers

This is done efficiently? Get the feeling that some of this is done in a roundabout way.

It doesn't look bad, but I don't know what kind of efficiency you mean. It's not overcomplicated and not too long.

Concerning speed, fooling around with GUI (e.g. square.getText().equals(target.button().getText())) is surely slower than board[i] == board[j], but does it matter?

Does it break any conventions?

For sure. Every program does. :D There are at least two spacing issues:

newGameItem.setOnAction( e -> newGame());

and

if(checkSet(0, 4, 8) || checkSet(2, 4, 6)) {

How are the variable and method names?

Not really bad, but e.g. checkSet doesn't say what it does. Something starting with "check" could e.g. throw an exception. Maybe something like isSameColorSet. The name equals is terrible as it has already a meaning. Moreover, it's only slightly related equality.

A few of my variables are static, I'd like to know if any are when they don't necessarily have to/shouldn't be.

Static variables are evil (constants are fine). Unnecessary evil.

The location of variables. I know this can be opinion based, but I always have several in mind and I can't know which is more understandable to someone else.

Ideally, use just a single model object in eac

StackExchange Code Review Q#95706, answer score: 5