HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Tic-Tac-Toe design

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
tacdesigntictoe

Problem

Kindly look at this code and point out design flaws or areas for improvement. I think I've done the last part right (the enum). I think the rest of the design is flawed but I am very new to object-oriented programming.

Game

```
//Game class establishes rules and finds winner

public class Game {
String[] gameState = new String[9];
Player player1;
Player player2;

// updates the gamestate array with the latest move

public void updateStatus(int position, String symbol) {
gameState[position] = symbol;
}

public Game() {
player1 = new Player(1);
player2 = new Player(2);
for(int i = 0; i < 9; i++) {
gameState[i] = "";
}
}

// checks if game over. If game over, return winner or tie, else return "Game in prgress"

public String getGameStatus() {
if((gameState[0].equals(gameState[1]))
&& gameState[0].equals(gameState[2])
&& gameState[1].equals(gameState[2])
&& !gameState[0].equals("")){
return gameState[0];
}
else if((gameState[3].equals(gameState[4]))
&& gameState[3].equals(gameState[5])
&& gameState[4].equals(gameState[5])
&& !gameState[3].equals("")){
return gameState[3];
}
else if((gameState[6].equals(gameState[7]))
&& gameState[6].equals(gameState[8])
&& gameState[7].equals(gameState[8])
&& !gameState[6].equals("")){
return gameState[6];
}
else if((gameState[0].equals(gameState[3]))
&& gameState[0].equals(gameState[6])
&& gameState[3].equals(gameState[6])
&& !gameState[0].equals("")){
return gameState[0];
}
else if((gameState[1].equals(gameState[4]))
&& gameState[1].equals(gameState[7])
&& gameState[4].equals(gameState[7])
&& !gameState[1].equals("")){
return gameState[1];
}
else if((gameState[2].equals(gameState[5]))
&& gameState[2].equals(gameState

Solution

If you like enums, this example has two of them. The code is untested but might give you an idea.

public class Game {
    private final Symbol[] gameState = new Symbol[9];
    private final Symbol player1;
    private final Symbol player2;
    private Symbol currentPlayer;

    public void updateStatus(int position, Symbol symbol) {
        gameState[position] = symbol;
    }

    public Game() {
        player1 = Symbol.X;
        player2 = Symbol.O;
        currentPlayer = player1;
        Arrays.fill(gameState, Symbol.EMPTY);
    }

    public GameState getGameStatus() {
        // check horizontal
        for (int i = 0; i < 9; i += 3) {
            if (gameState[i] != Symbol.EMPTY
                && gameState[i] == gameState[i+1]
                && gameState[i+1] == gameState[i+2]) {
                return gameState[i].won();
            }
        }

        // check vertical
        for (int i = 0; i < 3; i ++) {
            if (gameState[i] != Symbol.EMPTY
                && gameState[i] == gameState[i+3]
                && gameState[i+3] == gameState[i+6]) {
                return gameState[i].won();
            }
        }

        // check diagonal
        if (gameState[0] != Symbol.EMPTY
            && gameState[0] == gameState[4]
            && gameState[4] == gameState[9]) {
            return gameState[0].won();
        }
        if (gameState[2] != Symbol.EMPTY
            && gameState[2] == gameState[4]
            && gameState[4] == gameState[7]) {
            return gameState[2].won();
        }

        // check in progress
        for (int i = 0; i < 9; i++) {
            if (gameState[i] == Symbol.EMPTY) {
                return GameState.RUNNING;
            }
        }

        // draw
        return GameState.DRAW;
    }

    public Symbol play(int position) {
        Symbol playing = currentPlayer;
        updateStatus(position, playing);
        currentPlayer = currentPlayer == player1 ? player2 : player1;
        return playing;
    }
}

public enum Symbol {
    EMPTY(null),
    X(GameState.X_WON),
    O(GameState.O_WON);

    private final GameState won;

    private Symbol(GameState won) {
        this.won = won;
    }

    public GameState won() {
        return won;
    }
}

public enum GameState {
    RUNNING("Game in progress"),
    X_WON("X won"),
    O_WON("O won"),
    DRAW("It's a tie!!");

    private String message;

    private GameState(String message) {
        this.message = message;
    }

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


  • use loops to reduce the number of if/else statements



  • use meaningful objects wherever possible



  • use use Strings only where necessary



  • reduce the number of mutable objects (here we have only one)



Further improvements:

  • reduce the number of public methods



  • split long methods (getGameStatus in this case)



  • reduce the number of conditionals in getGameStatus, it is still spaghetti code

Code Snippets

public class Game {
    private final Symbol[] gameState = new Symbol[9];
    private final Symbol player1;
    private final Symbol player2;
    private Symbol currentPlayer;

    public void updateStatus(int position, Symbol symbol) {
        gameState[position] = symbol;
    }

    public Game() {
        player1 = Symbol.X;
        player2 = Symbol.O;
        currentPlayer = player1;
        Arrays.fill(gameState, Symbol.EMPTY);
    }

    public GameState getGameStatus() {
        // check horizontal
        for (int i = 0; i < 9; i += 3) {
            if (gameState[i] != Symbol.EMPTY
                && gameState[i] == gameState[i+1]
                && gameState[i+1] == gameState[i+2]) {
                return gameState[i].won();
            }
        }

        // check vertical
        for (int i = 0; i < 3; i ++) {
            if (gameState[i] != Symbol.EMPTY
                && gameState[i] == gameState[i+3]
                && gameState[i+3] == gameState[i+6]) {
                return gameState[i].won();
            }
        }

        // check diagonal
        if (gameState[0] != Symbol.EMPTY
            && gameState[0] == gameState[4]
            && gameState[4] == gameState[9]) {
            return gameState[0].won();
        }
        if (gameState[2] != Symbol.EMPTY
            && gameState[2] == gameState[4]
            && gameState[4] == gameState[7]) {
            return gameState[2].won();
        }

        // check in progress
        for (int i = 0; i < 9; i++) {
            if (gameState[i] == Symbol.EMPTY) {
                return GameState.RUNNING;
            }
        }

        // draw
        return GameState.DRAW;
    }

    public Symbol play(int position) {
        Symbol playing = currentPlayer;
        updateStatus(position, playing);
        currentPlayer = currentPlayer == player1 ? player2 : player1;
        return playing;
    }
}

public enum Symbol {
    EMPTY(null),
    X(GameState.X_WON),
    O(GameState.O_WON);

    private final GameState won;

    private Symbol(GameState won) {
        this.won = won;
    }

    public GameState won() {
        return won;
    }
}

public enum GameState {
    RUNNING("Game in progress"),
    X_WON("X won"),
    O_WON("O won"),
    DRAW("It's a tie!!");

    private String message;

    private GameState(String message) {
        this.message = message;
    }

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

Context

StackExchange Code Review Q#31769, answer score: 5

Revisions (0)

No revisions yet.