6
\$\begingroup\$

I've implemented a Tic Tac Toe game with a public method checkWin() to check if the game has a winner. This is my code.

public class TicTacToe {
    int size;
    int [][] board;


    public TicTacToe(int size){
        this.size = size;
        board = new int[size][size];
        for (int i = 0; i <size ; i++) {
            for (int j = 0; j <size ; j++) {
                board[i][j] = -1;
            }
        }

    }

    public void markCross(int row, int col){
        if(row >= size || col >= size) {
            throw new IllegalStateException("Invalid grid point");
        }
        board[row][col] = 1;

    }

    public void markZero(int row, int col){
        if(row >= size || col >= size){
            throw new IllegalStateException("Invalid grid location");
        }
        board[row][col] = 0;
    }

    public boolean checkWin(){
       int countOnes = 0;
        int countZeros = 0;
        for (int i = 0; i < size; i++) {
            for (int j = 0; j < size; j++) {

                if(board[i][j] == 0) countZeros++;
                else if(board[i][j] == 1)countOnes++;
            }
            if(countZeros == size || countOnes == size) return true;
            break;
        }
        countOnes = 0;
        countZeros = 0;

        //check diagonal right

        for (int i = 0, j= 0; i <size ; i++, j++) {
            if (board[i][j] == 0) countZeros++;
            else if(board[i][j] == 1) countOnes++;
        }
        if(countOnes == size || countZeros == size) return true;

        //check diagonal left
        for (int i = size-1, j= size-1; i >= 0 ; i--, j--) {
            if (board[i][j] == 0) countZeros++;
            else if(board[i][j] == 1) countOnes++;
        }
        if(countOnes == size || countZeros == size) return true;

        return false;


    }
}
\$\endgroup\$

2 Answers 2

2
\$\begingroup\$

I would suggest this instead :

public boolean checkWin(){
    boolean diagLTRHasWinner = true;
    boolean diagRTLHasWinner = true;
    for(int j = 1; j < this.size; j++){
        boolean columHasWinner = true;
        boolean rowHasWinner = true;
        for(int i =0; i < this.size; i++){
            columHasWinner = columHasWinner && (board[i][j] == board[i][j-1]);
            rowHasWinner = rowHasWinner && (board[j][i] == board[j-1][i]);
        }
        if ((rowHasWinner && board[j][0] != -1) || (columHasWinner && board[0][j] != -1)){
            return true;
        }
        diagLTRHasWinner = diagLTRHasWinner  && (board[j][j] == board[j-1][j-1]);
        diagRTLHasWinner = diagLTRHasWinner  && (board[this.size - j][j] == board[this.size - j+1][j-1]);
    }
    return (diagRTLHasWinner && board[0][0] != -1) || (diagLTRHasWinner && board[this.size-1][0] != -1);
}

In order to reduce the number of loops, and objects allocated. Besides the new code does not even count the element in the board, it just checks if we have a valid sequence of X or O, no matter which one, neither how many! It simply detects the end of the game !

\$\endgroup\$
4
  • \$\begingroup\$ It is better. I have removed my -1 \$\endgroup\$
    – SirPython
    Jan 15, 2016 at 0:45
  • \$\begingroup\$ This will also return true if a row, column, or diagonal is empty. Note that all the entries start as -1. So this is not a correct solution. \$\endgroup\$
    – mdfst13
    Jan 15, 2016 at 2:03
  • \$\begingroup\$ does my edit fix that ? \$\endgroup\$ Jan 15, 2016 at 8:57
  • 3
    \$\begingroup\$ This is a lot harder to read than the original code. Code Review isn't all about running time (why chose Java in that case at all?), it's about making readable and maintainable code. \$\endgroup\$
    – AlexR
    Feb 14, 2016 at 13:58
2
\$\begingroup\$

Great start. But there is room for improvement as well :)

Reuse

I would factor out for which player you are checking the win, so you only count for x or o (1 or 0) in your case.

Then way, your method becomes:

  • shorter and simpler
  • you can determine who has won
  • you can re-use it to determine any-win as well.

Something like this:

public boolean checkWin(int player){
    int countPlayer = 0;
    ....
}

And

public boolean checkWin() {
    return checkWin(1) && checkWin(0)
}

Names

I would encode the -1, 0 and 1 as static ints (er even Enums), so you code becomes more readable:

final static int PLAYER_X =  1;
final static int PLAYER_O =  0;
final static int EMPTY    = -1;

Code become something like this:

    for (int i = 0; i <size ; i++) {
        for (int j = 0; j <size ; j++) {
            board[i][j] = EMPTY;
        }
    }

And

public void markCross(int row, int col){
        if(row >= size || col >= size) {
            throw new IllegalStateException("Invalid grid point");
        }
        board[row][col] = PLAYER_X;

    }

Or, with enums:

 public enum State { Empty, X, O }
 public Enum<State>[][] board;

    for (int i = 0; i <size ; i++) {
        for (int j = 0; j <size ; j++) {
            board[i][j] = State.EMPTY;
        }
    }
\$\endgroup\$

This site is temporarily in read-only mode and not accepting new answers.

Not the answer you're looking for? Browse other questions tagged .