2
\$\begingroup\$

I have written a Tic Tac Toe application with the focus of clear design and easy to read code.

The current algorithms are very crude. I also want to separate them using interfaces. Any tips on improving the design, and making the code cleaner, change variable names?

TicTacToe.java

package tictactoe;

public class TicTacToe {

    /**
     * @param args the command line arguments
     */
    public static void main(String[] args) {
        Game g = new Game();    
    }

}

Game.java

package tictactoe;

import tictactoe.GameBoard.Board;
import tictactoe.GamePlayer.Player;
import tictactoe.GamePlayer.PlayerType;
import tictactoe.GamePlayer.CirclePlayer;
import tictactoe.GamePlayer.CrossPlayer;
import tictactoe.GameGui.MainFrame;

public class Game {
    private Board board;
    private CrossPlayer crossPlayer;
    private CirclePlayer circlePlayer;
    private Player currentPlayer;
    private MainFrame frame;

    public Game() {

        board = new Board();        //init Game Board.
        frame = new MainFrame(this); //init Gui 

        initPlayers();
    }

    private void initPlayers() {
        crossPlayer = new CrossPlayer(board);
        circlePlayer = new CirclePlayer(board);
        currentPlayer = crossPlayer;
    }

    public void togglePlayer() {
        if (currentPlayer.equals(crossPlayer)) {
            currentPlayer = circlePlayer;
        } else {
            currentPlayer = crossPlayer;
        }
    }
    /*
        Called when the player clicks on the ith square.
    */
    public void makeMove(int i) {
        //find what row and col, the i:th square lies on.
        int row = i/3;
        int col = i%3;

        currentPlayer.makeMove(row, col);
        frame.markSquare(i, currentPlayer);
        frame.disableSquare(i);
        togglePlayer();
        display();
    }

    /**
     * Display the board in the console.
     */
    public void display() {
        PlayerType markType;

        for (int i = 0; i < 3; i++) {

            for (int j = 0; j < 3; j++) {
                markType = board.getSquare(i, j).getMark();

                if (markType  == PlayerType.None) {
                    System.out.print("*");
                }
                else if(markType == PlayerType.Cross) {
                    System.out.print("X");
                }
                else if(markType == PlayerType.Circle) {
                    System.out.print("O");
                }
            }
            System.out.print("\n");
        }
    }

    public PlayerType checkDiag() {

        if (board.getSquare(0, 0).getMark() == board.getSquare(1, 1).getMark() &&
                board.getSquare(0, 0).getMark() == board.getSquare(2, 2).getMark()) {
            return board.getSquare(0, 0).getMark();
        }
        if (board.getSquare(0, 2).getMark() == board.getSquare(1, 1).getMark() &&
                board.getSquare(0, 2).getMark() == board.getSquare(2, 0).getMark()) {
            return board.getSquare(0, 2).getMark();
        }
        return PlayerType.None;
    }

    public PlayerType checkCol(int col) {
        if (board.getSquare(0, col).getMark() == board.getSquare(1, col).getMark() &&
                board.getSquare(0, col).getMark() == board.getSquare(2, col).getMark()){
            return board.getSquare(0, col).getMark();
        }
        return PlayerType.None;
    }

    public PlayerType checkRow(int row) {

        if (board.getSquare(row, 0).getMark() == board.getSquare(row, 1).getMark() &&
                board.getSquare(row, 0).getMark() == board.getSquare(row, 2).getMark()) { //row full
            return board.getSquare(row, 0).getMark();
        }

        return PlayerType.None;
    }

    public PlayerType playerWon() {
        PlayerType winner;
        for (int i = 0; i < 3; i++) {
            winner = checkRow(i);
            if (winner != PlayerType.None) {
                return winner;
            }

            winner = checkCol(i);
            if (winner != PlayerType.None) {
                return winner;
            }
        }
        return checkDiag();
    }

}

MainFrame.java

package tictactoe.GameGui;

import java.awt.GridLayout;
import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;
import java.util.ArrayList;
import javax.swing.JButton;
import javax.swing.JFrame;
import javax.swing.JPanel;
import tictactoe.Game;
import tictactoe.GamePlayer.Player;
import tictactoe.GamePlayer.PlayerType;

public class MainFrame extends JFrame implements ActionListener{

    private ArrayList<JButton> squares;         //Squares that the player can mark X or O.
    private JPanel SquaresPanel = new JPanel(); //The panel that holds the squares on the window.
    private Game game;

    private final int WinHeight = 300;
    private final int WinWidth = 300;
    private final int nrOffSquares = 9;


    public MainFrame(Game game) {
        super("Tic Tac Toe");
        this.game = game;

        initWindow();
        initSquares();  
        initSquaresPanel();

    }
    private void initWindow() {

        setSize(WinWidth, WinHeight);
        setDefaultCloseOperation(EXIT_ON_CLOSE);
        setVisible(true);
    }

    private void initSquares() {
        squares = new ArrayList<>();

        for (int i = 0; i < nrOffSquares; i++) {
            JButton b = new JButton();
            b.setFont(b.getFont().deriveFont(18.0f));
            b.addActionListener(this);
            SquaresPanel.add(b);
            squares.add(b);
        }
    }

    private void initSquaresPanel() {

        SquaresPanel.setLayout(new GridLayout(3,3)); 
        add(SquaresPanel);
    }

    public void markSquare(int i, Player p) {
        if (p.getType() == PlayerType.Cross) {
            squares.get(i).setText("X");
        }else {
            squares.get(i).setText("O");
        }
    }
    public void disableAllSquares() {
        for (int i = 0; i < nrOffSquares; i++)
            disableSquare(i);
    }
    public void disableSquare(int i) {
        squares.get(i).setEnabled(false);
    }

    @Override
    public void actionPerformed(ActionEvent e) {
        JButton b = (JButton) e.getSource();

        for (int i = 0; i < nrOffSquares; i++) {
            if (b.equals(squares.get(i))) {        //if the ith square is clicked
                game.makeMove(i);                  //Make a move on the ith square.
            }
        } 
    }
}

Board.java

package tictactoe.GameBoard;

import tictactoe.GamePlayer.PlayerType;

public class Board {
    private Square  board[][] = new Square [3][3];
    public Board() {
        for (int i = 0; i < 3; i++) {
            for (int j = 0; j < 3; j++) {
                board[i][j] = new Square();
            }
        }
    }
    public void setSquare (int row, int col, PlayerType type) {
        board[row][col].setMark(type);
    }
    public Square getSquare(int row, int col) {
        return board[row][col];
    }
}

Square.java

package tictactoe.GameBoard;

import tictactoe.GamePlayer.PlayerType;

public class Square {
    private PlayerType mark = PlayerType.None;

    public PlayerType getMark() {
        return mark;
    }

    public void setMark(PlayerType mark) {
        this.mark = mark;
    }

}

Player.java

package tictactoe.GamePlayer;

import tictactoe.GameBoard.Board;

public class Player {
    private PlayerType type;
    private Board board;

    public Player(Board board, PlayerType type) {
        this.type = type;
        this.board = board;
    }
    public void makeMove(int row, int col) {
        board.setSquare(row, col, type);
    }
    public PlayerType getType() {
        return this.type;
    }
}

PlayerType.java

package tictactoe.GamePlayer;


public enum PlayerType {
    None,
    Cross,
    Circle
}

CrossPlayer.java

package tictactoe.GamePlayer;

import tictactoe.GameBoard.Board;

public class CrossPlayer extends Player {

    private Board board;

    public CrossPlayer(Board board) {
        super(board, PlayerType.Cross);
        this.board = board;

    }
}

CirclePlayer.java

package tictactoe.GamePlayer;

import tictactoe.GameBoard.Board;

public class CirclePlayer extends Player {

    private Board board;

    public CirclePlayer(Board board) {

        super(board, PlayerType.Circle);
        this.board = board;
    }

}
\$\endgroup\$
0

3 Answers 3

3
\$\begingroup\$

As others have pointed out, you don't need the CrossPlayer and CirclePlayer classes. You can use one Player class for both, parameterized with a PlayerType.

And since Game.board and Game.crossPlayer and Game.circlePlayer never change, you could declare them as final, like this:

private final Board board = new Board();
private final Player crossPlayer = new Player(board, PlayerType.Cross);
private final Player circlePlayer = new Player(board, PlayerType.Circle);

It's good to declare variables with interface types. So instead of this:

private ArrayList<JButton> squares;

This is better:

private List<JButton> squares;

And since you only assign squares once, it's good to make it final:

private final List<JButton> squares = new ArrayList<>();

Instead of these chained else-ifs:

if (markType  == PlayerType.None) {
    System.out.print("*");
}
else if(markType == PlayerType.Cross) {
    System.out.print("X");
}
else if(markType == PlayerType.Circle) {
    System.out.print("O");
}

It's better to use a switch:

switch (markType) {
    case None:
        System.out.println("*");
        break;
    case Cross:
        System.out.println("X");
        break;
    case Circle:
        System.out.println("O");
        break;
}

Even better is to move the marker strings in the definition of the enum itself:

enum PlayerType {
    None("*"),
    Cross("X"),
    Circle("O");

    private final String symbol;

    PlayerType(String symbol) {
        this.symbol = symbol;
    }

    @Override
    public String toString() {
        return symbol;
    }
}

This way the earlier switch statement can be replaced with this single line:

System.out.println(markType);

And it will help you simplify the markSquare method too, so instead of:

public void markSquare(int i, Player p) {
    if (p.getType() == PlayerType.Cross) {
        squares.get(i).setText("X");
    }else {
        squares.get(i).setText("O");
    }
}

you will be able to write:

public void markSquare(int i, Player p) {
    squares.get(i).setText(p.getType().toString());
}
\$\endgroup\$
3
  • \$\begingroup\$ Why do we need to pass Board object to player? It would have been required if player object was the one who decides what to play? Can you please tell me the reason behind it? \$\endgroup\$
    – Arjuna
    Apr 23, 2018 at 13:37
  • \$\begingroup\$ @Arjuna it was like that in the original code and I simply did not analyze further. Indeed, if players moves are decided outside of player classes, then the board could be outside as well. In any case, I did not go deep enough to the design level in this review. It could be a good idea for someone to do it. \$\endgroup\$
    – janos
    May 15, 2018 at 6:39
  • \$\begingroup\$ Thanks a lot for answering the question. But can you please tell me which design is better in your opinion? May be Player class can be extended to play automatically e.g. computer vs human , if we pass Board object to player class? \$\endgroup\$
    – Arjuna
    May 15, 2018 at 17:52
2
\$\begingroup\$

The UI aspects of the code are convoluted and tightly coupled with your game implementation (the business logic). You can't make an instance for the Game without having a graphical display window. But yet, Game.display() writes to standard out. You should extract the business logic to be a standalone component. This way, you can use the same component without being tied to a specific display type. A console application, a graphical application, and even an application that connects to a game server over a network would options.


Within the Game class, checkDiag(), checkCol(), and checkRow() share much of the same code. You should write a sub-function that handles the common aspects and takes the different indexes as arguments. This will improve the readability greatly.

\$\endgroup\$
1
\$\begingroup\$

Ok, found another thing so I'll just make an answer out if this:

As stated in my comment, I don't think there's a need for the CirclePlayer and CrossPlayer classes. That inheritance layer does not provide any benefit since their differnce is just the value of a single attribute, not any behavior.

There's also no reason for the MainFrame#markSquare() method to know about player types. You want a simple p.getSymbol() in there. The player/mark know their symbol. Ask them to supply it. You can do a similar thing in Game#display().

The code could use some more comments but other than that it looks pretty solid and easily readable on first glance.

\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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