Skip to main content
Code Review

Return to Revisions

3 of 3
Corrected square number calculation
holroy
  • 11.8k
  • 1
  • 27
  • 59

There are flaws in your design, and room for improvements, so here are some comments to choice of coding algorithm:

  • Bug: columnCheck always return false – In columnCheck you set sum=0, so this should return false alway, that is if the badly named count (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.

holroy
  • 11.8k
  • 1
  • 27
  • 59
default

AltStyle によって変換されたページ (->オリジナル) /