JavaFX Tic-Tac-Toe Game

I programmed a Tic Tac Toe game using JavaFX, and I'm looking for a code review of it to improve my skills and practices in Java. It would highly be appreciated if you reviewers emphasize on these points specifically:

• Bad practices which I am following

• Inefficiencies and how would I rectify them

TicTacToe.java:

package tictactoe;

import javafx.application.Application;
import javafx.fxml.FXMLLoader;
import javafx.scene.Parent;
import javafx.scene.Scene;
import javafx.stage.Stage;

public class TicTacToe extends Application {

    @Override
    public void start(Stage stage) throws Exception {
        Parent root = FXMLLoader.load(getClass().getResource("TicTacToe.fxml"));

        stage.setTitle("TicTacToe by Hassan Althaf");
        Scene scene = new Scene(root);

        stage.setScene(scene);
        stage.show();
    }

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

}

TicTacToeController.java:

```
package tictactoe;

import java.net.URL;
import java.util.ResourceBundle;
import java.util.Random;
import javafx.scene.input.MouseEvent;
import javafx.fxml.FXML;
import javafx.fxml.Initializable;
import javafx.scene.shape.Circle;
import javafx.scene.control.Label;
import javafx.event.ActionEvent;

public class TicTacToeController implements Initializable {
@FXML
private Circle CircleOne;

@FXML
private Circle CircleTwo;

@FXML
private Circle CircleThree;

@FXML
private Circle CircleFour;

@FXML
private Circle CircleFive;

@FXML
private Circle CircleSix;

@FXML
private Circle CircleSeven;

@FXML
private Circle CircleEight;

@FXML
private Circle CircleNine;

@FXML
private Label XOne;

@FXML
private Label XTwo;

@FXML
private Label XThree;

@FXML
private Label XFour;

@FXML
private Label XFive;

@FXML
private Label XSix;

@FXML
private Label XSeven;

@FXML
private Label XEight;

@FXML
p

View

It was a lucky choice to implement tic-tac-toa and not e.g. go. With circleOne to circleThreeHundredAndSixtyOne it could get rather lengthy.

You can see what I dislike most. Starting with the XML abuse(*), the repetitiveness propagates everywhere.

Maybe FXML allows arrays and loops, which could shorten your XML. But then we get to another point, namely XML programing:

There's only one thing worse than people claiming they program in XML, and that's people who actually program in XML. See ANT for examples.

Your FXML has at least one tiny bug, namely the y-coordinates 99 and 100.

Model

You have a view (the FXML), a controller, but no model. A model is something what stores the whole game state and is completely independent of the GUI. Having a model has only advantages

• testability: You can test if the game logic works automatically.

• flexibility: You can save the model in a file, you can send it over network. You can plug in a network player or an AI.

• Separation of Concerns: The code gets much cleaner.

Controller

private Circle CircleOne;

According to Java conventions, variable names start lowercase. I'd go for circle1, as the suffix "One" gives you nothing besides looking better.

private int[] filledSquares = new int[9];
private int[] filledCircles = new int[5];
private int[] filledX = new int[5];

No idea, what this is about. Why are there just 5 of them? Maybe because of the game taking at most 9 plies, i.e., 5 moves? You should use constants (or comments) making it clear.

@FXML
public void handleSquareOneClick(MouseEvent event) {
    this.handleSquareClick(1);
}

...

public void handleSquareClick(int squareNumber) {
    if(!isAlreadySelectedBox(squareNumber) && this.allowMoves == true) {
        switch(squareNumber) {
            case 1:
                this.showCircleOne();
                break;

....

public void showCircleNine() {
    this.CircleNine.setVisible(true);
}

...

public void showCircleOne() {
    this.CircleOne.setVisible(true);
}

...

public void showXNine() {
    this.XOne.setVisible(true);
}

So due to the FXML, every single thing is repeated 9 or 18 times. You need 36 trivial methods, a complicated switch and you repeat and repeat and repeat....

Have I said already that the repetitive XML leads to repetitive code?

Have I said already that the repetitive XML leads to repetitive code?

Have I said already that the repetitive XML leads to repetitive code?

(*) YMMV, but usually, all these "declarativeness" means actually "verbosity and repetitiveness". IMHO, arrays, loops, and a good layout manager is the way to go. It works for Java FX, too. For tic-tac-toe, GridLayout is surely good enough, see my ultimatoe.
Practical advice

As you can see, I wouldn't use any FXML. YMMV, but in any case, use arrays, loops, and general methods.

private final Circle[] circles = {circle1, circle2, ..., circle9};

Replace all the handleSquareXxxClick by

public void handleSquareClick(MouseEvent event) {
     handleSquareClick(circles.indexOf(event.getSource());
}

(you may need to adapt it a bit to what really is the click source).

public void handleSquareClick(int squareNumber) {
    if(!isAlreadySelectedBox(squareNumber) && this.allowMoves == true) {
        circles[squareNumber].setVisible(true);
        ...

I can see I was a bit wrong, there's something like a model, namely filledSquares and filledCircles, but that's more useful for the history than for finding out the current state. Instead of

public boolean isAlreadySelectedBox(int squareNumber) {
    boolean found = false;

    for(int filledSquare : this.filledSquares) {
        if(squareNumber == filledSquare) {
            found = true;
        }
    }

    return found == true;
}

which btw. could be simplified to

public boolean isAlreadySelectedBox(int squareNumber) {
    for(int filledSquare : this.filledSquares) {
        if(squareNumber == filledSquare) {
            return true;
        }
    }
    return false;
}

you could have

private final boolean[] squareIsFilled = new boolean[9];

and set squareIsFilled[squareNumber] = true in handleSquareClick.

Or possibly better, use

enum FieldState {EMPTY, CIRCLE, CROSS}
private final FieldState fieldStates = new FieldState[9];

to keep all needed information in a single array.

Whatever you do, you should aim at making the code less repetitive as in the current state it's hard to see anything.
My personal rules

• before copying anything, think about a different solution (that's sometimes hard, but still easier than a later de-duplication)

• always make a gui-free part (create a class called Model and put in everything you can)

Example Model

```
class Model {
private int turn;
private final FieldState fieldStates = new FieldState[9];

public boolean canPlay(int index) ...
public void play(int index) ...
public boolean isDecide

StackExchange Code Review Q#95870, answer score: 8