3
\$\begingroup\$

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
    private Label XNine;

    @FXML
    private Label lblMessages;

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

    private int filledSquaresCounter = 0;
    private int filledCirclesCounter = 0;
    private int filledXCounter = 0;

    private char winningTeam;

    final private int[][] winningPositions = {
        {1, 5, 9},
        {3, 5, 7},
        {1, 2, 3},
        {4, 5, 6},
        {7, 8, 9},
        {1, 4, 7},
        {2, 5, 8},
        {3, 6, 9}
    };

    private boolean allowMoves = true;

    private boolean tie = false;

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

    @FXML
    public void handleSquareTwoClick(MouseEvent event) {
        this.handleSquareClick(2);
    }

    @FXML
    public void handleSquareThreeClick(MouseEvent event) {
        this.handleSquareClick(3);
    }

    @FXML
    public void handleSquareFourClick(MouseEvent event) {
        this.handleSquareClick(4);
    }

    @FXML
    public void handleSquareFiveClick(MouseEvent event) {
        this.handleSquareClick(5);
    }

    @FXML
    public void handleSquareSixClick(MouseEvent event) {
        this.handleSquareClick(6);
    }

    @FXML
    public void handleSquareSevenClick(MouseEvent event) {
        this.handleSquareClick(7);
    }

    @FXML
    public void handleSquareEightClick(MouseEvent event) {
        this.handleSquareClick(8);
    }

    @FXML
    public void handleSquareNineClick(MouseEvent event) {
        this.handleSquareClick(9);
    }

    public void handleSquareClick(int squareNumber) {
        if(!isAlreadySelectedBox(squareNumber) && this.allowMoves == true) {
            switch(squareNumber) {
                case 1:
                    this.showCircleOne();
                    break;
                case 2:
                    this.showCircleTwo();
                    break;
                case 3:
                    this.showCircleThree();
                    break;
                case 4:
                    this.showCircleFour();
                    break;
                case 5:
                    this.showCircleFive();
                    break;
                case 6:
                    this.showCircleSix();
                    break;
                case 7:
                    this.showCircleSeven();
                    break;    
                case 8:
                    this.showCircleEight();
                    break;
                case 9:
                    this.showCircleNine();
                    break;
                default:
                    System.out.println("Impossible choice");
                    break;
            }

            this.filledSquares[this.filledSquaresCounter] = squareNumber;
            this.filledCircles[this.filledCirclesCounter] = squareNumber;
            this.filledSquaresCounter++;
            this.filledCirclesCounter++;

            if(this.checkVictory()) {
                this.endGame();
            } else {
                this.playRandomMove();

                if(this.checkVictory()) {
                    this.endGame();
                }
            }
        } else if(this.filledSquaresCounter >= 9) {
            this.tie = true;
            this.endGame();
        }
    }

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

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

        return found == true;
    }

    public boolean checkVictory() {
        if(this.filledCirclesCounter < 3 && this.filledXCounter < 3) {
            return false;
        }

        for(int[] filled : this.winningPositions) {
            int slotCounter = 0;

            for(int singleFilled : filled) {
                if(this.isOccupiedByCircle(singleFilled)) {
                    slotCounter++;
                }
            }

            if(slotCounter == 3) {
                this.winningTeam = 'O';
                this.allowMoves = false;
                return true;
            }

            slotCounter = 0;

            for(int singleFilled : filled) {
                if(this.isOccupiedByX(singleFilled)) {
                    slotCounter++;
                }
            }

            if(slotCounter == 3) {
                this.winningTeam = 'X';
                this.allowMoves = false;
                return true;
            }
        }

        return false;
    }

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

    public void showCircleTwo() {
        this.CircleTwo.setVisible(true);
    }

    public void showCircleThree() {
        this.CircleThree.setVisible(true);
    }

    public void showCircleFour() {
        this.CircleFour.setVisible(true);
    }

    public void showCircleFive() {
        this.CircleFive.setVisible(true);
    }

    public void showCircleSix() {
        this.CircleSix.setVisible(true);
    }

    public void showCircleSeven() {
        this.CircleSeven.setVisible(true);
    }

    public void showCircleEight() {
        this.CircleEight.setVisible(true);
    }

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

    public void playRandomMove() {
        Random random = new Random();
        int result = random.nextInt(9 - 1 + 1) + 1;;

        if(this.filledSquaresCounter < 9) {
            while(this.isAlreadySelectedBox(result)) {
                result = random.nextInt(9 - 1 + 1) + 1;
            }

            switch(result) {
                case 1:
                    this.showXOne();
                    break;
                case 2:
                    this.showXTwo();
                    break;
                case 3:
                    this.showXThree();
                    break;
                case 4:
                    this.showXFour();
                    break;
                case 5:
                    this.showXFive();
                    break;
                case 6:
                    this.showXSix();
                    break;
                case 7:
                    this.showXSeven();
                    break;    
                case 8:
                    this.showXEight();
                    break;
                case 9:
                    this.showXNine();
                    break;
                default:
                    System.out.println("Impossible choice");
                    break;
            }

            this.filledSquares[this.filledSquaresCounter] = result;
            this.filledX[this.filledXCounter] = result;
            this.filledSquaresCounter++;
            this.filledXCounter++;
        } else {
            this.tie = true;
            this.endGame();
        }


    }

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

    public void showXTwo() {
        this.XTwo.setVisible(true);
    }

    public void showXThree() {
        this.XThree.setVisible(true);
    }

    public void showXFour() {
        this.XFour.setVisible(true);
    }

    public void showXFive() {
        this.XFive.setVisible(true);
    }

    public void showXSix() {
        this.XSix.setVisible(true);
    }

    public void showXSeven() {
        this.XSeven.setVisible(true);
    }

    public void showXEight() {
        this.XEight.setVisible(true);
    }

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

    public boolean isOccupiedByCircle(int circlePosition) {
        boolean found = false;

        for(int filledCircle : this.filledCircles) {
            if(filledCircle == circlePosition) {
                found = true;
            }
        }

        return found == true;
    }

    public boolean isOccupiedByX(int xPosition) {
        boolean found = false;

        for(int filled : this.filledX) {
            if(filled == xPosition) {
                found = true;
            }
        }

        return found == true;
    }

    public void endGame() {
        this.allowMoves = false;

        if(this.tie == true) {
            this.lblMessages.setText("It was a tie!");
        } else if(String.valueOf(this.winningTeam).equals("O")) {
            this.lblMessages.setText("You win!");
        } else if(String.valueOf(this.winningTeam).equals("X")) {
            this.lblMessages.setText("Sorry, you lose!");
        }
    }

    @FXML
    public void handleResetButton(ActionEvent event) {
        this.CircleOne.setVisible(false);
        this.CircleTwo.setVisible(false);
        this.CircleThree.setVisible(false);
        this.CircleFour.setVisible(false);
        this.CircleFive.setVisible(false);
        this.CircleSix.setVisible(false);
        this.CircleSeven.setVisible(false);
        this.CircleEight.setVisible(false);
        this.CircleNine.setVisible(false);

        this.XOne.setVisible(false);
        this.XTwo.setVisible(false);
        this.XThree.setVisible(false);
        this.XFour.setVisible(false);
        this.XFive.setVisible(false);
        this.XSix.setVisible(false);
        this.XSeven.setVisible(false);
        this.XEight.setVisible(false);
        this.XNine.setVisible(false);

        this.winningTeam = 0;

        this.allowMoves = true;
        this.tie = false;

        this.lblMessages.setText("");

        this.filledSquares = new int[9];
        this.filledCircles = new int[5];
        this.filledX = new int[5];

        this.filledSquaresCounter = 0;
        this.filledCirclesCounter = 0;
        this.filledXCounter = 0;
    }

    @Override
    public void initialize(URL url, ResourceBundle rb) {
        // TODO
    }    

}

TicTacToe.fxml:

<?xml version="1.0" encoding="UTF-8"?>

<?import javafx.scene.text.*?>
<?import javafx.scene.shape.*?>
<?import java.lang.*?>
<?import java.util.*?>
<?import javafx.scene.*?>
<?import javafx.scene.control.*?>
<?import javafx.scene.layout.*?>

<AnchorPane id="AnchorPane" prefHeight="450.0" prefWidth="600.0" xmlns="http://javafx.com/javafx/8" xmlns:fx="http://javafx.com/fxml/1" fx:controller="tictactoe.TicTacToeController">
   <children>
      <Line endX="150.0" layoutX="300.0" layoutY="150.0" startX="-150.0" />
      <Line endY="150.0" layoutX="250.0" layoutY="200.0" startY="-150.0" />
      <Line endY="150.0" layoutX="350.0" layoutY="200.0" startY="-150.0" />
      <Line endX="150.0" layoutX="300.0" layoutY="250.0" startX="-150.0" />
      <Rectangle fx:id="SquareOne" arcHeight="5.0" arcWidth="5.0" fill="WHITE" height="98.0" layoutX="151.0" layoutY="51.0" onMouseClicked="#handleSquareOneClick" stroke="WHITE" strokeType="INSIDE" width="98.0" />
      <Rectangle fx:id="SquareTwo" arcHeight="5.0" arcWidth="5.0" fill="WHITE" height="98.0" layoutX="251.0" layoutY="50.0" onMouseClicked="#handleSquareTwoClick" stroke="WHITE" strokeType="INSIDE" width="98.0" />
      <Rectangle fx:id="SquareThree" arcHeight="5.0" arcWidth="5.0" fill="WHITE" height="98.0" layoutX="351.0" layoutY="51.0" onMouseClicked="#handleSquareThreeClick" stroke="WHITE" strokeType="INSIDE" width="98.0" />
      <Rectangle fx:id="SquareFour" arcHeight="5.0" arcWidth="5.0" fill="WHITE" height="98.0" layoutX="150.0" layoutY="151.0" onMouseClicked="#handleSquareFourClick" stroke="WHITE" strokeType="INSIDE" width="98.0" />
      <Rectangle fx:id="SquareFive" arcHeight="5.0" arcWidth="5.0" fill="WHITE" height="98.0" layoutX="251.0" layoutY="151.0" onMouseClicked="#handleSquareFiveClick" stroke="WHITE" strokeType="INSIDE" width="98.0" />
      <Rectangle fx:id="SquareSix" arcHeight="5.0" arcWidth="5.0" fill="WHITE" height="98.0" layoutX="351.0" layoutY="151.0" onMouseClicked="#handleSquareSixClick" stroke="WHITE" strokeType="INSIDE" width="98.0" />
      <Rectangle fx:id="SquareSeven" arcHeight="5.0" arcWidth="5.0" fill="WHITE" height="98.0" layoutX="151.0" layoutY="251.0" onMouseClicked="#handleSquareSevenClick" stroke="WHITE" strokeType="INSIDE" width="98.0" />
      <Rectangle fx:id="SquareEight" arcHeight="5.0" arcWidth="5.0" fill="WHITE" height="98.0" layoutX="251.0" layoutY="251.0" onMouseClicked="#handleSquareEightClick" stroke="WHITE" strokeType="INSIDE" width="98.0" />
      <Rectangle fx:id="SquareNine" arcHeight="5.0" arcWidth="5.0" fill="WHITE" height="98.0" layoutX="351.0" layoutY="251.0" onMouseClicked="#handleSquareNineClick" stroke="WHITE" strokeType="INSIDE" width="98.0" />
      <Circle fx:id="CircleOne" fill="WHITE" layoutX="200.0" layoutY="99.0" radius="48.0" stroke="BLACK" strokeType="INSIDE" strokeWidth="8.0" visible="false" />
      <Circle fx:id="CircleTwo" fill="WHITE" layoutX="300.0" layoutY="99.0" radius="48.0" stroke="BLACK" strokeType="INSIDE" strokeWidth="8.0" visible="false" />
      <Circle fx:id="CircleThree" fill="WHITE" layoutX="400.0" layoutY="100.0" radius="48.0" stroke="BLACK" strokeType="INSIDE" strokeWidth="8.0" visible="false" />
      <Circle fx:id="CircleFour" fill="WHITE" layoutX="200.0" layoutY="200.0" radius="48.0" stroke="BLACK" strokeType="INSIDE" strokeWidth="8.0" visible="false" />
      <Circle fx:id="CircleFive" fill="WHITE" layoutX="300.0" layoutY="200.0" radius="48.0" stroke="BLACK" strokeType="INSIDE" strokeWidth="8.0" visible="false" />
      <Circle fx:id="CircleSix" fill="WHITE" layoutX="400.0" layoutY="200.0" radius="48.0" stroke="BLACK" strokeType="INSIDE" strokeWidth="8.0" visible="false" />
      <Circle fx:id="CircleSeven" fill="WHITE" layoutX="200.0" layoutY="300.0" radius="48.0" stroke="BLACK" strokeType="INSIDE" strokeWidth="8.0" visible="false" />
      <Circle fx:id="CircleEight" fill="WHITE" layoutX="300.0" layoutY="300.0" radius="48.0" stroke="BLACK" strokeType="INSIDE" strokeWidth="8.0" visible="false" />
      <Circle fx:id="CircleNine" fill="WHITE" layoutX="400.0" layoutY="300.0" radius="48.0" stroke="BLACK" strokeType="INSIDE" strokeWidth="8.0" visible="false" />
      <Label fx:id="XOne" alignment="CENTER" contentDisplay="CENTER" layoutX="151.0" layoutY="50.0" maxHeight="98.0" maxWidth="98.0" minHeight="98.0" minWidth="98.0" prefHeight="98.0" prefWidth="98.0" text="X" visible="false">
         <font>
            <Font size="88.0" />
         </font>
      </Label>
      <Label fx:id="XTwo" alignment="CENTER" contentDisplay="CENTER" layoutX="251.0" layoutY="51.0" maxHeight="98.0" maxWidth="98.0" minHeight="98.0" minWidth="98.0" prefHeight="98.0" prefWidth="98.0" text="X" visible="false">
         <font>
            <Font size="88.0" />
         </font>
      </Label>
      <Label fx:id="XThree" alignment="CENTER" contentDisplay="CENTER" layoutX="351.0" layoutY="50.0" maxHeight="98.0" maxWidth="98.0" minHeight="98.0" minWidth="98.0" prefHeight="98.0" prefWidth="98.0" text="X" visible="false">
         <font>
            <Font size="88.0" />
         </font>
      </Label>
      <Label fx:id="XFour" alignment="CENTER" contentDisplay="CENTER" layoutX="151.0" layoutY="151.0" maxHeight="98.0" maxWidth="98.0" minHeight="98.0" minWidth="98.0" prefHeight="98.0" prefWidth="98.0" text="X" visible="false">
         <font>
            <Font size="88.0" />
         </font>
      </Label>
      <Label fx:id="XFive" alignment="CENTER" contentDisplay="CENTER" layoutX="251.0" layoutY="152.0" maxHeight="98.0" maxWidth="98.0" minHeight="98.0" minWidth="98.0" prefHeight="98.0" prefWidth="98.0" text="X" visible="false">
         <font>
            <Font size="88.0" />
         </font>
      </Label>
      <Label fx:id="XSix" alignment="CENTER" contentDisplay="CENTER" layoutX="351.0" layoutY="151.0" maxHeight="98.0" maxWidth="98.0" minHeight="98.0" minWidth="98.0" prefHeight="98.0" prefWidth="98.0" text="X" visible="false">
         <font>
            <Font size="88.0" />
         </font>
      </Label>
      <Label fx:id="XSeven" alignment="CENTER" contentDisplay="CENTER" layoutX="151.0" layoutY="251.0" maxHeight="98.0" maxWidth="98.0" minHeight="98.0" minWidth="98.0" prefHeight="98.0" prefWidth="98.0" text="X" visible="false">
         <font>
            <Font size="88.0" />
         </font>
      </Label>
      <Label fx:id="XEight" alignment="CENTER" contentDisplay="CENTER" layoutX="251.0" layoutY="251.0" maxHeight="98.0" maxWidth="98.0" minHeight="98.0" minWidth="98.0" prefHeight="98.0" prefWidth="98.0" text="X" visible="false">
         <font>
            <Font size="88.0" />
         </font>
      </Label>
      <Label fx:id="XNine" alignment="CENTER" contentDisplay="CENTER" layoutX="351.0" layoutY="251.0" maxHeight="98.0" maxWidth="98.0" minHeight="98.0" minWidth="98.0" prefHeight="98.0" prefWidth="98.0" text="X" visible="false">
         <font>
            <Font size="88.0" />
         </font>
      </Label>
      <Label fx:id="lblMessages" alignment="CENTER" contentDisplay="CENTER" layoutX="150.0" layoutY="366.0" prefHeight="20.0" prefWidth="300.0">
         <font>
            <Font size="16.0" />
         </font>
      </Label>
      <Button fx:id="btnReset" layoutX="274.0" layoutY="398.0" mnemonicParsing="false" onAction="#handleResetButton" text="Reset" />
   </children>
</AnchorPane>
\$\endgroup\$
7
+50
\$\begingroup\$

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.

  <Circle fx:id="CircleTwo" fill="WHITE" layoutX="300.0" layoutY="99.0" radius="48.0" stroke="BLACK" strokeType="INSIDE" strokeWidth="8.0" visible="false" />
  <Circle fx:id="CircleThree" fill="WHITE" layoutX="400.0" layoutY="100.0" radius="48.0" stroke="BLACK" strokeType="INSIDE" strokeWidth="8.0" visible="false" />

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 isDecided() ...
    public FieldState getWinner() ...
}
\$\endgroup\$
  • \$\begingroup\$ @maaartinus public .indexOf doesn't seem to work. it says no such method. \$\endgroup\$ – Hassan Althaf Aug 29 '15 at 14:29
  • \$\begingroup\$ @HassanAlthaf Sorry, I forgot it's an array and no List. Switch to it or use Arrays.asList(array).indexOf(whatever). \$\endgroup\$ – maaartinus Aug 29 '15 at 19:51
  • \$\begingroup\$ @maaartinus I've implemented most of the stuff you mentioned, but I cannot seem to implement the MVC pattern in JavaFX. Any recommended resources I could refer to? Also, I have posted a new question onto how to improve the code again. \$\endgroup\$ – Hassan Althaf Aug 31 '15 at 10:51
4
\$\begingroup\$
  • Put the Circle and Label objects in arrays so you don't need all those "showCircleNumber()" methods.
  • The Random object should be class-level. It should only be created once

The way you are returning booleans in the isOccupiedByX and isOccupiedByCircle methods is over-complicated

public boolean isOccupiedByX(int xPosition) {
    for(int filled : this.filledX) {
        if(filled == xPosition) {
            return true;
        }
    }
    return false;
}
\$\endgroup\$
3
\$\begingroup\$

Various assignments are abundant and should be simplified for readability. For example:

int result = random.nextInt(9 - 1 + 1) + 1;;

There are two semicolons where one should be removed, and 9 - 1 + 1 evaluates into 9. That should be used instead for the parameter.

Conditional statements like found == true and this.tie == true performs an unnecessary comparison. They can be used on their own: return found;.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.