Return to Answer

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.

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.

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&, const Sudoku&);
  };

You could use a "2D" std::array instead if you'd like. It gives you these features, but it looks a bit long as a nested structure. It would look like this:

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 change 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 also won't need your second constructor.

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.

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&, const Sudoku&);
  };

You could use a "2D" std::array instead if you'd like. It gives you these features, but it looks a bit long as a nested structure. It would look like this:

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 change 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 also won't need your second constructor.

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&, const Sudoku&);
  };

You could use a "2D" std::array instead if you'd like. It gives you these features, but it looks a bit long as a nested structure. It would look like this:

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 change 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 also won't need your second constructor.