Tic Tac Towards FX

Question

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:

1. This is done efficiently? Get the feeling that some of this is done in a roundabout way.
2. Does it break any conventions?
3. How are the variable and method names? I always particularly welcome suggestions on this names are hard.
4. 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.

5. 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);
    }
}

TicTacToe

import javafx.application.Application;
import javafx.application.Platform;
import javafx.geometry.Insets;
import javafx.geometry.Pos;
import javafx.beans.binding.Bindings;
import javafx.beans.property.IntegerProperty;
import javafx.beans.property.SimpleIntegerProperty;
import javafx.beans.property.StringProperty;
import javafx.beans.property.SimpleStringProperty;
import javafx.scene.Scene;
import javafx.scene.text.Text;
import javafx.scene.control.Button;
import javafx.scene.control.Label;
import javafx.scene.control.TextField;
import javafx.scene.control.Menu;
import javafx.scene.control.MenuBar;
import javafx.scene.control.MenuItem;
import javafx.scene.input.KeyCode;
import javafx.scene.input.KeyCodeCombination;
import javafx.scene.input.KeyCombination;
import javafx.scene.layout.HBox;
import javafx.scene.layout.VBox;
import javafx.scene.layout.BorderPane;
import javafx.scene.layout.GridPane;
import javafx.stage.Stage;

public class TicTacToe extends Application {
    private static TicTacToeSquare[] board = new TicTacToeSquare[9];
    private static int boardTracker;
    private static StringProperty xPlayer = new SimpleStringProperty("X player");
    private static StringProperty oPlayer = new SimpleStringProperty("O player");
    private static IntegerProperty xScore = new SimpleIntegerProperty(0);
    private static IntegerProperty oScore = new SimpleIntegerProperty(0);
    private static IntegerProperty tieScore = new SimpleIntegerProperty(0);
    private static boolean scoreDisplayed;

    public static void main(String[] args) {
        launch(args);
    }

    @Override
    public void start(Stage stage) {
        BorderPane root = new BorderPane();

        MenuItem newGameItem = new MenuItem("_New Game");
        newGameItem.setAccelerator(
            new KeyCodeCombination(KeyCode.N, KeyCombination.SHORTCUT_DOWN));
        newGameItem.setOnAction( e -> newGame());

        MenuItem exitItem = new MenuItem("E_xit");
        exitItem.setOnAction(e -> Platform.exit());

        Menu gameMenu = new Menu("_Game");
        gameMenu.getItems().addAll(
            newGameItem,
            exitItem
        );

        MenuItem addItem = new MenuItem("_Add player name(s)");
        addItem.setAccelerator(
            new KeyCodeCombination(KeyCode.A, KeyCombination.SHORTCUT_DOWN));
        addItem.setOnAction(e -> addName());

        Text xText = new Text();
        xText.textProperty().bind(
            Bindings.concat(xPlayer).concat(" wins ").concat(xScore.asString()));

        Text oText = new Text();
        oText.textProperty().bind(
            Bindings.concat(oPlayer).concat(" wins ").concat(oScore.asString()));

        Text tieText = new Text();
        tieText.textProperty().bind(
            Bindings.concat("Ties: ").concat(tieScore.asString()));

        VBox scoreLayout = new VBox(5);
        scoreLayout.getChildren().addAll(xText, oText, tieText);
        scoreLayout.setPadding(new Insets(2));
        scoreLayout.setAlignment(Pos.CENTER);

        MenuItem trackItem = new MenuItem("_Toggle score display");
        trackItem.setAccelerator(
            new KeyCodeCombination(KeyCode.T, KeyCombination.SHORTCUT_DOWN));
        trackItem.setOnAction(e -> {
            if (!scoreDisplayed) {
                root.setRight(scoreLayout);
                scoreDisplayed = true;
                stage.sizeToScene();
            } else {
                root.setRight(null);
                scoreDisplayed = false;
                stage.sizeToScene();
            }
        });

        MenuItem resetItem = new MenuItem("_Reset score");
        resetItem.setAccelerator(
            new KeyCodeCombination(KeyCode.R, KeyCombination.SHORTCUT_DOWN));
        resetItem.setOnAction( e -> {
            xScore.set(0);
            oScore.set(0);
            tieScore.set(0);
        });

        Menu scoreMenu = new Menu("_Score");
        scoreMenu.getItems().addAll(
            addItem,
            trackItem,
            resetItem
        );

        activateMnemonics(
            gameMenu,
            newGameItem,
            exitItem,
            scoreMenu,
            addItem,
            trackItem,
            resetItem
        );

        MenuBar menuBar = new MenuBar();
        menuBar.getMenus().addAll(gameMenu, scoreMenu);
        root.setTop(menuBar);

        GridPane layout = new GridPane();
        for (int i = 0; i < board.length; i++) {
            board[i] = new TicTacToeSquare();
            layout.add(board[i].button(), i / 3, i % 3);
        }
        root.setCenter(layout);     

        stage.setScene(new Scene(root));
        stage.setTitle("Tic Tac Toe by Legato");
        stage.show();
    }

    public static void evaluateBoard() {
        for (int i = 0, j = 0; i < 3; i++) {
            // Horizontal
            if(checkSet(j++, j++, j++)) {
                return;
            }
            // Vertical
            if(checkSet(i, i + 3, i + 6)) {
                return;
            }
        }
        // Diagonal
        if(checkSet(0, 4, 8) || checkSet(2, 4, 6)) {
            return;
        }

        if (boardTracker == 8) {
            gameEndPrompt("It's a tie!");
            tieScore.setValue(tieScore.getValue() + 1);
            return;
        }

        boardTracker++;
    }

    private static boolean checkSet(int square1, int square2, int square3) {
        if (boardTracker >= 4) {
            if (board[square1].equals(board[square2]) 
            && board[square2].equals(board[square3])) {
                gameEndPrompt(
                    checkWinner(board[square1].button().getText()) + " wins!");
                return true;
            }
        }
        return false;
    }

    private static void gameEndPrompt(String message) {
        endGame();

        Stage stage = new Stage();
        Label label = new Label(message);
        label.setStyle("-fx-font-weight: bold;");

        Button reset = new Button("New Game");
        reset.setOnAction(e -> {
            stage.close();
            newGame();
        });
        reset.setDefaultButton(true);

        HBox layout = new HBox(5);
        layout.getChildren().addAll(label, reset);
        layout.setAlignment(Pos.CENTER);

        stage.setScene(new Scene(layout));
        stage.sizeToScene();
        stage.show();
    }

    private static void reset(TicTacToeSquare[] board) {
        for (int i = 0; i < board.length; i++) {
            board[i].clear();
        }
    }

    private static void endGame() {
        for (int i = 0; i < board.length; i++) {
            board[i].button().setDisable(true);
        }
    }

    private void activateMnemonics(MenuItem... items) {
        for (MenuItem item : items) {
            item.setMnemonicParsing(true);
        }
    }

    private static void newGame() {
        boardTracker = 0;
        reset(board);
    }

    private static void addName() {
        Stage stage = new Stage();

        Label xName = new Label("Enter X name");
        GridPane.setConstraints(xName, 0, 0);
        TextField xPlayerField = new TextField();
        GridPane.setConstraints(xPlayerField, 1, 0);

        Label oName = new Label("Enter O name");
        GridPane.setConstraints(oName, 0, 1);
        TextField oPlayerField = new TextField();
        GridPane.setConstraints(oPlayerField, 1, 1);

        Button submit = new Button("Submit");
        submit.setOnAction(e -> {
            String xString = xPlayerField.getText();
            String oString = oPlayerField.getText();
            if (!xString.replaceAll("[^a-zA-Z]", "").isEmpty()) {
                xPlayer.setValue(xString);
            }
            if (!oString.replaceAll("[^a-zA-Z]", "").isEmpty()) {
                oPlayer.setValue(oString);
            }
            stage.close();
        });
        submit.setDefaultButton(true);
        GridPane.setConstraints(submit, 0, 2);

        GridPane layout = new GridPane();
        layout.getChildren().addAll(
            xName,
            xPlayerField,
            oName,
            oPlayerField,
            submit
        );

        stage.setScene(new Scene(layout));
        stage.show();
    }

    private static String checkWinner(String winner) {
        if (winner.equals("X")) {
            xScore.setValue(xScore.getValue() + 1);
            return xPlayer.getValue();
        } else {
            oScore.setValue(oScore.getValue() + 1);
            return oPlayer.getValue();
        }
    }
}

If you have JRE 8, you may download and run the jar. For those interested the Github repository for this project is here.

Answer 1

Don't make classes into implicit objects

    static boolean turnTracker;

I don't think that this should be a static variable on TicTacToeSquare. It would make more sense for this to be a method on TicTacToe, as TicTacToe should know whose turn it is.

This is an example of the same anti-pattern as a Singleton. It essentially limits you to only having one game per application. However, you can an arbitrary number of games if you make this a method on the game object rather than the square class.

A second problem with this code is that it is not obvious what values of false and true mean without reading the code for context. This makes it easy to forget that true means it is currently O's turn. If you make a method called something like isOTurn() or isTurnO(), then that would be more obvious.

And of course, this limits you to just two sides. If you later get asked to make a three player version, you can't without rewriting code in many places.

                square.setText(turnTracker ? "O" : "X");
                square.setStyle(
                    turnTracker ? "-fx-text-fill: gold;" : "-fx-text-fill: darkred;");
                filled = true;
                turnTracker = turnTracker ? false : true;
                TicTacToe.evaluateBoard();

Similarly in this section, you could rewrite it

                player = game.getCurrentPlayer();
                square.setText(player.getText());
                square.setStyle(player.getStyle());
                filled = true;
                game.endTurn();

This both allows multiple games (by accessing an instance of TicTacToe rather than the class) and it abstracts out the current player in a way that keeps it extensible. Note that game.endTurn() does two things that used to be done in this method. It will change the current player and trigger the board evaluation. It can also update the turn count, which you currently do as part of the board evaluation. That might be confusing if someone evaluated the board at a time other than the end of a turn.

Note that you could also get rid of the filled variable and instead check that player is not null to get the same information.

Expressive variable names

    private static int boardTracker;

I already touched on this, but it's not obvious to me what this is. Looking at your code, perhaps

    private int turnCount = 0;

Now I know how many turns have been finished.

Note that I also got rid of the static keyword. This will allow you to track multiple games at once if you want. Or you can destroy the object at the end of a game and create a new one for the next game.

Avoid magic numbers

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

The 9 there should really be a constant, as you will reference it later.

        if (boardTracker == 8) {

If you always increment your turn counter before evaluating the board, you can use the same constant in this expression.

Again, that would also allow you to call this method at times other than the end of a turn. Currently if you called this method at the beginning of the ninth turn, it would end the game.

Answer 2

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 each graphic class, add graphic stuff at will.