Validating Swedish social security numbers (Luhn algorithm and dates)

Question

I've been writing code for some time but never had the need (or opportunity) to write C/C++ code. Did a small project to learn some C++ and would appreciate feedback on code, testing, project structure and building.

Project Layout

.
├── CMakeLists.txt
├── src
│   ├── personnummer.cpp
│   └── personnummer.hpp
└── test
    ├── catch.hpp
    └── unittest.cpp

CMakeLists.txt

cmake_minimum_required(VERSION 3.0)
project(cpp-personnummer)
set(CMAKE_CXX_STANDARD 11)

set(SOURCE_FILES src/personnummer.cpp)

add_library(Personnummer ${SOURCE_FILES})
target_link_libraries(Personnummer -lpthread)

add_executable(unittest test/unittest.cpp)
target_link_libraries(unittest Personnummer)

src/personnummer.hpp

#include <ctime>
#include <iostream>
#include <vector>

namespace Personnummer {
int stoi_or_fallback(const std::string &maybe_digit, int fallback);

bool valid_date(int year, int month, int day);

int checksum(std::tm date, int number);

void collect_digits(std::vector<int> &digits, int num);

void collect_digits_pad_zero(std::vector<int> &digits, int num, int min_len);

struct Personnummer {
  std::tm date;
  int number;
  int control;
  char divider[1];

  bool is_valid_date() {
    return valid_date(date.tm_year, date.tm_mon, date.tm_mday);
  };

  bool is_valid_luhn() { return checksum(date, number) == control; };

  bool valid() { return is_valid_date() && is_valid_luhn(); };
};

bool from_string(const std::string &pnr, Personnummer &personnummer);
} // namespace Personnummer

src/personnummer.cpp

#include "personnummer.hpp"
#include <regex>

namespace Personnummer {
bool from_string(const std::string &pnr, Personnummer &personnummer) {
  std::regex pnr_regex(
      "^(\\d{2})?(\\d{2})(\\d{2})(\\d{2})([-|+]?)?(\\d{3})(\\d?)$");
  std::smatch matches;

  int century, year;

  if (std::regex_search(pnr, matches, pnr_regex)) {
    century = stoi_or_fallback(matches.str(1), 19);
    year = stoi_or_fallback(matches.str(2), 0);

    personnummer.date.tm_year = century * 100 + year;
    personnummer.date.tm_mon = stoi_or_fallback(matches.str(3), 0);
    personnummer.date.tm_mday = stoi_or_fallback(matches.str(4), 0);
    personnummer.number = stoi_or_fallback(matches.str(6), 0);
    personnummer.control = stoi_or_fallback(matches.str(7), 0);

    personnummer.divider[0] = *matches.str(5).c_str();
  } else {
    return false;
  }

  return true;
}

int stoi_or_fallback(const std::string &maybe_digit, int fallback) {
  try {
    return std::stoi(maybe_digit);
  } catch (...) {
    return fallback;
  }
}

bool valid_date(int year, int month, int day) {
  if (month < 1 || month > 12)
    return false;

  if (day < 1 || day > 31)
    return false;

  if (day > 30) {
    switch (month) {
    case 2:
    case 4:
    case 6:
    case 9:
    case 11:
      return false;
    }
  }

  if (month == 2 && day > 28) {
    bool is_leap_year = year % 400 == 0 || (year % 100 != 0 && year % 4 == 0);

    if (day != 29 || !is_leap_year) {
      return false;
    }
  }

  return true;
}

int checksum(std::tm date, int number) {
  std::vector<int> digits;

  collect_digits_pad_zero(digits, date.tm_year % 100, 2);
  collect_digits_pad_zero(digits, date.tm_mon, 2);
  collect_digits_pad_zero(digits, date.tm_mday, 2);
  collect_digits_pad_zero(digits, number, 3);

  int sum = 0;

  for (int i = 0; i < digits.size(); i++) {
    int temp = digits.at(i);

    if (i % 2 == 0) {
      temp *= 2;

      if (temp > 9)
        temp -= 9;
    }

    sum += temp;
  }

  return 10 - (sum % 10);
}

void collect_digits(std::vector<int> &digits, int num) {
  if (num > 9)
    collect_digits(digits, num / 10);

  digits.push_back(num % 10);
}

void collect_digits_pad_zero(std::vector<int> &digits, int num, int min_len) {
  // New vector for this section.
  std::vector<int> section_digits;

  // Collect the digits from given number.
  collect_digits(section_digits, num);

  // Add the potential padded zeroes.
  int missing_digits = min_len - section_digits.size();
  for (int i = 0; i < missing_digits; i++) {
    section_digits.insert(section_digits.begin(), 0);
  }

  // Add the padded section to final vector.
  digits.insert(digits.end(), section_digits.begin(), section_digits.end());
}
} // namespace Personnummer

test/unittest.cpp

#define CATCH_CONFIG_MAIN
#include "../src/personnummer.hpp"
#include "catch.hpp"

TEST_CASE("Valid date", "[date]") {
  std::vector<std::vector<int>> valid_dates = {
      {1990, 1, 1},  {1990, 1, 31},
      {1990, 2, 28}, {2016, 2, 29}, // 2016 is leap year
      {2020, 4, 30},
  };

  std::vector<std::vector<int>> invalid_dates = {
      {1990, 13, 1},
      {1990, 1, 32},
      {2017, 2, 29}, // 2017 is not leap year
      {2020, 4, 31},
  };

  for (int i = 0; i < valid_dates.size(); i++) {
    std::vector<int> test_case = valid_dates[i];

    std::stringstream case_title;
    case_title << "Testinv VALID: Y=" << test_case[0] << ", M=" << test_case[1]
               << ", D=" << test_case[2];

    SECTION(case_title.str()) {
      REQUIRE(
          Personnummer::valid_date(test_case[0], test_case[1], test_case[2]));
    }
  }

  for (int i = 0; i < invalid_dates.size(); i++) {
    std::vector<int> test_case = invalid_dates[i];

    std::stringstream case_title;
    case_title << "Testinv INVALID: Y=" << test_case[0]
               << ", M=" << test_case[1] << ", D=" << test_case[2];

    SECTION(case_title.str()) {
      REQUIRE(
          !Personnummer::valid_date(test_case[0], test_case[1], test_case[2]));
    }
  }
}

TEST_CASE("Valid personal number", "[pnr]") {
  Personnummer::Personnummer p;

  std::vector<std::string> valid = {
      "6403273813",   "510818-9167", "19900101-0017", "19130401+2931",
      "196408233234", "0001010107",  "000101-0107",
  };

  std::vector<std::string> invalid = {
      "640327-381",
      "6403273814",
      "640327-3814",
  };

  for (int i = 0; i < valid.size(); i++) {
    std::stringstream case_title;
    case_title << "Testing VALID: " << valid[i];

    SECTION(case_title.str()) {
      REQUIRE(Personnummer::from_string(valid[i], p));
      REQUIRE(p.valid());
    }
  }

  for (int i = 0; i < invalid.size(); i++) {
    std::stringstream case_title;
    case_title << "Testing INVALID: " << invalid[i];

    SECTION(case_title.str()) {
      REQUIRE(Personnummer::from_string(invalid[i], p));
      REQUIRE(!p.valid());
    }
  }
}

test/catch.hpp

Downloaded from GitHub Repository

---

General feedback and reviews are very welcome but these are the main questions I had writing this:

• Does the code itself look OK? (It's formatted with clang-format --style=llvm)
• Are there any issues (big or small) that I've missed (bugs, memory leaks etc).
• How far off C++ pragma is it?
  • What would be a more C++ way of things?
• Is the way I split the code in files vs. headers a good/preferred way?
• Are the unit tests written in a good way (structure, location, naming, table driven)?

Answer 1

Here are some suggestions for how you might improve your code.

Use the required #includes

The code uses std::string which means that it should #include <string>. It was not difficult to infer, but it helps reviewers if the code is complete.

Know your tools

The CMake file starts with these lines:

cmake_minimum_required(VERSION 3.0)
project(cpp-personnummer)
set(CMAKE_CXX_STANDARD 11)

However, that's a problem because the CMAKE_CXX_STANDARD wasn't intruduced until CMake version 3.1. I'd recommend changing the cmake_minimum_required to 3.1.

Don't link with unnecesary libraries

The CMake file currently contains this line:

target_link_libraries(Personnummer -lpthread)

First, the correct way to do this in CMake would be like this instead:

find_package(Threads)
target_link_libraries(Personnummer ${CMAKE_THREAD_LIBS_INIT})

Second, and most importantly, it doesn't need to be done at all since nothing in there requires threads.

Use standard classes where appropriate

Much of the date handling from the Personnummer class could be done more simply by using the std::chrono::year_month_day class. For example, valid_date could be eliminated entirely in favor of year_month_day.ok() if your compiler supports C++20.

Use CMake's test facility

CMake has test facilities built in. I'd add the option to either build tests or not and in the top level CMakeLists.txt add this:

option(WITH_TEST "Build the test suite" OFF)
if (WITH_TEST)
    enable_testing()
    add_subdirectory(test)
endif()

Now if you invoke CMake with cmake -DWITH_TEST=1 .. testing will be enabled. If you then do make and then make test, you will invoke your test(s). Note here that I've written add_subdirectory. See the next suggestion for more on that.

Use multiple CMakeLists.txt files

Instead of a single CMakeLists.txt file, it's easier to maintain if you create one per directory. So the top level would look like this:

cmake_minimum_required(VERSION 3.1)
project(Personnummer)
set(CMAKE_CXX_STANDARD 11)
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -Wextra -pedantic")
option(WITH_TEST "Build the test suite" OFF)
add_subdirectory(src)

if (WITH_TEST)
    enable_testing()
    add_subdirectory(test)
endif()

Now the one in src looks like this:

cmake_minimum_required(VERSION 3.1)
add_library(Personnummer "personnummer.cpp")

And the one in test looks like this:

cmake_minimum_required(VERSION 3.1)
include_directories(${CMAKE_HOME_DIRECTORY}/src)
add_executable(unittest "unittest.cpp")
add_test(PersonnummerTest unittest)
target_link_libraries(unittest Personnummer)

Note that I've added the include_directories to point to the src directory. More on that in the next suggestion.

Don't hardcode project directory structures

In the current test program we have this line:

#include "../src/personnummer.hpp"

This buries a project configuration detail inside source code. I recommend keeping those kinds of details out of the individual source files and instead point the tools (compiler, linker, etc.) to the appropriate directory instead. That's why the include_directories was added to the CMakeLists.txt file for the test directory. This way, if you decided to rename the src directory or move it elsewhere, the change could easily be made in just the CMake files instead of having to hunt through every source code file. When there only a few, as with this small project, it might not seem like it makes much difference, but when you start working with large projects with thousands of files, following this advice will save you a great deal of time and frustration.

Prefer iteration to recursion

The current definition of collect_digits is this recursive solution:

void collect_digits(std::vector<int> &digits, int num) {
  if (num > 9)
    collect_digits(digits, num / 10);

  digits.push_back(num % 10);
}

It's not faulty, but it isn't as efficient as it could be. Generally, recursion is less efficient than iteration. The reason is that pushing the the return address and both arguments onto the stack takes a bit more time and memory than simply doing this:

void collect_digits(std::vector<int> &digits, int num) {
  while ( ; num > 9; num /= 10) {
    digits.push_back(num % 10);
  }
  digits.push_back(num);
}

Better though, see the next suggestion.

Rethink the interface

There are a number of specialized functions such as collect_digits_pad_zero that probably don't need to be part of the interface, or maybe shouldn't exist at all. For example, checksum could be much simplified and wouldn't require either collect_digits_pad_zero or collect_digits. Here's how I'd write it:

int luhn(std::string::iterator begin, std::string::iterator end) {
    int sum{0};
    for (bool even{true}; begin != end; ++begin, even ^= true) {
        int digit = *begin - '0';
        if (even) {
            if ((digit *= 2) > 9) {
                digit -= 9;
            }
        } 
        if ((sum += digit) > 9) {
            sum -= 10;
        }
    }
    return sum;
}

int Personnummer::checksum() const {
    using namespace std;
    stringstream ss;
    ss.fill('0');
    ss << setw(2)
       << date.tm_year % 100
       << setw(2)
       << date.tm_mon
       << setw(2)
       << date.tm_mday
       << setw(3) << number;
    auto str = ss.str();
    return 10 - luhn(str.begin(), str.end());
}

Now we have two compact and useful functions. Note also that checksum is a member function rather than a standalone function.

Use const where appropriate

The functions is_valid_luhn and valid and is_valid_date are all non-modifying functions. That is, none of them modify the underlying Personnummer object, so all of them should be declared const as with the checksum function above.

Prefer class to struct

If there is not a compelling that Personnummer must have all of its data and functions public, it should be a class instead of a struct, and from_string should be a member function.

Don't create pointless arrays

The Personnummer object definition currently includes this:

char divider[1];

That should just be this:

char divider;

Use "range for" to simplify your code

The test code currently has these kinds of loops:

for (int i = 0; i < valid.size(); i++) {
  std::stringstream case_title;
  case_title << "Testing VALID: " << valid[i];

  SECTION(case_title.str()) {
      REQUIRE(p.from_string(valid[i]));
      REQUIRE(p.valid());
  }
}

First, you can use a range for to simplify, and second, there's no need for a std::stringstream here:

for (const auto& test_string : valid) {
  SECTION({"Testing VALID" + test_string}) {
    REQUIRE(p.from_string(test_string));
    REQUIRE(p.valid());
  }
}

This also assumes that the from_string function has been made a member function, as mentioned earlier.

Use classes to simplify your code

The test cases for "Valid date" use std::vector<std::vector<int>> as the collection of test cases. I'd suggest using a class there instead:

struct FakeDate {
  int year, month, day;
  friend std::ostream& operator<<(std::ostream& out, const FakeDate& fd) {
      return out << "Y=" << fd.year << ", M=" << fd.month << ", D=" << fd.day;
  }
};

Now the collections can be std::vector<FakeDate>. Using the same range for as in the previous suggestion, we have:

for (const auto& test_case : valid_dates) {
  std::stringstream case_title;
  case_title << "Testing VALID: " << test_case;

  SECTION(case_title.str()) {
    REQUIRE(
        Personnummer::valid_date(test_case.year, test_case.month, test_case.day));
  }
}

Be careful with signed vs. unsigned

The current valid_date code takes ints for the month, day and year. Is it intended that negative values for the year do not make the date invalid? If not, I'd suggest either checking for negative values or using only unsigned integers for each.