What's wrong with my sudoku solver function?

Question

ok so i changed my function to a backtracking function (which i found online). It still reads from a file and inputs it in an array, the check function is working properly so i haven't changed that. If your wondering the following is the puzzle I'm trying to solve (where zeros are empty spaces).

0 5 0 0 2 0 0 7 0
7 2 0 4 0 3 0 0 0
9 0 0 0 5 0 6 2 0
0 0 5 0 8 6 0 0 0
1 0 0 0 4 0 0 0 8
0 0 0 2 3 0 4 0 0
0 9 3 0 1 0 0 0 2
0 0 0 3 0 2 0 4 6
0 8 0 0 0 0 0 1 0

// backtracking function
void Sudoku::solvePuzzle()
{
int x = 0;
int y = 0;
int r = 0;
bool back_flag;

while (r < 81) {
    back_flag = true; 
    x = r/9;
    y = r%9;

    for(int num = arr[x][y]; num < 10 && back_flag; num++) {

        if(check(x,y,num)) {
            arr[x][y] = num;
            back_flag=false;
            break;
        }
        else if(num >= 9) {
            arr[x][y] = 0;
        }
    }
    if(back_flag) {
        r--;
    }
    else {
        r++;
    }
}
}

What is supposed to happen in the loop? I don't see anything in the loop which would cause progress. But since I don't know what's supposed to make it progress, I can't say where the error is. (The obvious solution for Sudoku uses recursion and backtracking---in a function that is generally less than 10 lines.) — James Kanze, Nov 18 '11 at 08:47
You are implementing only one solving strategy? Even if that is implemented correctly, are you sure that your input sudokus are solvable with just that one strategy? You loop can only terminate if `zeros++` in your lower loop is executed. For that, `arr[r][c]` before your commented //cout must be executed. So I'd start my debugging there. @James: Be careful with using "obvious". If it'd be obvious user1053372 wouldn't need this question ;-) @user1053372: Your question might be better off at http://codereview.stackexchange.com/ — cfi, Nov 18 '11 at 08:58
@talonmies check is a function that checks and makes sure that there is only one number per row/col/3x3. — Sero Eskandaryan, Nov 18 '11 at 15:25

Doc Brown · Answer 1 · 2011-11-18T13:26:03.630

I don't know if these are the only errors, but at a first glance, I think instead of

    if (count == 1 && arr[r][c] == 0) {
         tempNum = num;
    }
    else {
        tempNum = 0;
    }
    count++;

it should be

    count++;
    if (count == 1) {
         tempNum = num;
    }

and instead of

    if (count == 1 && check(r, c, num) && arr[r][c] == 0) {
         arr[r][c] = tempNum;

it should be

    if (count == 1 && check(r, c, tempNum)) {
         arr[r][c] = tempNum;

or just

    if (count == 1 ) {
         arr[r][c] = tempNum;

since, when count==1, then check(r, c, tempNum) cannot be false, provided your checkfunction has no side effects.

By the way, the code gets much better readable if you organize it this way:

for (int r = 0; r < MAX_ROW; r++) {
   for (int c  = 0; c < MAX_COL; c++) {
       if(arr[r][c] != 0)
           continue;

           // ** no tests for arr[r][c] == 0 in this code block any more
           // ...
    }
}

And one last thing: you should stop the algorithm if the number of zeros in the outer loop does not change any more from one iteration to the next, there will be SuDoKus your solver cannot solve, and you don't want to get you program into an endless loop for those, I guess.

What's wrong with my sudoku solver function?

1 Answers1