8
\$\begingroup\$

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 <set>
#include <string>

#include <fstream>
#include <iostream>
#include <cassert>

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

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 < 9; row++) {

            std::set<short> valuesInRow;
            for (short col = 0; col < 9; col++) {

                short val = this->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 < 9; col++) {

            std::set<short> valuesInCol;
            for (short row = 0; row < 9; row++) {

                short val = this->values[row][col];
                if (!val) continue;

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

                valuesInCol.insert(val);

            }

        }

        return true;

    }

    // Returns whether all the partial squares are currently valid, 
    // i.e. whether no 3x3 square contains a duplicate value.
    bool checkValidSquares() {

        for (short squareX = 0; squareX < 3; squareX++) {
        for (short squareY = 0; squareY < 3; squareY++) {

            std::set<short> valuesInSquare;
            for (short m = 0; m < 3; m++) {
                for (short n = 0; n < 3; n++) {

                    short val = this->values[squareX*3+m][squareY*3+n];
                    if (val == 0) continue;
                    if (valuesInSquare.find(val) != valuesInSquare.end()) {
                        return false;
                    }
                    valuesInSquare.insert(val);

                }
            }

        }
        }

        return true;

    }

    // Returns whether the current position is valid, i.e., whether its rows, columns, 
    // and squares are all valid and do not contain a duplicate value.
    bool checkValid() {

        return this->checkValidRows() && this->checkValidCols() && this->checkValidSquares();

    }

public:

    Sudoku() {

        // Start with a blank position.
        for (short row = 0; row < 9; row++) {
        for (short col = 0; col < 9; col++) {

            this->values[row][col] = 0;

        }
        }

    }

    Sudoku (const short values[9][9]) {

        for (short row = 0; row < 9; row++) {
        for (short col = 0; col < 9; col++) {

            // Values that are not between [0 .. 9] 
            // are discarded and replaced with zeroes.
            short val = values[row][col];
            if (val >= 0 && val <= 9) {
                this->values[row][col] = val;
            } 
            else {
                this->values[row][col] = 0;
            }

        }
        }

    }

    // This is the solver method.
    // The way to solution is a standard backtrack exercise.
    // 
    // For the next empty space, we put the lowest possible value in, and recursively call the solver
    // to see whether it can complete all the other spaces in a valid manner – in which case, we are done.
    // In case that no possible value in the chosen space leads to a valid and complete solution, we clear 
    // the space entirely and declare the position as unsolvable. In recursion terms, this means that 
    // we need to attempt to put another value to the previous space, and so on.

    SolutionResult solve() {

        // Check that the situation is valid.
        if (!this->checkValid()) return SR_INVALID;

        // For the next empty space:
        for (short row = 0; row < 9; row++) {
        for (short col = 0; col < 9; col++) {

            if (this->values[row][col] > 0) continue;

            // Put an arbitrary value into the space:
            for (short val = 1; val <= 9; val++) {

                this->values[row][col] = val;

                // Recursively call the solver and see whether it can complete all 
                // the other spaces in the same manner. In case that it can, we're done.
                if (this->solve() == SR_SOLVED) {
                    return SR_SOLVED;
                }

            }

            // We tried all the possible values, and none has lead to a complete and 
            // valid solution. Clear the space, and return that the position is unsolvable.
            this->values[row][col] = 0;
            return SR_UNSOLVABLE;

        }
        }

        // There are no more empty spaces. 
        // We're done.
        return SR_SOLVED;

    }

    // Formatted output.
    friend std::ostream& operator<< (std::ostream &os, const Sudoku& s) {

        for (short row = 0; row < 9; row++) {

            os << std::endl;
            for (short col = 0; col < 9; col++) {

                os << ' ';
                if (s.values[row][col] > 0) {
                    os << s.values[row][col];
                } else {
                    os << '.';
                }

                if (col == 2 || col == 5) {
                    os << " |";
                }

            }

            if (row == 2 || row == 5) {
                os << std::endl << " ------+-------+------";
            }

        }

        os << std::endl;
        return os;

    }

    //
    static void runUnitTests();

};

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

int main (int argc, char* argv[])
{
    short pos[9][9] = {
        {0, 0, 4, 0, 0, 0, 0, 0, 0},
        {6, 0, 2, 1, 0, 0, 0, 0, 0},
        {1, 0, 8, 3, 4, 2, 5, 6, 7},
        {0, 0, 9, 7, 0, 0, 4, 2, 3},
        {4, 0, 6, 8, 0, 0, 0, 0, 1},
        {7, 1, 3, 9, 2, 0, 8, 0, 0},
        {0, 0, 0, 5, 3, 0, 2, 8, 4},
        {0, 0, 0, 4, 1, 0, 6, 0, 0},
        {3, 0, 0, 0, 0, 6, 1, 0, 0}
    };

    // Let's test the entire unit.
    Sudoku::runUnitTests();

    // Let's solve the particular position above:

    Sudoku s (pos);
    std::cout << s << std::endl;
    std::cout << "Solver starts, please wait..." << std::endl;

    switch (s.solve()) {

    case SR_SOLVED:
        std::cout << "Solution: " << std:: endl;
        std::cout << s << std::endl;
        break;

    case SR_INVALID:
        std::cout << "The position is not valid." << std:: endl;
        break;

    case SR_UNSOLVABLE:
        std::cout << "The position cannot be solved." << std:: endl;
        break;

    }

    std::cout << "Press any key to exit...";
    getchar();
    return 0;
}

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

void Sudoku::runUnitTests() {

    short dataZeroes[9][9] = {
        {0, 0, 0, 0, 0, 0, 0, 0, 0},
        {0, 0, 0, 0, 0, 0, 0, 0, 0},
        {0, 0, 0, 0, 0, 0, 0, 0, 0},
        {0, 0, 0, 0, 0, 0, 0, 0, 0},
        {0, 0, 0, 0, 0, 0, 0, 0, 0},
        {0, 0, 0, 0, 0, 0, 0, 0, 0},
        {0, 0, 0, 0, 0, 0, 0, 0, 0},
        {0, 0, 0, 0, 0, 0, 0, 0, 0},
        {0, 0, 0, 0, 0, 0, 0, 0, 0}
    };

    short dataValidPos1[9][9] = {
        {5, 3, 4, 6, 7, 8, 9, 1, 2},
        {6, 7, 2, 1, 9, 5, 3, 4, 8},
        {1, 9, 8, 3, 4, 2, 5, 6, 7},
        {8, 5, 9, 7, 6, 1, 4, 2, 3},
        {4, 2, 6, 8, 5, 3, 7, 9, 1},
        {7, 1, 3, 9, 2, 4, 8, 5, 6},
        {9, 6, 1, 5, 3, 7, 2, 8, 4},
        {2, 8, 7, 4, 1, 9, 6, 3, 5},
        {3, 4, 5, 2, 8, 6, 1, 7, 9}
    };

    short dataValidPos2[9][9] = {
        {0, 0, 0, 0, 0, 0, 0, 0, 0},
        {6, 0, 2, 1, 0, 0, 0, 0, 0},
        {1, 0, 8, 3, 4, 2, 5, 6, 7},
        {0, 0, 9, 7, 0, 0, 4, 2, 3},
        {4, 0, 6, 8, 0, 0, 0, 0, 1},
        {7, 1, 3, 9, 2, 0, 8, 0, 0},
        {0, 0, 0, 5, 3, 0, 2, 8, 4},
        {0, 0, 0, 4, 1, 0, 6, 0, 0},
        {3, 0, 0, 0, 0, 6, 1, 0, 0}
    };

    short dataInvalidRows[9][9] = {
        {5, 3, 4, 6, 7, 8, 9, 1, 5},
        {6, 7, 2, 1, 9, 5, 3, 4, 8},
        {1, 9, 8, 3, 4, 2, 0, 6, 7},
        {8, 5, 9, 7, 6, 1, 4, 2, 3},
        {4, 2, 6, 8, 5, 3, 7, 9, 1},
        {7, 1, 3, 9, 2, 4, 8, 5, 6},
        {9, 6, 1, 5, 3, 7, 2, 8, 4},
        {2, 8, 7, 4, 1, 9, 6, 3, 0},
        {3, 4, 5, 2, 8, 6, 1, 7, 9}
    };

    short dataInvalidCols[9][9] = {
        {5, 3, 4, 6, 7, 8, 9, 1, 2},
        {6, 7, 2, 1, 9, 0, 3, 4, 8},
        {1, 9, 8, 3, 4, 2, 5, 6, 7},
        {8, 5, 9, 7, 6, 1, 4, 2, 3},
        {4, 2, 6, 8, 5, 3, 7, 9, 1},
        {7, 1, 3, 9, 2, 4, 8, 5, 6},
        {9, 6, 1, 5, 3, 7, 2, 8, 4},
        {2, 8, 7, 4, 1, 9, 6, 3, 5},
        {5, 4, 0, 2, 8, 6, 1, 7, 9}
    };

    short dataInvalidSquares[9][9] = {
        {5, 3, 4, 6, 7, 8, 9, 1, 2},
        {6, 5, 2, 1, 9, 0, 3, 4, 8},
        {1, 9, 8, 3, 4, 2, 5, 6, 7},
        {8, 0, 9, 7, 6, 1, 4, 2, 3},
        {4, 2, 6, 8, 5, 3, 7, 9, 1},
        {7, 1, 3, 9, 2, 4, 8, 5, 6},
        {9, 6, 1, 5, 3, 7, 2, 8, 4},
        {2, 8, 7, 4, 1, 9, 6, 3, 5},
        {3, 4, 5, 2, 8, 6, 1, 7, 9}
    };

    Sudoku zeroes (dataZeroes);
    Sudoku validPos1 (dataValidPos1);
    Sudoku validPos2 (dataValidPos2);
    Sudoku invalidRows (dataInvalidRows);
    Sudoku invalidCols (dataInvalidCols);
    Sudoku invalidSquares (dataInvalidSquares);

    std::cout << "Unit tests start..." << std::endl;

    assert(zeroes.checkValidRows());
    assert(zeroes.checkValidCols());
    assert(zeroes.checkValidSquares());
    assert(zeroes.checkValid());
    assert(zeroes.solve() == SR_SOLVED);

    assert(validPos1.checkValidRows());
    assert(validPos1.checkValidCols());
    assert(validPos1.checkValidSquares());
    assert(validPos1.checkValid());
    assert(validPos1.solve() == SR_SOLVED);

    assert(validPos2.checkValidRows());
    assert(validPos2.checkValidCols());
    assert(validPos2.checkValidSquares());
    assert(validPos2.checkValid());
    assert(validPos2.solve() == SR_SOLVED);
    assert(validPos2.values[0][0] == 5);
    assert(validPos2.values[0][1] == 3);
    assert(validPos2.values[0][2] == 4);
    assert(validPos2.values[0][3] == 6);
    assert(validPos2.values[0][4] == 7);
    assert(validPos2.values[0][5] == 8);
    assert(validPos2.values[0][6] == 9);
    assert(validPos2.values[0][7] == 1);
    assert(validPos2.values[0][8] == 2);

    assert(!invalidRows.checkValidRows());
    assert( invalidRows.checkValidCols());
    assert( invalidRows.checkValidSquares());
    assert(!invalidRows.checkValid());
    assert( invalidRows.solve() == SR_INVALID);

    assert( invalidCols.checkValidRows());
    assert(!invalidCols.checkValidCols());
    assert( invalidCols.checkValidSquares());
    assert(!invalidCols.checkValid());
    assert( invalidCols.solve() == SR_INVALID);

    assert( invalidSquares.checkValidRows());
    assert( invalidSquares.checkValidCols());
    assert(!invalidSquares.checkValidSquares());
    assert(!invalidSquares.checkValid());
    assert( invalidSquares.solve() == SR_INVALID);

    std::cout << "All unit tests OK." << std::endl;

}
\$\endgroup\$
  • 2
    \$\begingroup\$ Your commenting and huge amount of unneeded whitespace can be improved. I have suggested Jamal. I think he'll be reviewing that part soon. \$\endgroup\$ – Aseem Bansal Jul 20 '13 at 18:41
  • 4
    \$\begingroup\$ You might be interested to read my series on writing a Sudoku solver in C#; I carefully describe the pros and cons of the various small and large choices that I make during the development of the algorithm. blogs.msdn.com/b/ericlippert/archive/tags/graph+colouring \$\endgroup\$ – Eric Lippert Jul 20 '13 at 21:22
13
\$\begingroup\$

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.)

\$\endgroup\$
  • \$\begingroup\$ Thanks, that is an excellent list of what I can indeed improve. I'll probably start over entirely. \$\endgroup\$ – mzi Jul 20 '13 at 21:15
  • 1
    \$\begingroup\$ It is absolutely OK (and encouraged!) to make a new code review when you have changed your code. Even more so if you start all over. \$\endgroup\$ – Lstor Jul 20 '13 at 21:20
  • \$\begingroup\$ Make your solver handle boards with multiple solutions. Technically a sudoku board only has one solution. If it has more than one solution and you are guessing at any square then it is not really a sudoku board. \$\endgroup\$ – Martin York Jul 21 '13 at 21:15
  • \$\begingroup\$ What is unprofessional about the "press any key to exit" part? (I agree it is not really useful, but I do not understand why it is unprofessional.) \$\endgroup\$ – Attilio Oct 11 '16 at 19:31
  • \$\begingroup\$ I'll respectfully disagree on "Put a default in your switch to catch errors." If you have appropriate compiler warnings, a default can hide errors that the compiler can pick up when switching on an enumeration such as this. A default leaves that detection until runtime. (This works best of all when all cases return, because you can get the best of both worlds and put exceptional cases after the switch, that are reached only if you've casted an out-of-range integer into the enumerator value). \$\endgroup\$ – Toby Speight Oct 13 '16 at 9:20
9
\$\begingroup\$
  • Read this and this regarding your commenting. You also shouldn't have that extra whitespace within your functions.

  • I cannot tell if your full solution uses a single file or separate files. Multiple files are preferred, but a single file is also okay if declarations and definitions are properly structured.

    Regarding declarations, you shouldn't have any function definitions in your header (considering you're not using accessors and mutators). Try this:

    class Sudoku {
    
    private: // private is preferred over protected here
        short values[9][9];
    
        // these functions should be const
        bool checkValidRows() const;
        bool checkValidCols() const;
        bool checkValidSquares() const;
    
    public:
        Sudoku();
        Sudoku(const short values[9][9]);
        SolutionResult solve();
        friend std::ostream& operator<<(std::ostream&, Sudoku const&);
    };
    
  • You could use a "2D" std::array instead. It gives you these features, but it looks a bit long as a nested structure (although you can use a typedef):

    std::array<std::array<short, 9>, 9> values;
    
  • solve() shouldn't be public since it's not part of the interface. Instead, make it private and void, and have another public function return the appropriate SolutionResult. You would then need to call solve() somewhere in the class as opposed to main().

  • For a serious application, you shouldn't need to hard-code your game board. Even after hiding this behind the class, it would be boring as you would be solving the same board each time (unless you're willing to keep changing it yourself).

    Instead, consider having the board randomized with values and making sure it's a valid starting board. Again, this would be done inside the class.

    You then won't need your second constructor.

\$\endgroup\$
  • 1
    \$\begingroup\$ I don't know C++ so I'll just ask you whether this and this should be suggested to the OP for his comments? If you think that they are relevant please do so. It might be helpful to the OP. \$\endgroup\$ – Aseem Bansal Jul 20 '13 at 18:36
  • \$\begingroup\$ @AseemBansal: Good point. I also just noticed all the whitespace, too. I'll add this. \$\endgroup\$ – Jamal Jul 20 '13 at 18:39
  • \$\begingroup\$ @Lstor: Thanks. I have much to learn from your answer, too. The one thing that I'm still not savvy with is error-handling. \$\endgroup\$ – Jamal Jul 20 '13 at 20:55
0
\$\begingroup\$

Other answers address the body of your code. I'm going to pick on improve the tests.

Firstly, it's great that you have written some tests. I see far too much code written by those who believe they can "just get it right", but actually can't (yes, I confess I sometimes find some of my own code without automated tests). I'm a strong advocate of test-driven development, which forces you to think of how to test before even writing any code.

Your tests will be much easier to read if each board was evaluated in its own test method:

// For test use, expose the protected methods for verification
class TestableSudoku : public Sudoku
{
public:
    using Sudoku::Sudoku;
    using Sudoku::checkValidRows;
    using Sudoku::checkValidCols;
    using Sudoku::checkValidSquares;
    using Sudoku::checkValid;
};

static void testZeros()
{

    short boardData[9][9] = {
        {0, 0, 0, 0, 0, 0, 0, 0, 0},
        {0, 0, 0, 0, 0, 0, 0, 0, 0},
        {0, 0, 0, 0, 0, 0, 0, 0, 0},
        {0, 0, 0, 0, 0, 0, 0, 0, 0},
        {0, 0, 0, 0, 0, 0, 0, 0, 0},
        {0, 0, 0, 0, 0, 0, 0, 0, 0},
        {0, 0, 0, 0, 0, 0, 0, 0, 0},
        {0, 0, 0, 0, 0, 0, 0, 0, 0},
        {0, 0, 0, 0, 0, 0, 0, 0, 0}
    };

    TestableSudoku board (boardData);

    assert(board.checkValidRows());
    assert(board.checkValidCols());
    assert(board.checkValidSquares());
    assert(board.checkValid());
    assert(board.solve() == SR_SOLVED);
}

Here, I've removed the test method from the Sudoku class. It's possibly a point of opinion, but to me, the tests are something you do to an object rather than part of the function of an object. The idea being that if Sudoku were a library, then the test program could link to that library just like any other client code.

In order to expose the protected members, I've created a class just for the test that exposes these. We could possibly go further, by specifying in one go which conditions we expect to be true:

const int ROWS_VALID = 1;
const int COLS_VALID = 2;
const int SQUARES_VALID = 4;
const int BOARD_VALID = 8;

// For test use, expose the protected methods for verification
class TestableSudoku : public Sudoku
{
public:
    using Sudoku::Sudoku;

    inline static bool expect(int flags, int mask)
    {
        return flags & mask;
    }

    void checkValidity(SolutionResult result, int flags)
    {
        assert(expect(flags, ROWS_VALID) == checkValidRows());
        assert(expect(flags, COLS_VALID) == checkValidCols());
        assert(expect(flags, SQUARES_VALID) == checkValidSquares());
        assert(expect(flags, BOARD_VALID) == checkValid());
        assert(solve() == result);
    }
};

static void testZeros()
{

    short boardData[9][9] = {
        {0, 0, 0, 0, 0, 0, 0, 0, 0},
        {0, 0, 0, 0, 0, 0, 0, 0, 0},
        {0, 0, 0, 0, 0, 0, 0, 0, 0},
        {0, 0, 0, 0, 0, 0, 0, 0, 0},
        {0, 0, 0, 0, 0, 0, 0, 0, 0},
        {0, 0, 0, 0, 0, 0, 0, 0, 0},
        {0, 0, 0, 0, 0, 0, 0, 0, 0},
        {0, 0, 0, 0, 0, 0, 0, 0, 0},
        {0, 0, 0, 0, 0, 0, 0, 0, 0}
    };

    TestableSudoku board (boardData);
    board.checkValidity(SR_SOLVED, ROWS_VALID | COLS_VALID | SQUARES_VALID | BOARD_VALID);
}

The motivation for this is that it's taking us towards data-driven tests. Instead of each test having custom logic for its setup and assertions, the logic is the same for all tests, and only the inputs and outputs change:

// Common control for all tests
const int ROWS_VALID = 1;
const int COLS_VALID = 2;
const int SQUARES_VALID = 4;
const int BOARD_VALID = 8;

class TestableSudoku : public Sudoku
{
public:
    using Sudoku::Sudoku;

    inline static bool expect(int flags, int mask)
    {
        return flags & mask;
    }

    void checkValidity(SolutionResult result, int flags)
    {
        assert(expect(flags, ROWS_VALID) == checkValidRows());
        assert(expect(flags, COLS_VALID) == checkValidCols());
        assert(expect(flags, SQUARES_VALID) == checkValidSquares());
        assert(expect(flags, BOARD_VALID) == checkValid());
        assert(solve() == result);
    }

    void checkRow(int row, const short (&expected)[9])
    {
        assert(!memcmp(values[row], expected, 9 * sizeof *expected));
    }
};

static TestableSudoku testBoard(short boardData[9][9],
                                SolutionResult result,
                                int flags)
{
    TestableSudoku board (boardData);
    board.checkValidity(result, flags);
    return board;
}
// Our tests now just supply data to the logic above
static void testZeros()
{
    short board[9][9] = {
        {0, 0, 0, 0, 0, 0, 0, 0, 0},
        {0, 0, 0, 0, 0, 0, 0, 0, 0},
        {0, 0, 0, 0, 0, 0, 0, 0, 0},
        {0, 0, 0, 0, 0, 0, 0, 0, 0},
        {0, 0, 0, 0, 0, 0, 0, 0, 0},
        {0, 0, 0, 0, 0, 0, 0, 0, 0},
        {0, 0, 0, 0, 0, 0, 0, 0, 0},
        {0, 0, 0, 0, 0, 0, 0, 0, 0},
        {0, 0, 0, 0, 0, 0, 0, 0, 0}
    };
    testBoard(board, SR_SOLVED, ROWS_VALID | COLS_VALID | SQUARES_VALID | BOARD_VALID);
}

static void testValidPos1()
{
    short board[9][9] = {
        {5, 3, 4, 6, 7, 8, 9, 1, 2},
        {6, 7, 2, 1, 9, 5, 3, 4, 8},
        {1, 9, 8, 3, 4, 2, 5, 6, 7},
        {8, 5, 9, 7, 6, 1, 4, 2, 3},
        {4, 2, 6, 8, 5, 3, 7, 9, 1},
        {7, 1, 3, 9, 2, 4, 8, 5, 6},
        {9, 6, 1, 5, 3, 7, 2, 8, 4},
        {2, 8, 7, 4, 1, 9, 6, 3, 5},
        {3, 4, 5, 2, 8, 6, 1, 7, 9}
        };
    testBoard(board, SR_SOLVED, ROWS_VALID | COLS_VALID | SQUARES_VALID | BOARD_VALID);
}

static void testValidPos2()
{
    short board[9][9] = {
        {0, 0, 0, 0, 0, 0, 0, 0, 0},
        {6, 0, 2, 1, 0, 0, 0, 0, 0},
        {1, 0, 8, 3, 4, 2, 5, 6, 7},
        {0, 0, 9, 7, 0, 0, 4, 2, 3},
        {4, 0, 6, 8, 0, 0, 0, 0, 1},
        {7, 1, 3, 9, 2, 0, 8, 0, 0},
        {0, 0, 0, 5, 3, 0, 2, 8, 4},
        {0, 0, 0, 4, 1, 0, 6, 0, 0},
        {3, 0, 0, 0, 0, 6, 1, 0, 0}
        };
    auto solver = testBoard(board, SR_SOLVED, ROWS_VALID | COLS_VALID | SQUARES_VALID | BOARD_VALID);
    solver.checkRow(0, { 5, 3, 4, 6, 7, 8, 9, 1, 2});
}

static void testInvalidRows()
{
    short board[9][9] = {
        {5, 3, 4, 6, 7, 8, 9, 1, 5},
        {6, 7, 2, 1, 9, 5, 3, 4, 8},
        {1, 9, 8, 3, 4, 2, 0, 6, 7},
        {8, 5, 9, 7, 6, 1, 4, 2, 3},
        {4, 2, 6, 8, 5, 3, 7, 9, 1},
        {7, 1, 3, 9, 2, 4, 8, 5, 6},
        {9, 6, 1, 5, 3, 7, 2, 8, 4},
        {2, 8, 7, 4, 1, 9, 6, 3, 0},
        {3, 4, 5, 2, 8, 6, 1, 7, 9}
    };
    testBoard(board, SR_INVALID, COLS_VALID | SQUARES_VALID);
}

You now have something that's easier to maintain and reason about, and that's more amenable to being executed by a unit-test framework. Moving to such a framework is useful on bigger projects, because (unlike assert()) it can continue after the first failure (to report on all the failing tests) and it may give more informative failure messages (by printing the actual and expected values, as well as the failing expression).

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.