7
\$\begingroup\$

I've written some Python-code to generate the Norwegian National Identification Numbers.

Being fairly new to Python, I'd really appreciate a review on this code in regards to best practices and style.

import calendar

def is_valid_date(d):
    dd,mm,yyyy = [int(i) for i in [d[:2],d[2:4],d[4:]]]
    days = {
        1: 31, 2: 29 if calendar.isleap(yyyy) else 28, 3: 31, 4: 30, 5: 31, 6: 30, 7: 31, 8: 31, 9: 30, 10: 31, 11: 30, 12: 31
    }

    if yyyy < 1900 or yyyy > 2039: return False
    if mm < 1 or mm > 12: return False
    if dd < 1 or dd > days[mm]: return False

    return True


def get_range(year):
    if year >= 1990 and year <= 1999: return range(500)
    if year >= 2000 and year <= 2039: return range(500,1000)
    return range(0)


def pad(n):
    return '{:03}'.format(n) if n < 100 else n

def generate(d):
    if type(d) is not str or is_valid_date(d) != True:
        raise ValueError('Expected a valid date-string with the form: ddmmyyyy')

    year, dt = int(d[4:]), [int(i) for i in d]
    for i in get_range(year):
        n = map(int,list(str(pad(i))))
        k1 = 11 - ((3*dt[0] + 7*dt[1] + 6*dt[2] + 1*dt[3] + 8*dt[-2] + 9*dt[-1] + 4*n[0] + 5*n[1] + 2*n[2]) % 11)
        k2 = 11 - ((5*dt[0] + 4*dt[1] + 3*dt[2] + 2*dt[3] + 7*dt[-2] + 6*dt[-1] + 5*n[0] + 4*n[1] + 3*n[2] + 2*k1) % 11)
        if k1 < 10 and k2 < 10: 
            print ''.join(map(str,dt[:4] + dt[6:] + n + [k1,k2]))


generate('31101990')
\$\endgroup\$
8
\$\begingroup\$

I only started Python about 3 months ago, so take my advice with a grain of salt.

(also my first review here :)

PEP8

Try to follow the PEP8 styleguide. Most of the issues I have with this are some whitespace issues. For example:

  • Two empty lines between import calendar and def is_valid_date(d):
  • Two empty lines between def pad(n): and def generate(d):
  • Put some whitespace between variable declaration and operators for increased readability. See dd, mm, yyyy = [int(i) for i in [d[:2],d[2:4],d[4:]]]
  • Most variable names in the functions don't really have descriptive names. For example d of the is_valid_date or generate
  • Try not to put if-statements on a single line. (Not sure if this is PEP8, but still a good practice).
  • None of your functions have docstrings. Documenting your code is great if you need to modify the code later on. Try not to reiterate how the function does something (e.g. explaining the code - that's what the code is for), but rather on why you chose to make this function.

Now on to actual code.

  • The days dictionary of the is_valid_date function is not being modified. Rather than creating it every time the method is called, perhaps consider making it a global variable, and making it more readable:
days = {
    1: 31,
    2: 29 if calendar.isleap(yyyy) else 28,
    3: 31,
    4: 30,
    5: 31,
    6: 30,
    7: 31,
    8: 31,
    9: 30,
    10: 31,
    11: 30,
    12: 31
}

def is_valid_date(date):
    ... 

However another way of doing this, is by using the already imported calendar module. This has the function calendar.monthrange(year, month), which also returns the number of days in the given month. This automatically takes care of leap years or other irregularities. Be wary that this returns 2 values though. To return only the amount of days in a month, you could use calendar.monthrange(year, month)[1].

  • In pad(n) you return either a formatted string, or a number. This means the there are two return types. You could have it return a string always:
def pad(n):
    return '{:03}'.format(n) if n < 100 else str(n)

or

def pad(n):
    n = str(n)
    if n < 100:
        return n.zfill(3)
    return n

which makes it a bit more readable. However, the pad function is not necessary, as the string.zfill(width) function can take of that for you:

>>>'75'.zfill(3)
'075'
>>>'655'.zfill(3)
'655'
>>>'{:03}'.format(75)
'075'
>>>'{:03}'.format(655)

Both functions only pad if necessary. This means you can do str(n).zfill(3) instead of the entire pad function.

  • In generate(d), your string type check is not python2 and 3 compatible. See this link for a better answer on string type checking.
  • The first line of the generate(d) function check for != True. As is_valid_date already returns a boolean value, using not would be a better choice:
def generate(d):
    if not isstring(d) or not is_valid_date(d):
        raise ValueError('Expected date string in format ddmmyyy, but got {}'.format(d))

Some more!

In get_range(year) you use

if year >= 1990 and year <= 1999:
    return range(500)

but this can also be written as:

if 1990 <= year <= 1999:
    return range(500)

or even:

if year in range(1990, 2000):  # more 'human' like. 
    return range(500)

... which are both slightly more succinct. The first option (interval comparison) can also be applied in the is_valid_date function:

if 1900 > yyyy > 2039:
    return False
if 1 > mm > 12:
    return False
if 1 > dd > calendar.monthrange(yyyy, mm)[1]:
    return False

Another completely different direction you could go into, is converting the given date string to a datetime object:

from datetime import datetime

birthdate = datetime.strptime('%d%m%Y', datestring)

A nice bonus that comes with this, is that date validation is already done, as it will throw a ValueError if it's given an invalid date, thus eliminating the entire is_valid_date function.

This answer is already long enough, so I will keep it at that :)

\$\endgroup\$
  • 1
    \$\begingroup\$ Good job on your first answer :) ! \$\endgroup\$ – яүυк Jan 2 '17 at 21:09
3
\$\begingroup\$

I would like to comment on this block of code:

    k1 = 11 - ((3*dt[0] + 7*dt[1] + 6*dt[2] + 1*dt[3] + 8*dt[-2] + 9*dt[-1] + 4*n[0] + 5*n[1] + 2*n[2]) % 11)
    k2 = 11 - ((5*dt[0] + 4*dt[1] + 3*dt[2] + 2*dt[3] + 7*dt[-2] + 6*dt[-1] + 5*n[0] + 4*n[1] + 3*n[2] + 2*k1) % 11)
    if k1 < 10 and k2 < 10: 
        print ''.join(map(str,dt[:4] + dt[6:] + n + [k1,k2]))

The lines k1 = and k2 = are very complex, so you should add a link to the technical document from which this calculations were taken.

The if makes the function fail silently if the calculations are wrong and the result is not a single digit, better fail with an error message.

The print makes this function single purpose, return would allow re-use inside a bigger program.

\$\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.