How can I remove one of these if statements and shorten the code?
-
13-11-2019 - |
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;
}
Solution
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.
OTHER TIPS
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.
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.
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;
}
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();