Generate the Norwegian equivalent to social security numbers - follow-up

Question

First of, here is my previous post.

That should explain a fair bit. The reason I'm posting one more time, is I have changed a lot. Using more classes, let them do all the work, and the overall structure.

#include <iostream>

const int DATE_LENGTH = 6;

class Date {
    private:
        char *ans;
    protected:
        int date[DATE_LENGTH];
        int year;
    public:
        Date() {
            askForDate();
        }

        ~Date() {
            if(ans) { delete ans; }
        }

        void askForDate() {
            using namespace std;
            ans = new char[DATE_LENGTH];
            cout << "Date of Birth (DDMMYY): ";         cin.getline(ans, DATE_LENGTH + 1);
            while (!validDate(ans)) {
                cin.getline(ans, DATE_LENGTH + 1);
            }

            for (int i = 0; i < DATE_LENGTH; i++) { date[i] = int(ans[i] - '0'); }
            year = 1900 + date[4] * 10 + date[5];
        }

        bool validDate(const char *d) {
            return strlen(d) == 6;
        }
};

class IndividSiffer : public Date {
    protected:
        int i1, i2, i3; 
    public:
        IndividSiffer(int ii1, int ii2, int ii3) : i1(ii1), i2(ii2), i3(ii3) {}

        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;
            }
        }
};

class KontrollSiffer : public IndividSiffer {
    protected:
        int k1, k2;
    public:
        KontrollSiffer(int kk1 = 0, int kk2 = 0, int i1 = 0, int i2 = 0, int i3 = 0) : IndividSiffer(i1, i2, i3), k1(kk1), k2(kk2) {
            for (int index = 0; index < 500; index++) {
                individSiffer();
                generate();
            }
        }

        void generate() {
            using namespace std;
            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++) { cout << date[i]; }
                cout << i1 << i2 << i3 << k1 << k2 << endl;
            }   
        }
};

int main(int argc, char *argv[]) {
    KontrollSiffer k;
    return 0;
}

I'd really like some input on how I structured it, or if things should be done differently.

Answer 1

Here are some suggestions which may help you improve your code.

Don't abuse using namespace std

Putting using namespace std within your program is generally a bad habit that you'd do well to avoid.

Use the appropriate #includes

The code uses the strlen() function a number of places but does not #include <cstring> which is where that library function is defined.

Eliminate unused variables

This code declares main with arguments argc and argv in the usual way, but neither argc nor argv are actually used. This could be addressed by either declaring int main() or by modifying the code to actually use the variables. There is a third way, which is to declare it as int main(int, char *[]) but this is not recommended because it's a bit confusing to read.

Separate I/O from object creation

The Date class should not do I/O directly to std::cout and std::cin. Better design would be to create a custom inserter and custom extractor for the class to allow but not to force I/O operations for the creation of an object. For example, your Date class might have the following two definitions:

std::ostream &operator<<(std::operator &out, const Date &d);
std::istream &operator>>(std::operator &in, Date &d);

The same is true of the KontrollSiffer class.

Perform better error checking for user input

As it is currently written, one can type in "Norway" as the Date and the code proceeds to proces that input as though it were a valid date, producing invalid results. Better would be to include more checking beyond just whether the string is 6 characters long.

Prefer std::string to raw char *

The Date class uses a raw char * but would probably be better served by using std::string in this context.

Use aggregation rather than inheritance for composition

The inheritance mechanism is generally suited for "is-a" relationships. For example, if one had a Birthday class, it might reasonably be derived from a Date base class, since a Birthday is a Date. However, an IndividSiffer is not really a Date, but is rather constructed from one, so a better design might either have a Date as a private instance variable with the IndividSiffer class or to simply accept a const Date & variable as an argument to an IndividSiffer constructor.

Don't keep variables you don't need

The ans variable is only actually used within the askForDate() routine, so there's no reason to keep it as part of the Date class. Better would be to declare it as a local variable within the askForDate() function.

Prefer to avoid using new and delete directly

The ans variable within the Date::askForDate() function is small enough to easily fit on the stack, so rather than using new to allocate it, better would be to declare it locally within the askForDate function as:

char ans[DATE_LENGTH];

That way, it is automatically created with the correct length and then discarded when the function is complete.

Think carefully about static variables

The code for IndividSiffer::individSiffer() uses a static variable individ which is a problem. When you create two IndividSiffer instances in the same program, only the first one will have the variable initialized. That is because a static variable only gets initialized once before the program is run. This very greatly limits the use of these classes.

Consider using a standard container

The effect of the KontrollSiffer class is to create and print 500 individual numbers. Rather than simply print and discard them, it might be more sensible for general use to create a collection of them, perhaps in a std::vector<IndividSiffer> for example.

Omit return 0

If your program completes successfully, the return 0 at the end of main() will be generated automatically, so it's not needed in C++ programs.

Answer 2

@Edward has mentioned many great points, but I still have several things to add:

• Since you only use DATE_LENGTH within the class (and because it's an integer type), you may make it a private member:

  private static const int DATE_LENGTH = 6;
  

  You could also consider naming it in either camelCase or snake_case since all-uppercase is usually reserved for macros (#defines).

• It's discouraged to have variables declared/initialized on the same line:

  int i1, i2, i3;
  

  Although they appear to be similar, you still want to make sure that they stand out case they need to be modified or commented:

  int i1;
  int i2;
  int i3;
  

  In addition, there is one fundamental issue: the use of Hungarian notation. The i prefix would refer to int, but it's already clear that they're ints because C++ is a strongly-typed language. Instead, ditch this notation, and name these variables based on their real purpose.

• Instead of having the initializer list items horizontally:

  IndividSiffer(int ii1, int ii2, int ii3) : i1(ii1), i2(ii2), i3(ii3) {}
  

  you can have them vertically:

  IndividSiffer(int ii1, int ii2, int ii3)
      : i1(ii1)
      , i2(ii2)
      , i3(ii3)
  {}
  

  In this way, the list won't reach across the screen, and it'll be easier to add/remove items.

  Another thing to be aware of is that, when using an initializer list, argument and member names can be the same (without using this->). The compiler can tell them apart when used this way.

• Since validateDate() doesn't modify any data members, it should be const. You should also replace the "magic number" with your DATE_LENGTH constant.

  bool validDate(const char *d) const {
      return strlen(d) == DATE_LENGTH;
  }
  

• This is a pretty long line:

  KontrollSiffer(int kk1 = 0, int kk2 = 0, int i1 = 0, int i2 = 0, int i3 = 0) : IndividSiffer(i1, i2, i3), k1(kk1), k2(kk2) {
  

  If you really cannot simplify it yourself, considering wrapping it:

  KontrollSiffer(int kk1 = 0, int kk2 = 0, int i1 = 0, int i2 = 0, int i3 = 0)
      : IndividSiffer(i1, i2, i3), k1(kk1), k2(kk2) {