Entry-level recursive Sudoku solver

I've been asked to attach a C++ code sample to my application for an entry-level C++ programmer position in a videogame company. I've created a simple, yet complete console application to solve Sudoku problems. How can I improve the code that I have?

Please do comment on the algorithm, the code practices, presentation, or anything else. It's one of my first attempts to also set up unit tests with my code.

Also, I am interested in whether you think this would work well as a code sample in particular – i.e. does it show what people who look at the code samples want to see?

Complete code

```
#include
#include

#include
#include
#include

// ======================================================================================

enum SolutionResult {

// The position has been solved OK.
SR_SOLVED,

// The current position in invalid.
// For that reason, it is useless to even attempt to solve it.
SR_INVALID,

// The current position is valid in its current state, but there
// exists no complete solution that would uphold the sudoku rules.
SR_UNSOLVABLE

};

// ======================================================================================

class Sudoku {

protected:

// 0 = Empty cell
short values[9][9];

// Returns whether all the rows are currently valid,
// i.e. whether no rows contain any duplicate values.
bool checkValidRows() {

for (short row = 0; row valuesInRow;
for (short col = 0; col values[row][col];
if (!val) continue;

if (valuesInRow.find(val) != valuesInRow.end()) {
return false;
}

valuesInRow.insert(val);

}

}

return true;

}

// Returns whether all the columns are currently valid,
// i.e. whether no columns contain any duplicate values.
bool checkValidCols() {

for (short col = 0; col valuesInCol;
for (short row

From a brief glance, I can see a number of issues with the code. I'm sorry if I am overly blunt, but it's better that you hear it from me than a recruiter.

In no particular order:

-
Use the language. Structure your code better. Consider using more classes to represent the board. Don't write a sudoku-solving program. Make it a tiny sudoku-solver library, and include a driver program to use that library. Write reusable code.

-
You have protected members of the class. This usually means the class is designed to be a base class. However, base classes should have a virtual destructor, but yours isn't. You probably want to change protected to private.

-
Make your solver handle boards with multiple solutions.

-
Your explicit use of this just creates clutter, in my opinion.

-
Your code is not exception safe. There is no way to test the state of the solver if solve() throws an exception, for example. You should have a function in the public interface to query the state of the solver.

-
Avoid excessive whitespace and comments.

-
Make member functions that don't mutate state const.

For example:

bool checkValidCols() const { /* ... */ }

-
Don't hard code sizes. Read your game board from a file. Accept variable-sized boards.

-
Prefer pre-increment to post-increment when pre-increment is what you want. You want to say "increment i by one", not "make a copy of i and then increment the original value by one". This is a good habit in general, because post-increment for user-defined types does make an extra, unnecessary copy.

-
Don't repeat yourself. checkValidRows and checkValidCols is more or less the same function. If you make a bit of an abstraction, the same goes for the square checking.

-
Consider using a one-dimensional array, and/or using one-dimensional or two-dimensional std::array or std::vector.

-
There are indentation issues in the code you have posted.

-
Your solving algorithm is possibly the slowest one I can think of. Do a quick google search and do something smarter.

-
You have very long functions defined inline. It gives the impression that you only know Java, but happen to be writing in C++. Move the implementation code out of the class definition, and structure your code into files.

-
Use a separate unit test library, for example googletest. Don't always run unit tests when the program runs.

-
Drop the "Press any key to exit" part. It looks very unprofessional.

-
Consider adding read-only access to your board tiles, so that your class can be used as a back-end for a GUI later.

-
Don't pass parameters to main if you are not going to use them.

-
Put a default in your switch to catch errors.

-
Use assertions to ensure pre- and post-conditions and invariants of functions during debugging.

-
Perform error handling, preferably using exceptions. Make your program very robust; you don't want CTD in a game.

-
Have many small unit tests that test exactly a single thing. If a test fails, you should know from its name what has failed. Look at an example of unit tests online.

-
Sort your #includes alphabetically.

-
Consider making your solver multi-threaded.

-
Remember to include the other important parts. Primarily: Documentation, and use a proper build system (make is enough for such a simple application). In the documentation, you can include a discussion of your solving algorithm, including analysis of its efficiency (asymptotic complexity/Big-O as a minimum.)

StackExchange Code Review Q#28746, answer score: 13