4
\$\begingroup\$

I have written some code in Node.js that takes a date as argument and return all the possible combination of social security numbers.

I'd really appreciate some advice when it comes to these areas:

  • Coding style

  • Code structure

In my code I have included a brief description on how these numbers are generated.

personnr.js

/*
Norwegian social security-number structure;

dd mm yy i1 i2 i3 k1 k2


i1, i2, i3 => 'Individ'-numbers

This structure is used pr. oct. 2013;

000–499 for people born some time in 1900–1999.
500–999 for people born some time in 2000–2039.

k1, k2 => calculated based on the numbers above (a checksum)


more info;

https://no.wikipedia.org/wiki/Fødselsnummer
*/


module.exports = init;


function Personnr(birthday) {
    if (typeof birthday !== 'string')   throw new Error('Argument must be a string');
    if (birthday.length !== 8)          throw new Error('Argument has to be on the format \'DDMMYYYY\'')

    var dd      = addZero(birthday, 0, 2),
        mm      = addZero(birthday, 2, 4),
        yy      = addZero(birthday, 6, 8),
        yyyy    = addZero(birthday, 4, 8)

    this.birthday   = [dd, mm, yy].join('');
    this.dd         = dd;
    this.mm         = mm;
    this.yy         = yy;
    this.yyyy       = yyyy;

    this.individ = {
        i1: 0,
        i2: 0,
        i3: 0
    };
}

Personnr.prototype.make = function() {
    var range       = getRange(this.yyyy),
        min         = range[0],
        max         = range[1],
        generate    = generateIndividSiffer(min, this.yy, this.individ),
        result      = [];

    for(var i = min; i < max; i++) {
        var individ = generate();
        var kontroll = generateKontrollSiffer(this.dd, this.mm, this.yy, this.individ);

        var personnr = [this.birthday, individ, kontroll].join('');
        result.push(personnr);
    }

    return result;
}


function init(birthday) {
    return new Personnr(birthday);
}



//Private functions



//If the number is < 10, an extra '0' is inserted before (e.g. 08, 09 etc..)

function addZero(number, min, max) {
    if (number.length === 2) {
        var number = parseInt(number.slice(min, max)),
            result = ('0' + number).slice(-2);   
    }

    return result || number.slice(min, max);
}


function getRange(year) {
    var min = null,
        max = null;

    if (year >= 1900 && year <= 1999) {
        min = 0;
        max = 499;
    } else if (year >= 2000 && year <= 2039) {
        min = 500;
        max = 999;
    }

    return [min, max];
}


function generateIndividSiffer(start, year, individ) {
    var increment = start;

    return function() {
        increment++;

        individ.i3 = increment % 10;

        if (individ < 100){
            individ.i1 = 0;
            individ.i2 = parseInt(increment / 10) % 10;
        } else if (increment < 10) {
            individ.i1 = increment.i2 = 0;
        } else {
            individ.i1 = parseInt(increment / 100) % 10;
            individ.i2 = parseInt(increment / 10) % 10;
        }

        return [individ.i1, individ.i2, individ.i3].join('');
    }
}

function generateKontrollSiffer(day, month, year, individ) {

    var d1  = parseInt(day / 10),
        d2  = day % 10,

        m1  = parseInt(month / 10),
        m2  = month % 10,

        y1  = parseInt(year / 10),
        y2  = year % 10,

        i1  = individ.i1,
        i2  = individ.i2,
        i3  = individ.i3;


    var k1 = 11 - ((3*d1 + 7*d2 + 6*m1 + 1*m2 + 8*y1 + 9*y2 + 4*i1 + 5*i2 + 2*i3) % 11); 
    if (k1 > 9) k1 = 0; 

    var k2 = 11 - ((5*d1 + 4*d2 + 3*m1 + 2*m2 + 7*y1 + 6*y2 + 5*i1 + 4*i2 + 3*i3 + 2*k1) % 11);
    if (k2 > 9) k2 = 0; 

    return [k1, k2].join('');

}

test.js

var personnr = require('./personnr');

var results = personnr('16111992').make();
console.log(results);
\$\endgroup\$
  • 1
    \$\begingroup\$ i3 tells us the sex of the person. Males have odd numbers, females have even numbers. \$\endgroup\$ – Revetahw says Reinstate Monica Apr 29 '16 at 4:24
  • 1
    \$\begingroup\$ Sorry I don't have time to answer now, but this entire problem can be solved in 10-20 lines of code at most. The current solution is over-complicating things. I'd start by avoiding objects and just using simple functions. \$\endgroup\$ – Jonah Apr 30 '16 at 19:29
  • \$\begingroup\$ @Jonah I'd really appreciate a review when you have the time. \$\endgroup\$ – Nilzone- Apr 30 '16 at 21:13
  • \$\begingroup\$ Follow-up question \$\endgroup\$ – 200_success May 3 '16 at 19:44
1
\$\begingroup\$

This isn't a full answer, but hopefully will give you a few helpful hints. Let's start with some low-hanging fruit and rewrite getRange:

function getRange(year) {
  return isBetween(1900, 1999, year) ? [0, 499]   :
         isBetween(2000, 2039, year) ? [500, 999] : [null, null];
  function isBetween(min, max, x) { return x >= min && x < max }
}

Another easy improvement is to use ES6 destructuring assignments. So you can rewrite something like:

var range       = getRange(this.yyyy),
    min         = range[0],
    max         = range[1];

as:

 var [min, max] = getRange(this.yyyy);

Similarly, use a helper function to avoid repetition such as this:

 var d1  = parseInt(day / 10),
     d2  = day % 10,

     m1  = parseInt(month / 10),
     m2  = month % 10,

     y1  = parseInt(year / 10),
     y2  = year % 10,

Rewrite:

 var [d1, d2]  = divmod(day, 10),
     [m1, m2]  = divmod(month, 10),
     [y1, y2]  = divmod(year, 10),

 function divmod(n, d) { return [parseInt(n/d), n % d] }

Those are all somewhat cosemetic things, though. They'll tighten up the code and save you lines, but there's deeper issues to be fixed too...

The worst offender imo is generateIndividSiffer(start, year, individ) because it's a factory function, and it returns a function which is not referentially transparent. That is, the returned function mutates the increment var it closes over, and even more confusingly mutates individ, a member variable. It's very difficult to reason about such functions. Rewrite that logic so that you use only pure functions -- functions guaranteed to return the same results for the same input.

Hope that helps as a start...

\$\endgroup\$
  • \$\begingroup\$ I don't know where the a and b comes from in the isBetween method, but other than that - thanks a lot for the tips! \$\endgroup\$ – Nilzone- May 1 '16 at 8:05
  • \$\begingroup\$ Woops, just forget to rename everything. I fixed it. \$\endgroup\$ – Jonah May 1 '16 at 17:15
1
\$\begingroup\$

Here are some general critique on the code. I'm not that familar with the specifics of javascript so I'll leave that to people who knows this better.


Validate input

When creating the object you should validate the brithday making sure it's correct. This can be done by adding a check_birthday function like e.g.

function check_birthday(day, month, year){

   // Check if month is in [1,12]
   if(month < 1 || month > 12) return false;

   // Check if day is valid given the month (i.e. < 31, 30 (29 if leap year), 31, ...

   // Check that year is valid (get_range assumes [1900,2039])
   if(year < 1900 || year > 2039) return false;

   return true;
}

See this page for how the day-check can be implemented (taking leaps-years into account). Another, option is to take the input of the function to be a birthday string instead.


Functions should do what the name says it does

The addZero function (as it's used) looks useless / does not really do what it says it does. For example for dd you send in a string ddmmyyyy, extract the [0,2] part of the string giving us dd, parse that as an int, add a zero if the number is less than 10 and then return it as a string. This should be equivalent of just extracting the dd part (given that the string is a valid birthday-string) thus the only things this function really does is to extract the [min,max] part of the string and does not add any zeros.

I would do the splitting of birthday into dd, mm and yyyy outside of addZero, i.e. in the initialization.

dd = birthday.slice(0, 2);

If you need an addZero function I would make this more generic (which allows it to be used for other stuff if needed) and make sure it does exactly what the name suggest: it should take in a string/number and output a string of a given size with leading zero(s) added. Here is an example of how one can do this:

function addLeadingZeros(number, length){
   var paddednumber = '' + number;

   // Here we just return number / show error message that length
   // of number is larger than the required length
   if(number.length > length) return paddednumber;

   // Pad number with zeros
   while (paddednumber.length < length) {
     paddednumber = '0' + paddednumber;
   }      
   return paddednumber;
}

Other things to consider

As pointed out by Fiksdal, the third control digit i3 gives the sex of the person. If you are interested in separating between males and females when making the allowed numbers then the sex should be part of the class which can be set by changing personnr(number) to personnr(number, sex) adding sex to the individ struct and in make you need to add a check for this if(individ.i3 % 2 == individ.sex) result.push(personnr); (here for simplicity assuming sex is stored as 0 for women and 1 for men).

Right now you store the day, month and year as a string. This might be good for the intended use, but I would also consider making these into integers as this just sounds more natural to me. If you also need them as a string then this can easily be done with a addZero function.

\$\endgroup\$

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.