How can I remove one of these if statements and shorten the code?

Question

I have the following code. The only problem is that we run it through a checkstyle program and it comes up with the error Cyclomatic Complexity is 11 (max allowed is 10). I would like to know how can remove one of the if statement to make it do the same thing and let the program pass the test.

 /**
 * Check if there is a winner on the board
 * @return the winner if BLANK there is no winner
 **/

public char checkWinner(){
   this.winner = BLANK;
   int totalTiles = GRIDSIZE*GRIDSIZE;

    //Check if the game has a win
   for (int i=0; i < GRIDSIZE; i++) {

    if((grid[i][0] == grid[i][1]) && (grid[i][1] == grid[i][2])){
        winner = grid[i][0];
        return winner;
    }
    if((grid[0][i] == grid[1][i]) && (grid[1][i] == grid[2][i])){
        winner = grid[0][i];
        return winner;
    }

   }

   if((grid[0][0] == grid[1][1]) && (grid[1][1] == grid[2][2])){
        winner = grid[0][0];
        return winner;
    }

   if((grid[0][2] == grid[1][1]) && (grid[1][1] == grid[2][0])){
        winner = grid[0][2];
        return winner;
   }
   //Check if the game is a tie

   if (movesMade == totalTiles){
    winner = TIE;
   }
   return winner;
}

Pretty much it is to check the winner of a 3x3 tictactoe game. — ron8, May 14 '11 at 15:16
that's a silly check to run.. the code looks pretty readable to me.. — Claudiu, May 14 '11 at 15:24
looks readable, the only thing I can see to reduce the amount of if statements without harming readability is to capture them in a method and iterate over it instead if statements: `boolean checkWinner(GRID_TYPE squrare1,GRID_TYPE aquare2,GRID_TYPE square3)` which returns if there was a win (and sets this.winner). I am not sure I would have done it, or just leave it as is. — amit, May 14 '11 at 15:27

score 3 · Answer 1 · answered May 14 '11 at 15:25

3

I don't know how the checker works but how about this:

if(((grid[0][0] == grid[1][1]) && (grid[1][1] == grid[2][2])) || 
   ((grid[0][2] == grid[1][1]) && (grid[1][1] == grid[2][0]))) {
     winner = grid[1][1];
     return winner;
 }

If this does work, the irony of course is that this seems a little less readable than your code.

answered May 14 '11 at 15:25

Claudiu

224,032
165
485
680

Damn it doesn't work. It also checks the opperators. This is really annoying. Thanks for the help! – ron8 May 14 '11 at 15:29
1

This is not the same code as you assume the winner is always in [1][1] – Nicolas Bousquet May 14 '11 at 15:30
@Nicolas: the winner is always in [1][1] if one of those is true, as it checks the diagonals. – Claudiu May 14 '11 at 22:04

Martin Wickman · Accepted Answer · 2011-05-14T16:33:49.673

2

You could extract methods for checking rows and column and rewrite your code something like this:

public char checkWinner()
{    
   for (int i=0; i < GRIDSIZE; i++) {
       if (checkRow(i)) return winner;
       if (checkColumn(i)) return winner;    
   }

   if (checkDiagTopLeft()) return winner;
   if (checkDiagBottomLeft()) return winner;
}

Easier to read and less complexity.

Side note: Obviously, the winner stuff could use a redesign, but that was not part of the question and is left as an exercise for the reader (and commenters) if they feel like it.

edited May 14 '11 at 16:33

answered May 14 '11 at 15:35

Martin Wickman

19,662
12
82
106

1

This exactly the type of thing that should be done, but modifying the member field winner in the submethod and returning in the outer is like of a code smell in my opinion. – Nicolas Bousquet May 14 '11 at 15:39
Using helper methods is the best way. I like your thinking!! – ron8 May 14 '11 at 15:41

score 1 · Answer 3 · answered May 14 '11 at 15:31

The solution is already up there (combining the if statements), but I would not let Cyclomatic Complexity dictate my coding if the code of a method fits on a single page. The measure you want to aim for in a big project is readability and ease of understanding. Remember that code will be written potentially only once, but read quite a few times.

Thomas Jung · Answer 4 · 2011-05-14T16:28:37.747

The first step can be to remove some redundancy from the equal expression. The allEqual makes the intent a bit clearer.

Assinging the winner to a field is strange. I've removed that in the refactoring. If you really need the assignment you could do it in a separate method calling checkWinner. The problem with returning and assigning is that it's unexpected for a caller to have this side effect.

public char checkWinner() {
    // Check if the game has a win
    for (int i = 0; i < GRIDSIZE; i++) {
        if (allEqual(grid[i][0], grid[i][1], grid[i][2])) return grid[i][0];
        if (allEqual(grid[0][i], grid[1][i], grid[2][i])) return grid[0][i];
    }

    if (allEqual(grid[0][0], grid[1][1], grid[2][2])) return grid[0][0];
    if (allEqual(grid[0][2], grid[1][1], grid[2][0])) return grid[0][2];

    // Check if the game is a tie
    int totalTiles = GRIDSIZE * GRIDSIZE;
    return movesMade == totalTiles ? TIE : BLACK;
}

private boolean allEqual(char... c) {
    for(int i=1;i<c.length;i++) if(c[i-1] != c[i]) return false;
    return true;
}

Open Problems:

The char[][] array is not the most efficient data structure to represent the board. You could use a BitSet.
You defined GRIDSIZE constant but you're could would break down if you actually changed it from 3 to another value.
You can use the fact that checking row/columns and diagonals is symmetric. The parameters have to be transposed use this.

Using the GRIDSIZE constant you do not have to address all cells explicitly:

public char checkWinner() {
    // Check if the game has a win
    for (int i = 0; i < GRIDSIZE; i++) {
        if (rowEqual(i)) return grid[i][0];
        if (columnEqual(i)) return grid[0][i];
    }

    if (diagonalLeftToRightEqual()) return grid[0][0];
    if (diagonalRightToLefttEqual()) return grid[0][GRIDSIZE];

    // Check if the game is a tie
    int totalTiles = GRIDSIZE * GRIDSIZE;
    return movesMade == totalTiles ? TIE : BLACK;
}

private boolean rowEqual(int r) {
    for(int i=1;i<GRIDSIZE;i++) if(grid[r][i-1] != grid[r][i]) return false;
    return true;
}

private boolean diagonalLeftToRightEqual() {
    for(int i=1;i<GRIDSIZE;i++) if(grid[i-1][i-1] != grid[i][i]) return false;
    return true;
}

score -1 · Answer 5 · answered May 14 '11 at 15:26

-1

Cyclometric complexity is a measure of the number of paths through your code. Your function is composed almost exclusively of if statements.

You can combine two or more if statements with or:

if(a)
  do_something();
if(b)
  do_something();

Should be replaced by:

if(a || b)
  do_something();

answered May 14 '11 at 15:26

edoloughlin

5,821
4
32
61

Cyclomatic complexicty do inclure or/and etc into account otherwise the global cyclomatic complexity would no be 11. – Nicolas Bousquet May 14 '11 at 15:32

How can I remove one of these if statements and shorten the code?

5 Answers5