patterncppMinor
Sudoku Solver in C++ weekend challenge
Viewed 0 times
solverchallengesudokuweekend
Problem
I am very new to programming (using C++) and coded a sudoku solver as a weekend challenge. I guess there are so many things to criticize, but I would really appreciate some constructive criticism. Just what strikes you the most. If you want to run the code, a "sudoku.txt" has to be in the same directory. It's content are 9 lines with 9 numbers, representing the whole field. Zeroes are empty fields.
Here some brief explanation of what's going on:
The field is represented by an 1D int array with 81 elements. Used fields and left possibilities are counted throughout. There is also another array with 81 elements, but those are arrays themselves with 9 elements. They represent the possible numbers for every block. 0-8 is mapped to the numbers 1-9. When the value at [5] is 1, that means that the number 5+1 is a possible number for that block. In the last array there are attributes for each block.
When reading the file, the possible numbers for every block are calculated. The recursive function first checks if it is already finished or can't be finished. Then it looks for the block with the least possibilities (best case 1 possibility). Then the function is called recursively as many times as there are possibilities for this block. If it returns the solution, that's the field we want.
I compared the runtime to other implementations
http://www.sanfoundry.com/cpp-program-solve-sudoku-problem-backtracking/ and
Sudoku Solver in C
and figured that my one is quite fast. Am I missing something or is my one actually okayish fast?
Please no flame answers :)
```
#include
#include
#include
#include
#include
#include
// SUDOKU SOLVER v1.337
struct blockAttributes
{
int row, column, box, possibilities = 0;
};
struct sudokuField
{
int usedFields{ 0 };
int possibilities{ 0 };
int values[9 * 9];
int possField[9 * 9][9];
blockAttributes attributeField[9 * 9];
};
void calcPossMove(int x, int y, sudokuField &field)
{
int i{ x + y * 9 };
if (fi
Here some brief explanation of what's going on:
The field is represented by an 1D int array with 81 elements. Used fields and left possibilities are counted throughout. There is also another array with 81 elements, but those are arrays themselves with 9 elements. They represent the possible numbers for every block. 0-8 is mapped to the numbers 1-9. When the value at [5] is 1, that means that the number 5+1 is a possible number for that block. In the last array there are attributes for each block.
When reading the file, the possible numbers for every block are calculated. The recursive function first checks if it is already finished or can't be finished. Then it looks for the block with the least possibilities (best case 1 possibility). Then the function is called recursively as many times as there are possibilities for this block. If it returns the solution, that's the field we want.
I compared the runtime to other implementations
http://www.sanfoundry.com/cpp-program-solve-sudoku-problem-backtracking/ and
Sudoku Solver in C
and figured that my one is quite fast. Am I missing something or is my one actually okayish fast?
Please no flame answers :)
```
#include
#include
#include
#include
#include
#include
// SUDOKU SOLVER v1.337
struct blockAttributes
{
int row, column, box, possibilities = 0;
};
struct sudokuField
{
int usedFields{ 0 };
int possibilities{ 0 };
int values[9 * 9];
int possField[9 * 9][9];
blockAttributes attributeField[9 * 9];
};
void calcPossMove(int x, int y, sudokuField &field)
{
int i{ x + y * 9 };
if (fi
Solution
I see a number of things that could help you improve your program.
Fix the initialization
There is a subtle but real bug lurking in this code. The code contains this structure:
Because you have specified initialization values for
The problem is that the
It's a subtle change, but the C++ standard will now interpret this to zero-initialize each member of the array, fixing this particular problem. However, as I mention later, I think that this could be better handled by turning this into a
Sanitize user input
If the input file does not exist or is malformed, the program crashes. There are a number of ways that this could be fixed. First is that any character other than 1-9 could simply considered empty or zero. This could be inserted into
For the problem of a missing, one simple way to address that problem would be to more fully initialize the
This much will prevent it from crashing, but it will produce an obviously wrong result:
Next is the possibility of a short line. Because the code reads each line into a string and then indexes into it, if there aren't 9 characters on a line, this will access off the end of the string and possibly crash.
Lastly, there is the problem of a grid like this one:
That will cause the program to infinitely recurse until the stack is exhausted and again, the program crashes.
Use objects
Most of your functions operate ono a
However, this still leaves us with the obviously wrong result as above because the values are not all initialized to coherent values. We could fix that by adding a constructor:
Eliminate "magic numbers"
There are many numbers in the code, such as
Consider simplifying data structure
The values of
This stores the number of possib
Fix the initialization
There is a subtle but real bug lurking in this code. The code contains this structure:
struct sudokuField
{
int usedFields{ 0 };
int possibilities{ 0 };
int values[9 * 9];
int possField[9 * 9][9];
blockAttributes attributeField[9 * 9];
};Because you have specified initialization values for
usedFields and possibilities, those will both recieve values, but the other fields will be uninitialized. This does not pose a problem in this code except for possField. Specifically, within the solveSudoku routine, there is a conditional statment:if (field.possField[index][t] == 1) {
lastTry = t;
break;
}The problem is that the
possField is being examined but may never have been initialized to any particular value. This is a bug which should be fixed. There are many ways to address the problem, but the simplest is to simply initialize the field:int possField[9 * 9][9]{}; // add default initializationIt's a subtle change, but the C++ standard will now interpret this to zero-initialize each member of the array, fixing this particular problem. However, as I mention later, I think that this could be better handled by turning this into a
class instead of a simple struct.Sanitize user input
If the input file does not exist or is malformed, the program crashes. There are a number of ways that this could be fixed. First is that any character other than 1-9 could simply considered empty or zero. This could be inserted into
readSudoku:if (line[x] '9') {
line[x] = '0';
}For the problem of a missing, one simple way to address that problem would be to more fully initialize the
sudokuField structure:struct sudokuField
{
int usedFields{ 0 };
int possibilities{ 0 };
int values[9 * 9]{};
int possField[9 * 9][9]{};
blockAttributes attributeField[9 * 9]{};
};This much will prevent it from crashing, but it will produce an obviously wrong result:
1 2 3|4 5 6|7 8 9
0 0 0|1 1 1|1 1 1
0 0 0|1 1 1|1 1 1
=================
0 1 1|1 1 1|1 1 1
0 1 1|1 1 1|1 1 1
0 1 1|1 1 1|1 1 1
=================
0 1 1|1 1 1|1 1 1
0 1 1|1 1 1|1 1 1
0 1 1|1 1 1|1 1 1Next is the possibility of a short line. Because the code reads each line into a string and then indexes into it, if there aren't 9 characters on a line, this will access off the end of the string and possibly crash.
Lastly, there is the problem of a grid like this one:
1...1....
.........
.........
.........
.........
.........
.........
.........
.........That will cause the program to infinitely recurse until the stack is exhausted and again, the program crashes.
Use objects
Most of your functions operate ono a
sudokuField structure and wouldn't make much sense without it. This strongly suggests that the functions would be better implemented as member functions of a class named sudokuField. One way to do that would be to simply convert the functions you have. One way to do that might look like this:class sudokuField
{
public:
void read(std::string fileName);
sudokuField solve(int x=-1, int y=-1, int n=-1) const;
void printField() const;
protected:
void calcPossMove(int x, int y);
void updatePoss(int x, int y, int n);
void addNumberAt(int x, int y, int n);
private:
int usedFields{ 0 };
int possibilities{ 0 };
int values[9 * 9]{};
int possField[9 * 9][9]{};
blockAttributes attributeField[9 * 9]{};
};However, this still leaves us with the obviously wrong result as above because the values are not all initialized to coherent values. We could fix that by adding a constructor:
sudokuField() :
usedFields{},
possibilities{9*9},
values{},
possField{},
attributeField{}
{
int i=0;
for (int y = 0; y < 9; ++y) {
for (int x = 0; x < 9; ++x, ++i) {
attributeField[i].row = y;
attributeField[i].column = x;
attributeField[i].box = x/3 + 3*(y/3);
}
}
}Eliminate "magic numbers"
There are many numbers in the code, such as
3 and 9 that have a specific meaning in their particular context. By using named constants such as BlocksPerRow or Dimension, the program becomes easier to read and maintain. For cases in which the constant only has sense with respect to a particular object, consider making that constant part of the object.Consider simplifying data structure
The values of
row, column and box within the blockAttributes structure are always the same for every puzzle. Further, everyplace they're referenced, we already have the x and y values and can easily calculate box on the fly. For that reason, the blockAttributes structure can be completely eliminated and the field in the sudokuField class could be int possCount[9 * 9]{};This stores the number of possib
Code Snippets
struct sudokuField
{
int usedFields{ 0 };
int possibilities{ 0 };
int values[9 * 9];
int possField[9 * 9][9];
blockAttributes attributeField[9 * 9];
};if (field.possField[index][t] == 1) {
lastTry = t;
break;
}int possField[9 * 9][9]{}; // add default initializationif (line[x] < '0' || line[x] > '9') {
line[x] = '0';
}struct sudokuField
{
int usedFields{ 0 };
int possibilities{ 0 };
int values[9 * 9]{};
int possField[9 * 9][9]{};
blockAttributes attributeField[9 * 9]{};
};Context
StackExchange Code Review Q#111568, answer score: 5
Revisions (0)
No revisions yet.