3
\$\begingroup\$

This is just a little hobby project I've been working on. I've been using it to learn/improve my knowledge of C++.

The program generate Norwegian personal-numbers (11 digits) for males born a given year. It works just fine, but I'd like some input on how to improve the code. For now I have a basic class structure.

Norwegian personal-numbers is built up like this.

#include <iostream>

using namespace std;

const int DATE_LENGTH = 6;
const int BUFFER = 30;

class ID {
    private:
        int date[DATE_LENGTH];
        int i1, i2, i3;
        int k1, k2;
        int year;
    public:
        ID(int i1 = 0, int i2 = 0, int i3 = 0, int k1 = 0, int k2 = 0) {
            for (int i = 0; i < DATE_LENGTH; i++) { date[i] = 0; }
        }

        int *getDate() { return date; }

        void setYear(int year) {
            this->year = year;
        }

        int getYear() {
            return year;
        }

        void individSiffer() {
            static int individ = -1;
            if(year >= 1900 && year <= 1999 ){
                    individ++;
                    i1 = i2 = i3 = 0;
                }
                i3 = individ % 10;
                if(individ < 100){
                    i1 = 0;
                    i2 = individ / 10 % 10;
                }else if(individ < 10){
                    i1 = i2 = 0;
                }else{
                    i1 = individ / 100 % 10;
                    i2 = individ / 10 % 10;
                }
        }

        void generate() {
            k1 = 11 - ((3*date[0] + 7*date[1] + 6*date[2] + 1*date[3] + 8*date[4] + 9*date[5] + 4*i1 + 5*i2 + 2*i3) % 11); 
            if(k1 == 11){
                k1 = 0;
            }
            k2 = 11 - ((5*date[0] + 4*date[1] + 3*date[2] + 2*date[3] + 7*date[4] + 6*date[5] + 5*i1 + 4*i2 + 3*i3 + 2*k1) % 11);
            if(k2 == 11){
                k2 = 0;
            }
            if((i3 != 0 && i3 != 2 && i3 !=4 && i3 !=6 && i3 !=8) && (k1 != 10 && k2 !=10)){
                for (int i = 0; i < DATE_LENGTH; i++) {
                    printf("%d", date[i]);
                }
                printf("%d%d%d%d%d\n", i1, i2, i3, k1, k2);
            }   
        }
};

int main(int argc, char *argv[]) {
    char dateString[BUFFER];
    int i = 0;
    ID generator;

    printf("%s", "Your date of birth (DDMMYY): ");
    cin.getline(dateString, BUFFER);

    for(int i = 0; i < DATE_LENGTH; i++){
        *(generator.getDate() + i) = int(dateString[i]-'0');
    }
    generator.setYear(1900 + (*(generator.getDate() + 4) * 10) + *(generator.getDate() + 5));
    while(i < 500){
        generator.individSiffer();
        generator.generate();
        i++;
    }
    cout << "\n\n\n";
    return 0;
}
\$\endgroup\$
0

1 Answer 1

5
\$\begingroup\$

Comment on C++ interface:

You are doing work outside of ID that belongs as part of the class:

ID generator;

for(int i = 0; i < DATE_LENGTH; i++){
    *(generator.getDate() + i) = int(dateString[i]-'0');
}

generator.setYear(1900 + (*(generator.getDate() + 4) * 10) + *(generator.getDate() + 5));

while(i < 500){
    generator.individSiffer();
    generator.generate();
    i++;
}

Because this work is not part of a method you are exposing implementation details of your ID class via getter methods. All of which is bad:

A better interface would have been:

std::string date = getDate(); // Get a valid date or throw.
ID          generator(date);

std::cout << generator;

Comments on Code

Don't use this interface. What happens if the user types an extra long line of stuff.

cin.getline(dateString, BUFFER);

Its not going to crash but you are left with lots of crap on the input stream. User input is line based so read a line from the user and then try and parse the line. Because a line can be any length don't use that version of get line.

std::string line;
while(std::getline(std::cin, line))
{
    // Valid line has been read.
    if (validDate(line))
    {    return line;
    }
    std::cerr << "Invalid date please try again\n";
}
throw std::runtime_error("The user give up trying to enter a date correctly");

Comments on C functions

C based functions for printing printf() are dangerous, because they are not type safe. Prefer to use the C++ type safe streams.

Comments on Data values

generator.setYear(1900 + (*(generator.getDate() + 4) * 10) + *(generator.getDate() + 5));

What about people born after 31st Dec 1999?

Comments on Loop structures

while(i < 500){
    // STUFF
    i++;
}

// This kind of loop is better written as a for:

for(int loop=0; loop < 500; ++loop) {
    // STUFF
}

Also notice that I am declaring my loop variable as close to the point of usage as possible. Do not use the C style of putting all the variables at the top of the function. Put your variables where you can see there declaration and type easily.

\$\endgroup\$
1
  • \$\begingroup\$ Thank you very much! I'll do my best to implement as much of these changes as possible :) \$\endgroup\$
    – Nilzone-
    Jul 2, 2014 at 18:49

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.