Validating sudoku columns and rows

Question

I'm writing a sudoku col/row validator, what is does is:

1. Reads user input on how many Sudoku instances to validate.
2. Reads the sudoku matrix grid 9x9 (for me, in my specific case 9x9) into a 2d array. Each line read/row should contain something like: 8 2 1 3 6 4 9 7 5.
3. Validates each row and column by flipping a 9 bit bitset to test whether all columns and rows contains nums from 1-9.

Please note that the only purpose of this program is to validate sudoku columns/rows as specified and not to completely validate it. i.e also checking each sudoku block.

How can I improve my code? I believe there's lots to it, naming conventions, fundamental approach...etc..etc.

#include <iostream>
#include <sstream>
#include <bitset>
namespace Constants
{
 constexpr int ROW_COL_SIZE = 9;
 const std::string STR_YES = "YES";
 const std::string STR_NO = "NO";
}
template<unsigned int N>
class Sudoku
{
private:
 int m_matrix[N][N] {{0}};
public:
 void ReadRows();
 std::string strIsValid() const;
};
template<unsigned int N>void Sudoku<N>::ReadRows()
{
 for(unsigned int row{0}; row<N; row++)
 {
 static std::string line;
 std::getline(std::cin, line);
 std::istringstream issline(line);
 int readnum {0};
 for(unsigned int i{0}; i<N; i++)
 {
 issline >> readnum;
 m_matrix[row][i] = readnum;
 }
 }
}
template<unsigned int N>std::string Sudoku<N>::strIsValid() const
{
 //9bit default ctor all zeroes, 000000000
 std::bitset<N> bitRow[N];
 std::bitset<N> bitCol[N];
 for(int i{0}; i<N; i++)
 {
 for(int j{0}; j<N; j++)
 {
 bitRow[i].flip(m_matrix[i][j]-1); //verify nums 1-9 row, bitset index is 0 not 1.
 bitCol[i].flip(m_matrix[j][i]-1); //verify nums 1-9 col
 }
 if(!bitRow[i].all() && !bitCol[i].all())
 {
 return Constants::STR_NO;
 }
 }
 return Constants::STR_YES;
}
int main(int, char**)
{
 int instances {0};
 std::cin >> instances;
 std::cin.clear();
 std::cin.ignore();
 for(int i{0}; i<instances; i++)
 {
 std::cout << "Instance " << i+1 << std::endl;
 Sudoku<Constants::ROW_COL_SIZE> sudokuInstance;
 sudokuInstance.ReadRows();
 std::cout << sudokuInstance.strIsValid() << std::endl << std::endl;
 }
 return 0;
}

Answer 1

_{It is not a complete answer. Just some of my thoughts about this code.}

C++

• Do not compare integers of different signedness. Loop counters i and j in function strIsValid are declared as int, but they are compared with N which is unsigned int. It could lead to some problems. Make the counters unsigned.

• Use strict compilation flags. The compiler would tell you about your signed/unsigned issue if you pass it -Wall key.

  You should always achieve that the compiler will not display any diagnostic messages with the most strict compilation keys (-Wall, -Wextra, -pedantic, etc).

  _{But you probably would not want to use Clang's -Weverything.}

• What happened with your main? Your main function looks very unnatural:

  int main(int, char**)
  

  If you do not need argc and argv, write just

  int main()
  

• About std::endl. You should avoid globally using std::endl. It is not the same as just \n. There is a good answer on SO about this topic.

  And why are you printing std::endl twice?

  std::cout << sudokuInstance.strIsValid() << std::endl << std::endl;
  

• You do not have to explicitly return from main. You don't have to explicitly return 0; at the end of main.

Programming

• Minimize the scope of local variables. You should always minimize the scope of local variables. In your case, for example, scope of the readnum variable can be reduced. Some reading about this topic.

• The predicative function should return boolean. Your strIsValid is a predicative function. It means that this function checks something and returns either true or false. But you return string "YES" (true) or "NO" (false). You should return bool instead.

• What the strIsValid function does? The name of this function doesn't answer the question.

Architecture

• Validate user's input. At this time you do not validate user's input in ReadRows. What if I'll input a number which is greater than 9? Or less than 0? If I'll input too many numbers?..

  In this case, you store invalid data into m_matrix. Then this invalid data will be used as arguments of std::bitset's flip() which will cause std::out_of_range.

• Split row and cols validating and reading user's input. The Sudoku class should know nothing about reading user's input. The only thing that it should do is validate Sudoku columns and rows! This is called the Single Responsibility Principle (SRP).

• L. F.: Some of your thoughts about the code makes a valid code review, don't worry about that
• Ron Klein: I totally agree regarding "You should return bool instead".
• Vinícius: tyvm, applied all I've learned,

Answer 2

First, I just want to remark that this is excellent use of std::bitset<>! Further, I'm just adding to eanmos's comments.

Don't define constants that are named after their value

Apart from the fact that, as already mentioned, strIsValid() should return a bool, you define the constants Constants::STR_YES and Constants::STR_NO. These constants are longer to type than literal "YES" and "NO". Also, consider that you would ever change the value of STR_YES. The code was written with the assumption that the constant would be the literal "YES", as that is what its name clearly says, but now you are breaking that assumption. So in main(), just I would just write:

std::cout << sudokuInstance.strIsValid() ? "YES\n" : "NO\n";

If the goal is to make it easy to translate the program, then you should use a so-called internationalization library to do this, such as gettext. Writing constants like this doesn't scale for programs with a large amount of text.

Consider returning a meaningful value from main()

Your program prints whether each Sudoku instance is valid to standard output, but the exit code is always 0. It is custom to have the exit code be non-zero if an error happened. While technically your program still runs perfectly fine if you feed it an invalid Sudoku, you might consider returning 1 if it has encountered any non-valid Sudoku. This is similar to what some command line tools do, like cmp or grep -q. This way, you can call your program from a shell script, and have the shell script react to the result of your program in an easy way, making it easy to integrate it into larger projects.

• Vinícius: Makes a lot of sense, tyvm