Sudoku Checker in Java

Question

public class SudokuChecker{ 

static int[][] sMatrix={

                    {5,3,4,6,7,8,9,1,2},
                    {6,7,2,1,9,5,3,4,8},
                    {1,9,8,3,4,2,5,6,7},
                    {8,5,9,7,6,1,4,2,3},
                    {4,2,6,8,5,3,7,9,1},
                    {7,1,3,9,2,4,8,5,6},
                    {9,6,1,5,3,7,2,8,4},
                    {2,8,7,4,1,9,6,3,5},
                    {3,4,5,2,8,6,1,7,9}
                };
static int rSum=0;

static int cSum=0;

static int[] rSumArray=new int[9];

static int[] cSumArray=new int[9];

static int[] boxSumArray=new int[9];

static boolean checkArrayStatus(int[] rSumArray,int[] cSumArray,int[] boxSumArray)
{   
    int i=0;

    boolean sudukoStatus=true;

    while(i<9){
        if(rSumArray[i]!=45&&cSumArray[i]!=45&&rSumArray[i]!=45)
        {   
            sudukoStatus=false;
            break;
        }
        i++;
    }
    return sudukoStatus;
}   

    public static void main(String[] args) {
        for(int i=0 ; i<sMatrix.length ; i++){
            for(int j=0 ; j<sMatrix.length ; j++){
                rSum+=sMatrix[i][j];
                cSum+=sMatrix[j][i];
                }
            rSumArray[i]=rSum;
            cSumArray[i]=cSum;
            rSum=0;
            cSum=0;
        }

        for(int i=0 ; i< sMatrix.length ; i++){
            for(int j=0 ; j<sMatrix.length ; j++){
                if(i<=2&&j<=2)
                {
                    boxSumArray[0]+=sMatrix[i][j];
                }
                if(i<=2&&(j>=3&&j<=5))
                {
                    boxSumArray[1]+=sMatrix[i][j];
                }
                if(i<=2&&(j>=6&&j<=8))
                {
                    boxSumArray[2]+=sMatrix[i][j];
                }
                if((i>=3&&i<=5)&&(j<=2))
                {
                    boxSumArray[3]+=sMatrix[i][j];
                }
                if((i>=3&&i<=5)&&(j>=3&&j<=5))
                {
                    boxSumArray[4]+=sMatrix[i][j];
                }
                if((i>=3&&i<=5)&&(j>=6&&j<=8))
                {
                    boxSumArray[5]+=sMatrix[i][j];

                }
                if((i>=6)&&(j<=2))
                {
                    boxSumArray[6]+=sMatrix[i][j];
                }
                if((i>=6)&&(j>=3&&j<=5))
                {
                    boxSumArray[7]+=sMatrix[i][j];
                }
                if((i>=6)&&(j>=6))
                {
                    boxSumArray[8]+=sMatrix[i][j];
                }
            }
        }

        if(checkArrayStatus(rSumArray,cSumArray,boxSumArray))
        {
            System.out.println("The matrix is sudoku compliant");
        }
        else
        {
            System.out.println("The matrix is not sudoku compliant");
        }
    }
}

This a program which verifies if a 2D matrix is a Sudoku or not. The sum of every row, column and 3x3 matrices have to be 45. Please review the code and provide feedback on best practices and code optimization.

Answer 1

You have waaaay too many sums you are checking. KISS-Principle --> Keep it Simple {and} Stupid:

private boolean checkSudokuStatus(int[][] grid) {
    for (int i = 0; i < 9; i++) {

        int[] row = new int[9];
        int[] square = new int[9];
        int[] column = grid[i].clone();

        for (int j = 0; j < 9; j ++) {
            row[j] = grid[j][i];
            square[j] = grid[(i / 3) * 3 + j / 3][i * 3 % 9 + j % 3];
        }
        if (!(validate(column) && validate(row) && validate(square)))
            return false;
    }
    return true;
}

private boolean validate(int[] check) {
    int i = 0;
    Arrays.sort(check);
    for (int number : check) {
        if (number != ++i)
            return false;
    }
    return true;
}

Short Explanation:

1. A Sudoku has three elements to validate. Each has to contain the single digit numbers from 1 to 9 exactly once.

2. These three elements are: each single row, each single column, the 9 3x3 sqares.
   These elements are exactly mapped by the three variables you see in the for-loop.

3. Each of these elements is given to a validate function to verify, that it contains the numbers only once. Originally this was a golfed solution, so the square calculation is heavily algorithmic.

4. The validate function is quite simple. It initializes a counter to 0, and then walks the sorted "validated element" through, while always incrementing by 1. this gives the numbers from 1 to 9. In fact it is just a shortened for-loop, written a little different it's:

this one

for(int i = 1; i <= 9; i++){
    if(check[i-1] != i){
       return false;
    }
}
return true;

Longer Explanation

How I got to the algorithmic generation for the squares:

[(i / 3) * 3 + j / 3]      [i * 3 % 9 + j % 3]

---

(i / 3) * 3 + j / 3

This part is relatively simple. It gives the correct "column" by help of implicit integer conversion: The first part of the equation is a little difficult:

\$ (i / 3) \$
\$ {0,1,2} \mapsto 0 \$
\$ {3,4,5} \mapsto 1 \$
\$ {6,7,8} \mapsto 2 \$

take it * 3 and you got the sqare you want to get in to. then if we now iterate row-wise, we need to jump to the next column every 3 Elements. That's done with j / 3.

Now for the difficult one:

 i * 3 % 9 + j % 3

\$ (i * 3 \% 9) \$
\$ {0,3,6} \mapsto 0 \$
\$ {1,4,7} \mapsto 3 \$
\$ {2,5,8} \mapsto 6 \$

This one is for "jumping" the sqares vertically. Then we just have to iterate 0, 1, 2. This is accomplished by running the % 3 operation.

Answer 2

Mind your scope
You have a lot of static members (cSum, rSum, etc.), which you calculate in one method, and the pass them as arguments to other methods. This means that they should be members at all - they should be declared locally, and not be exposed outside the method. This way you can be sure no-one else is changing them when you are not looking...

What should a main method do?
Your main method contains a lot of functionality. I suggest you move this functionality to another method (checkBoard()). This way, when you decide to take this code, and use it as a module in another application, you can simply do it!

Keep it DRY
As a programmer, you should have bells ringing in your head each time you find yourself using Copy+Paste. Say you find a bug in your code, now you have to fix it 9 times! and if you miss a fix somewhere - good luck finding where it is...
Take the similarities between your snippets, and refactor them out. Analyze the differences, and pass them as arguments. Find how you decide which argument values to pass, and develop an algorithm to make that decision for you:

for(int i=0 ; i< sMatrix.length ; i++){
   for(int j=0 ; j<sMatrix.length ; j++){
       int boxIndex = (i / 3) * 3 + j / 3;
       boxSumArray[boxIndex]+=sMatrix[i][j];
   }
}

Answer 3

Just a couple of minor things:

• In Java, { braces should be put at the end of the same line and not on it's own line.

• Instead of using break and a boolean sudokuStatus in your checkArrayStatus method, use a return statement

  while (i < 9) {
      if (rSumArray[i] != 45 && cSumArray[i] != 45 && rSumArray[i] != 45) {   
          return false;
      }
      i++;
  }
  return true;
  

• Since the above loop is just doing a fixed-length iteration, replace while (i < 9) with

  for (int i = 0; i < 9; i++)
  

• 9 is a "magic number" in your code. Let's say you wanted to change the Sudoku grid size (there are 4x4 Sudokus as well, in fact, almost any size is possible). If you wanted to change the size, you'd have to replace the value in many different places. Instead use a constant.

  public static final int GRID_SIZE = 9;
  

  Now, instead of writing 9 you can use GRID_SIZE

And one major thing:

Just because some numbers sum up to 45 doesn't mean that it's valid in a Sudoku group!

Consider these numbers: 1, 1, 1, 1, 5, 9, 9, 9, 9. Do they some up to 45? Yes. Would it be a valid group/row/column in a Sudoku? NO! This is why @Vogel612's solution is a whole lot better.