Skip to main content
Code Review

Return to Revisions

1 of 3
Simon S.
Simon S.
  • 123
  • 5

C++ code, testing, project structure and building

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:

The Code

  • 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?

Project layout

This is a really small project but even with a single .cpp file I feel insecure about the project layout.

  • Is the layout reasonable?
  • Are files in the right place and named correctly?

Testing

I always want to unit test my code but I didn't find any official best practices. Right now I'm building an executable with the unit tests and using Catch2.

  • Could this be done better?
  • Should I add some kind of target so e.g. cmake --test would work? (How?)
  • When assertions fail, the variable names are printed. Becuase of that I create a label and use SECTION for each test. How could this be improved?

(c)make

Since I'm new to C/C++ I feel really insecure about cmake and CMakeLists.txt.

  • Should I use a regular Makefile?
  • How is my CMakeLists.txt
  • Should I use separate CMakeList.txt for the library and the executable (unittest)?
  • Should I generate or manage CMakeLists.txt or is manual work OK? (Is it even possible to generate?)

Building

So, obviously this is working and this is how I build the code.

Configure

cmake \
    --no-warn-unused-cli \
    -DCMAKE_EXPORT_COMPILE_COMMANDS:BOOL=TRUE \
    -DCMAKE_BUILD_TYPE:STRING=Debug \
    -DCMAKE_C_COMPILER:FILEPATH=/usr/bin/clang \
    -DCMAKE_CXX_COMPILER:FILEPATH=/usr/bin/clang++ \
    -H$(pwd) \
    -B$(pwd)/build

Build

cmake \
    --build $(pwd)/build \
    --config Debug \
    --target all \
    -- \
    -j 10
  • Is this a correct way to do it?

Files and headers

To be frank I'm still not really sure about the best practices around files with code and header files. As I understand it you cannot reference code that's not yet been seen by the compiler. One way to work around this is to define your functions in a header file and by including those definitions (like #include <personnummer.cpp>). I can then write my code in whatever order I want.

  • Is this a correct assumption about header files?
  • Is the way I defined methods and struct in the header file OK?

Distributing

I wanted to write this code to be able to distribute the library. I noticed by using Catch2 that you can distribute your code in a single header file. This is not the way I went, mostly because I don't understand how. I've understood that one reasone to distribute the library as source code is that the consumer can compile the code for a given architecture instead of me having to deal with those things in the potential header file.

  • Is distributing as source code a good way?
  • If someone wants to use this code, is their best bet to compile the library according to CMakeLists.txt and then link said library with their own application?
  • If someone were to release an application dependent on this code, how would they this be bundled?
    • Like a submodule?
    • Just linked in a README?
    • Bundled with compiled library?
Simon S.
  • 123
  • 5