Class ISBN - checks for valid ISBN 13

Question

I am at the moment trying to grasp multi-file programming instead of one big main.cpp.

The class I wrote is about ISBN 13.

Can you point out what could be done to improve my code?

Also is there a good guide how to write include guards? My sources differate in this respect.

Runnable code here on Wandbox

ISBN.hpp

#include <ostream>

class ISBN
{
/* examples 978-345-314-697-6
978-3-12-732320-7
*/
private:
    //the numbers between the minus signs
    unsigned int isbn_field1;
    unsigned int isbn_field2;
    unsigned int isbn_field3;
    unsigned int isbn_field4;
    unsigned int isbn_field5;
public:
    //constructors
    ISBN()
        : ISBN{978,3,12,732320,7} // a valid ISBN as default
    {}
    ISBN(const unsigned int& a, const unsigned int& b, const unsigned int& c, const unsigned int& d, const unsigned int& e);

    //output operator overloading
    friend std::ostream& operator<<(std::ostream& os, ISBN const& i);
};//ISBN

ISBN.cpp

#include "ISBN.hpp"
#include <iostream>
#include <string>

bool is_valid_isbn(const std::string& digits)
{
    unsigned int weighted_sum=0;
    std::string temp;
    for (unsigned int i=0; i<(digits.length());i++)
    {
        temp=digits[i];
        if ((i%2)==0)
        {
            weighted_sum+=std::stoi(temp);
        }
        else
        {
            weighted_sum+=std::stoi(temp)*3;
        }
    }
    if ((weighted_sum%10)!=0)
    {
        return false;
    }
    return true;
}

ISBN::ISBN(const unsigned int& a, const unsigned int& b, const unsigned int& c, const unsigned int& d, const unsigned int& e)
{
    const std::string digit_string=std::to_string(a)
    +std::to_string(b)
    +std::to_string(c)
    +std::to_string(d)
    +std::to_string(e);
    std::string temp;
    //check if last field is exactly one digit
    temp=std::to_string(e);
    if(temp.length()!=1)
    {
        throw "last unit is not a single digit\n";
    }
    //check if first field is 978 or 979
    if(a!=978 && a!=979)
    {
        throw "first unit is neither 978 nor 979\n";
    }
    //check number of digits=13
    if(digit_string.length()!=13)
    {
        throw "number of digits is not exactly 13\n";
    }
    if (!is_valid_isbn(digit_string))
    {
        throw "not a valid ISBN\n";
    }

    isbn_field1=a;
    isbn_field2=b;
    isbn_field3=c;
    isbn_field4=d;
    isbn_field5=e;
}

std::ostream& operator<<(std::ostream& os, ISBN const& i)
{
    return os << i.isbn_field1 << "-"
    << i.isbn_field2 << "-"
    << i.isbn_field3 << "-"
    << i.isbn_field4 << "-"
    << i.isbn_field5;
}

main.cpp

#include "ISBN.hpp"

#include <iostream>


int main()
{
    try
    {
        ISBN test;
        std::cout << test;
    }
    catch(char const* e)
    {
        std::cout << "Error:" << e;
    }
    catch(...)
    {
        std::cout << "test error";
    }
    return 0;
}

Answer 1

The comment in the ISBN class should rather be outside the class declaration, and it should contain a few words for the human reader, such as the expanded abbreviation and the Wikipedia page, to make it easy to get additional information about the topic of ISBNs.

Storing an ISBN as 5 numbers is wrong. If the length of these numbers doesn't sum up to 13, you cannot be sure where to add the zeroes. The Wikipedia article contains a table of example ISBN-10 numbers, and some of them have leading digits.

The variable names isbn_field1 are bad. The word isbn is not necessary since that information is given by the class ISBN already. Therefore field1 is better than isbn_field1. But what is field1, what does it mean? The Wikipedia article gives nice names to these fields, and so should your code.

The characters between the digits are not "minus signs" (as you say) but hyphens.

The no-argument constructor is wrong. There should be no way to construct an ISBN object without specifying all its parts. Having a single "best default" is simply not possible for an ISBN. You definitely don't want to convert all your programming mistakes to a book about analytic geometry.

In the constructor with the const unsigned int& parameters, the const& is not necessary. Integer values are typically passed directly instead of referencing them since they are one machine word long, and the computer can process them quickly. References make your code run slower in this case. For larger types such as std::vector<std::vector<ISBN>> it makes sense to use references instead of simple values.

In the is_valid_isbn function, you should add spaces around the tokens in i=0. Most C++ programmers are used to read this as i = 0.

In the same line, the parentheses around digits.length() are not necessary.

Instead of calling std::stoi, you can also write digits[i] - '0', which gives you the numeric value of a digit character. Which reminds me that some of the characters might not be digits at all. Your code should skip them and later verify that the string contained 13 digits in total.

In the ISBN::ISBN constructor, instead of checking whether the last field has length 1, you can just check whether e < 10. This is simpler and faster.

When validating multiple values, you should validate them in reading order. Start with the first field, and validate the last field last.

You didn't test your code by calling ISBN{0, 0, 0, 0, 0}.

When you throw something from your code, it should be a proper exception from the C++ standard library, such as std::invalid_argument.

Instead of the verbose std::string temp; temp = std::to_string(e); temp.length() != 1, you can simply write std::to_string(e).length() != 1. This way you don't have to think about a better variable name instead of temp. The name temp is always a bad variable name since it doesn't tell enough of a story. (Well, except if you use it as an abbreviation for temperature, but that's not the case here.)

The exception strings should not include a newline character. That character should be added when the exception is printed in a log file. If you want to save the exception somewhere else, the newline is probably wrong.

The main program is pretty useless. There is no way to enter an ISBN and validate it. You should rather define the following functions to get some automated tests:

void assert_valid_isbn(const std::string &isbn)
{
    /* TODO */
}

void assert_invalid_isbn(const std::string &isbn, const std::string &expected_error)
{
    /* TODO */
}