Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
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.

share|improve this question
2  
Hint why checking for sum == 45 is not sufficient: (1+9)+(2+8)+(3+7)+(4+6)+5 is OK, but (1+9)+(1+9)+(1+9)+(1+9)+5 also fulfills that constraint! –  amon Apr 2 at 12:08
    
@amon For lack of ability to think straight at the moment, is it possible to simultaneously fulfill the 45 sum on all rows and columns and mini-grids using numbers other than 1-9 once each? –  Niet the Dark Absol Apr 2 at 14:35
2  
Fill it all with 5. It sums 45 everywhere, but it is obviously not correct. –  Davidmh Apr 2 at 15:01
2  
I'd just store all possible Sudokus in a list. That way you only need to figure out how to generate every possible Sudoku and then develop an algorithm to walk the list and check equality. :^) –  helrich Apr 2 at 16:27
    
@helrich Clever. :) –  asteri Apr 2 at 16:48

3 Answers 3

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.

share|improve this answer
1  
Your variables iterator and columnIterator are very weird and hide their actual meaning – either use i, j, row, column, or row, col. –  amon Apr 2 at 12:06
    
@amon row an column are taken, also I don't understand how you mean "hide their actual meaning", but I edited either way ;) –  Vogel612 Apr 2 at 12:08
    
(There are no java.util.Iterators here, but you have integer indices) –  amon Apr 2 at 12:12
    
@amon I fully ignored that. not using Iterator much, thanks for the hint ;) –  Vogel612 Apr 2 at 12:12
    
Is there an example of an invalid grid which passes the row and column tests but fails the square test? –  Edward Apr 2 at 12:52

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];
   }
}
share|improve this answer
    
@uri....Thanks for your feedback...How did you come up with this int boxIndex = i % 3 * 3 + j % 3 ? –  user3296744 Apr 2 at 18:27
    
Sorry, my mistake - it should be /, not % (modulo) - I fixed my answer... –  Uri Agassi Apr 2 at 19:20
    
Thanks for your feedback! –  user3296744 Apr 3 at 17:24
    
In many cases I would also suggest moving the functionality out of the class and into its instances, although that doesn't really apply in this case. –  AJMansfield Apr 8 at 15:48

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.

share|improve this answer
    
@simon.....Yes i agree to what you said! –  user3296744 Apr 3 at 5:25
    
:Instead of using break and a boolean sudokuStatus in your checkArrayStatus method, use a return statement....Can you tell me why this is prefered over a booleanStatus? –  user3296744 Apr 3 at 6:28
    
:since the above loop is just doing a fixed-length iteration, replace while (i < 9) with for (int i = 0; i < 9; i++)...why is this prefered over a normal for loop? –  user3296744 Apr 3 at 6:29

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.