patternjavaMinor
Alternative methods of solving Sudoku
Viewed 0 times
methodssolvingsudokualternative
Problem
I am learning Java as my first programming language and have written a bit of code. I would love to read expert reviews on this code and suggestions to improve right from naming conventions to logic. Please let me know if there is any kind of mistake.
```
public class Sudoku {
public static void main(String[] args) {
long startTime = System.currentTimeMillis();
int[][] array = { { 0, 2, 0, 5, 0, 0, 0, 9, 0 },
{ 5, 0, 0, 0, 7, 9, 0, 0, 4 },
{ 3, 0, 0, 0, 1, 0, 0, 0, 0 },
{ 6, 0, 0, 0, 0, 0, 8, 0, 7 },
{ 0, 7, 5, 0, 2, 0, 0, 1, 0 },
{ 0, 1, 0, 0, 0, 0, 4, 0, 0 },
{ 0, 0, 0, 3, 0, 8, 9, 0, 2 },
{ 7, 0, 0, 0, 6, 0, 0, 4, 0 },
{ 0, 3, 0, 2, 0, 0, 1, 0, 0 } };
solve(array);
System.out.println("Solution is ");
for (int row = 0; row < 9; row++) {
for (int col = 0; col < 9; col++) {
System.out.print(" " + array[row][col] + " ");
}
System.out.println();
}
long endTime = System.currentTimeMillis();
long totalTime = endTime - startTime;
System.out.println("Time taken (in MilliSeconds)= " + totalTime);
}
public static boolean solve(int[][] array) {
if (isSum45(array)) {
return true;
}
for (int row = 0; row < 9; row++) {
for (int col = 0; col < 9; col++) {
if (array[row][col] == 0) {
for (int num = 1; num <= 9; num++) {
if (noConflict(array, row, col, num)) {
array[row][col] = num;
if (solve(array)) {
return true;
}
}
array[row][col] = 0;
}
```
public class Sudoku {
public static void main(String[] args) {
long startTime = System.currentTimeMillis();
int[][] array = { { 0, 2, 0, 5, 0, 0, 0, 9, 0 },
{ 5, 0, 0, 0, 7, 9, 0, 0, 4 },
{ 3, 0, 0, 0, 1, 0, 0, 0, 0 },
{ 6, 0, 0, 0, 0, 0, 8, 0, 7 },
{ 0, 7, 5, 0, 2, 0, 0, 1, 0 },
{ 0, 1, 0, 0, 0, 0, 4, 0, 0 },
{ 0, 0, 0, 3, 0, 8, 9, 0, 2 },
{ 7, 0, 0, 0, 6, 0, 0, 4, 0 },
{ 0, 3, 0, 2, 0, 0, 1, 0, 0 } };
solve(array);
System.out.println("Solution is ");
for (int row = 0; row < 9; row++) {
for (int col = 0; col < 9; col++) {
System.out.print(" " + array[row][col] + " ");
}
System.out.println();
}
long endTime = System.currentTimeMillis();
long totalTime = endTime - startTime;
System.out.println("Time taken (in MilliSeconds)= " + totalTime);
}
public static boolean solve(int[][] array) {
if (isSum45(array)) {
return true;
}
for (int row = 0; row < 9; row++) {
for (int col = 0; col < 9; col++) {
if (array[row][col] == 0) {
for (int num = 1; num <= 9; num++) {
if (noConflict(array, row, col, num)) {
array[row][col] = num;
if (solve(array)) {
return true;
}
}
array[row][col] = 0;
}
Solution
Overall, this is pretty good code for a beginner's solution to a relatively complex problem.
But your code does have me a little lost. For one, I'm not quite sure why you're using recursion here. It seems like that will be a lot more memory intensive than it needs to be for a straightforward problem. It also confused me for a good ten minutes why you were checking for
I would rewrite this as an iterative solution rather than a recursive solution. I think the recursion adds a lot of unnecessary complexity to the code and flow. I don't have the time at the moment to rewrite it as such, so I'll just give some other general comments on your code.
I'm not a big fan of the above implementation because it relies on the
On a positive note, I really like that you named your loop variables
This makes your code a bit less complex and a bit more readable. I completely avoid your
This is just general nitpicking, but you should only expose methods as
Also, in those two methods, what happened to your beautiful variable names?!? Why did you suddenly move to using single letters instead of something more descriptive?
But your code does have me a little lost. For one, I'm not quite sure why you're using recursion here. It seems like that will be a lot more memory intensive than it needs to be for a straightforward problem. It also confused me for a good ten minutes why you were checking for
isSum45 to return that the problem is solved, when the sum for the entire board is actually 405.I would rewrite this as an iterative solution rather than a recursive solution. I think the recursion adds a lot of unnecessary complexity to the code and flow. I don't have the time at the moment to rewrite it as such, so I'll just give some other general comments on your code.
solve(array);
System.out.println("Solution is ");
for (int row = 0; row < 9; row++) {
for (int col = 0; col < 9; col++) {
System.out.print(" " + array[row][col] + " ");
}
System.out.println();
}I'm not a big fan of the above implementation because it relies on the
solve() method modifying the original array in place. It also makes your paradigm of returning a boolean from the solve() method irrelevant; you never compare or check for it. I think it would be a better design if you preserve the original board and create a new int[][] solved variable which you return from the method.for (int row = 0; row < 9; row++) {
for (int col = 0; col < 9; col++) {
if (array[row][col] == 0) {
for (int num = 1; num <= 9; num++) {
if (noConflict(array, row, col, num)) {
array[row][col] = num;
if (solve(array)) {
return true;
}
}
array[row][col] = 0;
}
return false;
}
}
}On a positive note, I really like that you named your loop variables
row and col versus i and j as so many people do. It makes your code much more readable. But one of the principles of good code design is to avoid this kind of intense nesting as much as possible. You can do this like the following:for(int row = 0; row < array.length; row++) {
for(int column = 0; column < array[row].length; column++) {
if(array[row][column] != 0) continue;
for(int num = 1; num <= 9; num++) {
if(!noConflict(array, row, column, num)) continue;
array[row][column] = num;
if(solve(array)) return true;
array[row][column] = 0
}
return false;
}
}This makes your code a bit less complex and a bit more readable. I completely avoid your
if(solve(array)) block by simply returning the boolean directly from the recursive method call (EDITED: As David Harkness says in the comments below, this actually changes how the method functions, so I've removed that bit.). Also note that it my loops I compare row and column to array.length and array[row].length, respectively. This isn't a huge deal if you always know you're going to be handling boards which are 9x9, however when you're designing code you should always try to make your code as flexible as possible. The more flexible it is and the more scenarios it can handle, the more powerful it is.public static boolean noConflict(int[][] array, int row, int col, int num) {This is just general nitpicking, but you should only expose methods as
public which actually need to be public. The solve method makes sense as a public method because it's the part of the API that you will want to use in other parts of the application. This method, however, is purely internal and thus should be private. Also, method names should pretty much always be verbs or verb phrases. I might rename this method to something like noConflictExists or isNoConflict. Actually, because of the nature of how I rewrote your loops above, I would change it to conflictExists or isConflict that way my usage of it is more intuitive. These notes also apply to your isSum45 method, which is public and might use a more intuitive name for code readability.Also, in those two methods, what happened to your beautiful variable names?!? Why did you suddenly move to using single letters instead of something more descriptive?
Code Snippets
solve(array);
System.out.println("Solution is ");
for (int row = 0; row < 9; row++) {
for (int col = 0; col < 9; col++) {
System.out.print(" " + array[row][col] + " ");
}
System.out.println();
}for (int row = 0; row < 9; row++) {
for (int col = 0; col < 9; col++) {
if (array[row][col] == 0) {
for (int num = 1; num <= 9; num++) {
if (noConflict(array, row, col, num)) {
array[row][col] = num;
if (solve(array)) {
return true;
}
}
array[row][col] = 0;
}
return false;
}
}
}for(int row = 0; row < array.length; row++) {
for(int column = 0; column < array[row].length; column++) {
if(array[row][column] != 0) continue;
for(int num = 1; num <= 9; num++) {
if(!noConflict(array, row, column, num)) continue;
array[row][column] = num;
if(solve(array)) return true;
array[row][column] = 0
}
return false;
}
}public static boolean noConflict(int[][] array, int row, int col, int num) {Context
StackExchange Code Review Q#39337, answer score: 4
Revisions (0)
No revisions yet.