Return to Answer

• 26.1k
• 13
• 47
• 114

• 26.1k
• 13
• 47
• 114

This is a nice first Code Review Question. It's not a simple hello world program and it took some real effort.

Use the Tools You Have
When developing new code in C++ always compile with -Wall switch. This switch provides additional compiler error checking and allows removal of possible bugs as early as possible. The compile can also treat all warnings as errors.

When I compiled the code I got the following warnings: ../src/SudokuSolver.cpp: In function ‘bool isNumeric(const string&)’: ../src/SudokuSolver.cpp:46:41: warning: comparison between signed and unsigned integer expressions [-Wsign-compare] for(int itr = 0; itr < input.length(); itr++){ ../src/SudokuSolver.cpp: In function ‘void strToUpper(std::string&)’: ../src/SudokuSolver.cpp:56:37: warning: comparison between signed and unsigned integer expressions [-Wsign-compare] for(int itr=0; itr<input.length(); itr++){ ^ ../src/SudokuSolver.cpp: In function ‘bool squareContainsNum(subSquare ()[9], const int&, const int&, const int&, const string&)’: ../src/SudokuSolver.cpp:208:1: warning: control reaches end of non-void function [-Wreturn-type] } ^ ../src/SudokuSolver.cpp: In function ‘bool rowContainsNum(subSquare ()[9], const int&, const int&, const string&)’: ../src/SudokuSolver.cpp:231:1: warning: control reaches end of non-void function [-Wreturn-type] } ^ ../src/SudokuSolver.cpp: In function ‘bool colContainsNum(subSquare (*)[9], const int&, const int&, const string&)’: ../src/SudokuSolver.cpp:254:1: warning: control reaches end of non-void function [-Wreturn-type] } ^

Type MisMatch
When using any C++ container type that has the function length(). the function returns type size_t. The type size_t is currently defined as unsigned int. Comparing or assigning type size_t to type int can lead to surprising results because a positive unsigned value may become negative when it is converted.

The loop should be recoded as

 for(size_t itr = 0; itr < input.length(); itr++){

Missing Header File
The function printf() is defined in stdio.h. My compiler reports printf() as an error because stdio.h is not included.

Using printf() instead of std::cout
The code already uses std::cin for input (this is a good thing since the standard C++ input and output std::cin and std::cout). This is another mismatch. If using printf() for ouput than use scanf() for input.

While printf() and scanf() are still accepted in C++ as long as the proper header files are included, they are not the standard methods for C++ input and output. Prefer std::cin and std::cout over scanf() and printf().

Magic Numbers
The term Magic Numbers is sometimes used when code contains numeric constants where a named constant is more readable and maintainable. A named constant in this case might be:

const int SUDOKU_PUZZLE_SIZE = 9;

It's much easier to maintain code when it only requires a one line edit. If I needed to change the puzzle size in this code, I would have to edit 44 lines rather than just 1 line.

Namespaces
Name spaces were invented to solve a very real problem in enterprise level programming. In the bad old days, libraries acquired from other sources might contain functions that that had the same name and variable types as the code one might be writing. Then we would have to change our function names so that they didn't collide with the library names. When a programmer includes:

using namespace std;

in their code they are allow possible collisions of functions from their code and different libraries they might be using. The code here is complicated enough that one might want to include libraries to do some of the work.

Most professional programmers will use std::cin, std::cout and std::vector rather than the using namespace std; statement.