7
\$\begingroup\$

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[8])  
       && gameState[5].equals(gameState[8])  
       && !gameState[2].equals("")){  
        return gameState[2];  
    }  
    else if((gameState[0].equals(gameState[4]))  
       && gameState[0].equals(gameState[8])  
       && gameState[4].equals(gameState[8])  
       && !gameState[0].equals("")){  
        return gameState[0];  
    }  
    else if((gameState[2].equals(gameState[4]))  
       && gameState[2].equals(gameState[6])  
       && gameState[4].equals(gameState[6])  
       && !gameState[2].equals("")){  
        return gameState[2];  
    }  
    else {  
        for(int i=0; i < 9; i++) {  
            if(gameState[i].equals("")) {  
                return "Game in progress";  
            }  
        }  
        return "It's a tie!!";  
    }  
    }  

public String play(int position) {  
    if(!player1.hasPlayed()) {  
        player1.played(true);  
        player2.played(false);  
        updateStatus(position,player1.getSymbol());  
        return player1.getSymbol();  
    }  
    else {  
        player2.played(true);  
        player1.played(false);  
        updateStatus(position,player2.getSymbol());  
        return player2.getSymbol();  
    }  
}  
}

Player

//Player class initializes players in the game using id

public class Player {  
    private boolean hasPlayed;  
    private String symbol;  
    private int id;  

    Player(int id) {  
        this.id = id;  
        if(id == 1) {  
            hasPlayed = false;  
            symbol = Symbols.X.toString();  
        }  
        else {  
            hasPlayed = true;  
            symbol = Symbols.O.toString();  
        }  
    }  

    public void played(boolean flag) {  
        hasPlayed = flag;  
    }  
    public boolean hasPlayed() {  
        return hasPlayed;   
    }  
    public String getSymbol() {  
        return symbol;  
    }  
}  

Symbols

// Enum stores the symbols used in the game

public enum Symbols {  
    X,O;  
}  
\$\endgroup\$
5
\$\begingroup\$

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
\$\endgroup\$
5
\$\begingroup\$

Use metadata array for checking win combinations.

e.g. int[][] winCombinations = {{1,2,3}, {4,5,6}, {7,8,9}, {1,5,9}, etc}

And check them with a loop. Please note that array indexes are starting with 0, and you will need -1 or special method for getting state or changes in metadata.

    for(int[] winCombo: winCombinations) {
        String stateOfFirstComboCell = gameState[winCombo[0]];
        if (stateOfFirstComboCell != EMPTY) {
             if (stateOfFirstComboCell.equals(gameState[winCombo[1]])
                  && stateOfFirstComboCell.equals(gameState[winCombo[2]])) {
                  System.out.println("Winner is " + stateOfFirstComboCell);  
             }
        }
    }
\$\endgroup\$
2
\$\begingroup\$

IMHO the getGameStatus() method has extremely high cyclomatic complexity. Also I'm not sure what is the purpose of the Player class. Following is my solution:

public class TicTacToe {
    public static final char BLANK = '\u0000';
    private final char[][] board;
    private int moveCount;
    private Referee referee;

    public TicTacToe(int gridSize) {
        if (gridSize < 3)
            throw new IllegalArgumentException("TicTacToe board size has to be minimum 3x3 grid");
        board = new char[gridSize][gridSize];
        referee = new Referee(gridSize);
    }

    public char[][] displayBoard() {
        return board.clone();
    }

    public String move(int x, int y) {
        if (board[x][y] != BLANK)
            return "(" + x + "," + y + ") is already occupied";
        board[x][y] = whoseTurn();
        return referee.isGameOver(x, y, board[x][y], ++moveCount);
    }

    private char whoseTurn() {
        return moveCount % 2 == 0 ? 'X' : 'O';
    }
}

And the core logic to check if the game is over.

private class Referee {
    private static final int NO_OF_DIAGONALS = 2;
    private static final int MINOR = 1;
    private static final int PRINCIPAL = 0;
    private final int gridSize;
    private final int[] rowTotal;
    private final int[] colTotal;
    private final int[] diagonalTotal;

    private Referee(int size) {
        gridSize = size;
        rowTotal = new int[size];
        colTotal = new int[size];
        diagonalTotal = new int[NO_OF_DIAGONALS];
    }

    private String isGameOver(int x, int y, char symbol, int moveCount) {
        if (isWinningMove(x, y, symbol))
            return symbol + " won the game!";
        if (isBoardCompletelyFilled(moveCount))
            return "Its a Draw!";
        return "continue";
    }

    private boolean isBoardCompletelyFilled(int moveCount) {
        return moveCount == gridSize * gridSize;
    }

    private boolean isWinningMove(int x, int y, char symbol) {
        if (isPrincipalDiagonal(x, y) && allSymbolsMatch(symbol, diagonalTotal, PRINCIPAL))
            return true;
        if (isMinorDiagonal(x, y) && allSymbolsMatch(symbol, diagonalTotal, MINOR))
            return true;
        return allSymbolsMatch(symbol, rowTotal, x) || allSymbolsMatch(symbol, colTotal, y);
    }

    private boolean allSymbolsMatch(char symbol, int[] total, int index) {
        total[index] += symbol;
        return total[index] / gridSize == symbol;
    }

    private boolean isPrincipalDiagonal(int x, int y) {
        return x == y;
    }

    private boolean isMinorDiagonal(int x, int y) {
        return x + y == gridSize - 1;
    }
}

Full source available at: https://github.com/nashjain/tictactoe/tree/master/java

\$\endgroup\$
  • 1
    \$\begingroup\$ Same as on your other post. I would warn the OP that throwing an Exception in order to control the flow of the program rather than when there's actually an error is very poor design. Also, the method names are pretty odd and lengthy (even for Java), and you should declare everything explicitly as public, protected, or private. Other than that, this code has some decent tips contained in it. \$\endgroup\$ – asteri Oct 17 '13 at 13:33
  • \$\begingroup\$ Good point about using errors to control the flow. If you can suggest better names for the methods, it would really help me learn. \$\endgroup\$ – Naresh Jain Oct 17 '13 at 16:40
  • \$\begingroup\$ I've removed the exceptions and using a simple string to control the flow. Also fixed a bunch of typos in the name. \$\endgroup\$ – Naresh Jain Oct 18 '13 at 4:16
2
\$\begingroup\$

Dependency injection - there's no particular reason that Game needs to specify a particular implementation of Player, or that the two players need to have the same type. So these should be arguments to a constructor

public Game () {
    this(new Player(1), new Player(2));
}

public Game(Player player1, Player player2) {
    this.player1 = player1;
    this.player2 = player2;
}

Although to be honest, it's not clear that the Player object is actually contributing in anyway - at the moment, it is just storing state that you don't seem to need.

You've probably got an enum floating around with five states: X to play, O to play, X wins, O wins, Tie. These enums might each be carrying an isGameOver() flag.

Game.gameState could be represented by a finite state machine (FSM). Take a look at the State pattern in Design Patterns (Gamma, et al). Each state knows which cells in the grid are occupied, which player has the move, which moves are valid, and what state comes next for each move. 9 factorial states may seem like a lot, but a number of them get pruned away because the game ends before they can be reached, or because there are multiple paths to get there, or because you can cleverly rotate/reflect the coordinates of the board to take advantage of symmetries.

In the FSM approach, you would probably calculate the status of each state as you create it. I would probably use a Scorer (aka Referee, as suggested by @NareshJain), but break out the different win conditions explicitly (checkColumnForWin(), checkRowForWin(), checkDiagonalForWin()). But given that there are only 8 winning conditions to worry about, maybe @Admit's array is the right idea. Or you could combine the ideas, and have the Referee (implementing the rules in a human readable way) producing a list of winning positions (which are then used to evaluate the states).

\$\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.