5

I have been trying to figure out my mistake in the Sudoku backtracking solver for three days. The problem is from leetcode Sudoku Solver.

My solver is based on the deduction in the attached picture. The problem is that my board is changed even if a path from root to leaf is invalid.

In other words, after it goes through an invalid path, the values it attempted are fixed in my original board. However, I only update my original board when its children returns true ( see the part in helper method: // put a number and generate children).

Basically, for each '.', I generate all possibilities from 1 to 9, construct a temp board which fills the current '.' with one possibility, then call helper of the next '.' with temp board. I know it is not good to store a same size boardTemp for each possible child because of space cost, but my main concern now is to solve the problem first before optimizing the cost.

All in all, why is my board changing even if all children is not valid?

For example, base on the initial board

..9748...; 7........; .2.1.9...;

..7...24.; .64.1.59.; .98...3..;

...8.3.2.; ........6; ...2759..;

I got the final result after I run:

139748652; 745326189; 826159437;

35769.24.; .64.1.59.; .98...3..;

...8.3.2.; ........6; ...2759..; 

public void sudokuSolver(char[][] board) {
    for (int i = 0 ; i < board.length ; i++) {
        for (int j = 0 ; j < board.length ; j++) {
            if (board[i][j] == '.') {
                // find the first '.' as root
                helper(i, j, board);
                return;
            }
        }
    }
}

private boolean helper(int row, int col, char[][] board) {
    // case 2. check if it has following '.' and store its position
    boolean hasNext = false;
    boolean nextSearching = false;
    int nextRow = row;
    int nextCol = col;
    for (int i = 0 ; i < board.length ; i++) {
        for (int j = 0; j < board.length ; j++) {
            if (nextSearching && !hasNext && board[i][j] == '.') {
                hasNext = true; // there is next!
                nextRow = i;
                nextCol = j;
            }
            if (i == row && j == col) {
                nextSearching = true;
            }
        }
    }

    // exit condition: last '.'
    if (!hasNext) {
        for (char put = '1' ; put <= '9' ; put ++) {
            if (isValid(row, col, board, put)) {
                return true;
            }
        }
        return false;
    }

    // put a number and generate children
    for (char put = '1' ; put <= '9' ; put ++) {
        if (isValid(row, col, board, put)) {
            char[][] boardTemp = board;
            boardTemp[row][col] = put;
            boolean valid = helper(nextRow, nextCol, boardTemp);
            if (valid) {
                // board is supposed to change only when valid is true.
                board[row][col] = put;
                return true;
            }
        }
    }
    return false;
}

private boolean isValid(int row, int col, char[][] board, char c) {
    // go through each row, column, and subblock to determine if c is a valid choice based on current board.
    for (int jCol = 0 ; jCol < 9 ; jCol ++) {
        if (board[row][jCol] == c) {
            return false;
        }
    }
    for (int iRow = 0 ; iRow < 9 ; iRow ++) {
        if (board[iRow][col] == c) {
            return false;
        }
    }
    for (int i = row/3*3 ; i < row/3*3 + 3 ; i++) {
        for (int j = col/3*3 ; j < col/3*3 + 3 ; j++) {
            if (board[i][j] == c) {
                return false;
            }
        }
    }
    return true;
}

My tree for backtracking

Holger Just
  • 52,918
  • 14
  • 115
  • 123
Appalachian Math
  • 169
  • 1
  • 10
  • 3
    Side note: good OO programming is about creating useful abstractions. Like, you could model your board with "real" classes; instead of relying on a 2 dim char array. As a consequence of you using "low-level" abstractions; your code itself is pretty abstract. And your naming doesn't help. A name should say what is behind it. So, what is a method called *helper* doing? And why is it returning a boolean?! And then that method has at least 3 returns. Wtf?! Long story short: This code is hard to read and understand. It is no surprise that it contains bugs that are hard to spot! – GhostCat Aug 10 '16 at 06:52
  • 3
    In other words: you should read about the "single layer of abstraction" principle for example. You have **all** your code in your one helper method. Instead, you should have lots of small methods that do **one** thing each. And please note: if you had many smaller methods, you could also write small unit tests for each of them (and make no mistake: such assignments are **perfect** for using TDD and writing unit tests). In essence: try to dissect your problem into smaller ones; and solve those independently ... and test them. Then pull together the "big picture". – GhostCat Aug 10 '16 at 06:57
  • Thank you so much for your comment. The general method you mentioned should be really good for me to decompose a big problem into many small subproblems. Then I can figure out where it goes wrong. I may use it for some tough or harder problems. – Appalachian Math Aug 10 '16 at 07:13
  • @GhostCat But still, I found where my problem is by simply debugging. The problem I already described: why the board has changed even if I construct a temp board. Hopefully I would figure it out asap by checking comments and googling. – Appalachian Math Aug 10 '16 at 07:15
  • Glad to hear that. And to be precise: I am not saying that you have to do the things I listed in order to find and fix your problem. I am merely saying: do not only focus on "just solving assignments" when programming. So I am simply recommending to spent some of your time to look into such concepts as "clean code" or TDD. – GhostCat Aug 10 '16 at 08:10
  • @AppalachianMath Please don't add answers to your question itself. You can add your own answer to your question below. If you have further questions, please ask them in a separate question. Please be sure to add sufficient detail. – Holger Just Aug 10 '16 at 10:25

1 Answers1

4

There is no difference between using boardTemp and board. char[][] boardTemp = board means they point to the same memory... What you missed in your original code is the else part in after you put an invalid number: 

for (char put = '1' ; put <= '9' ; put ++) {
    if (isValid(row, col, board, put)) {
        char[][] boardTemp = board; // board and boardTemp are essentially the same thing
        boardTemp[row][col] = put;
        boolean valid = helper(nextRow, nextCol, boardTemp);
        if (valid) {
            // board is supposed to change only when valid is true.
            board[row][col] = put;
            return true;
        }
        // THIS IS WHAT YOU MISSED!!
        // if you don't reset the '.' back, your later backtrack will not work 
        // because you put an invalid number on your board and you will never find a solution
        boardTemp[row][col] = '.';
    }
}
Shawn Song
  • 301
  • 1
  • 9