There are flaws in your design, and room for improvements, so here are some comments to choice of coding algorithm:
- Bug:
columnCheck
always returnfalse
– IncolumnCheck
you setsum=0
, so this should return false alway, that is if the badly namedcount
(which actually is a sum, and not a count) has increased it value during the loop - Feature: Relying on the sum to be 45 – As indicated in comments, there are multiple combinations of numbers which gives the sum 45. I.e. \1ドル + 2 + 4 + 4 + 5 + 6 + 6 + 8 + 9\$ or \$ -9 + -8 + -7 + -6 + -5 + -4 + -3 + -2 + 89\$. The first would fail on non-unique numbers, and the second could be countered limiting to the numbers 1 through 9.
- Inefficient loop constructions – Your code loops 5 times through the double loop from 1 through 9 (in the standard case). Using a little more memory, you could loop through the double loop once, and still find the same solution.
Alternate solution
My suggestion is to use 3 * size
boolean arrays of size
dimension to keep track of whether all numbers have been visited. In the standard case with size=9
, there would be 9 arrays for keeping track of the rows we're checking, 9 arrays for the columns, and 9 arrays for the squares within the board.
Instead of using the possibly expensive HashSet
or the slightly unreliable sum of 45 for checking uniqueness, I suggest to check the value to be in the correct range, and that corresponding value cell in either of corresponding row, column or square has not been visited before.
Here is a tested implementation of this algorithm:
public boolean isValid() {
boolean[][] row = new boolean[size][size];
boolean[][] column = new boolean[size][size];
boolean[][] square = new boolean[size][size];
int value, square_no;
for (int x = 0; x < size; x++) {
for (int y = 0; y < size; y++) {
value = board[x][y] - 1;
// Check for valid values
if ((value < 0) || (value >= size)) {
return false;
}
// Calculate the square number using mod and div
// NB! This should be generalised somewhat...
square_no = (x / 3) + (y / 3) * 3;
// Check if this value has been seen in row, column or square
if (row[y][value] ||
column[x][value] ||
square[square_no][value] ) {
return false;
} else {
// if not, mark it as seen
row[y][value] = true;
column[x][value] = true;
square[square_no][value] = true;
}
}
}
return true;
}
This should replace most of your methods, whilst still providing the correct result on whether the sudoku has been solved correctly or not.
- 11.8k
- 1
- 27
- 59