5
\$\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
\$\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;
        }
    }
| improve this answer | |
\$\endgroup\$
1
\$\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 !

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ It is better. I have removed my -1 \$\endgroup\$ – SirPython Jan 15 '16 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 '16 at 2:03
  • \$\begingroup\$ does my edit fix that ? \$\endgroup\$ – Younes Regaieg Jan 15 '16 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 '16 at 13:58

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.