Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'm doing some exercises in Python and this one is about using the Luhn-algorithm to calculate the checksum digit in Swedish personal identification numbers.

The description is the same as this question:

Given a string of 9 digits, abcdefghi compute:

array = [a*2, b, c*2, d, e*2, f, g*2, h, i*2]

Then you compute the sum of the digits in this array, so for example if a*2 is 16, then that part counts as 1 + 6

Then the result is how much more you have to add to make it evenly divisible by 10. For example, if sum == 54 then the result is 6 as 60 - 54 = 6.

def control_numb(nbr_string):
    cut_string = nbr_string[:9]  ## We need 9 numbers to calculate the 10th
    mult_numb = [2,1]            ## we should alternate multiplying 2 and 1 starting with 2
    i = 0                        ## holds the index of which mult_numt to use. starting with index 0

    sum_of_digits = 0
    for s in cut_string:         ## loop through the string of digits
        for d in str(int(s) * mult_numb[i]):  ## use s as int to do the multiplication. then turn back to str to loop through digit
            sum_of_digits += int(d)
        i = (i+1)%2

    return (10 - sum_of_digits % 10) % 10

I'm pretty new to Python and would very much like feedback, if there are any pythonic way to do things please spread the knowledge. Or just tear the code apart completely. Any comments are welcome!

share|improve this question

2 Answers 2

up vote 6 down vote accepted

Some comments:

  • Variable names could be improved a little bit. For example, I think nbr_string could be better as swedish_id
  • Please add a docstring (PEP257)
  • itertools.cycle is useful to iterate multiple times over the same sequence (in this case, the multiplication factors)
  • Use list comprehensions where possible instead of for loops

Things that might be different from the question's code:

  • I'm assuming the initial string has 9 digits, but if it has already 10, then go with your code to keep only the first 9 numbers, but note that you can use some assertions to be sure the data passed is correct.
  • I've calculated partial sums to apply the algorithm as in the explanation, but doing the sums as in the question should be fine.

Please find my solution based on your code below:

from itertools import cycle


def control_number(swedish_id):
    """Calculate control number in Swedish personal IDs

    :param swedish_id: A Swedish personal ID
    :type swedish_id: str

    """
    assert swedish_id.isdigit()
    assert len(swedish_id) == 9

    # Multiplication factors for each digit depending on its position
    mult_factors = cycle([2, 1])

    def partial_sum(number, mult_factor):
        """Calculate partial sum ofr a single digit."""
        quotient, remainder = divmod(number * mult_factor, 10)
        return quotient + remainder

    final_sum = sum(
        partial_sum(int(character), mult_factor)
        for character, mult_factor in zip(swedish_id, mult_factors))

    # Calculate control number based on partial sums
    control_number = -final_sum % 10

    return control_number

I hope this helps.

share|improve this answer
1  
As a quick review to the review : you don't actually need to built the partial_sums list as you could just write generator expression and use sum : final_sum = sum(partial_sum(number, mult_factor) for number, mult_factor in zip(numbers, mult_factors)). In the same kind of idea, the numbers` list is not required (you can call the int function as you use it). Also, you could use divmod in your partial_sum function : q, r = divmod(n*m, 10); return q+r. –  Josay Jul 24 at 13:08
    
@Josay Thanks for your feedback. I've updated the response following your advice and now it looks definitely better. In particular, divmod fits really good. –  jcollado Jul 24 at 13:29
    
Thank you very much for the answer. It's very contructiv. :) A comment on control_number = 10 - (final_sum % 10). If final_sum % 10 = 0 then the the function returns 10. It should only return a single digit which in that case would be 0. –  André Samuelsson Jul 24 at 15:10
1  
Aren't the following values equal : (10 - (final_sum % 10)) % 10 ; (- (final_sum % 10)) % 10 ; (-final_sum % 10) % 10 ; -final_sum % 10 ? –  Josay Jul 24 at 15:36
1  
@AndréSamuelsson That's true for python, but not true for many other programming languages; so if you do use that, add a comment explaining what it does. –  Joe Jul 24 at 18:07

Interface design

Avoid arbitrarily shortening names of functions — it makes it hard to call them properly. (You chose "numb"; others might have chosen "num" or "nbr" — which you actually did in the parameter name.)

Since the function accepts a string, it should also return a string. That would make it convenient to for the caller to concatenate the result after the original non-checksummed version.

ID number formats

According to Wikipedia, Swedish Personal Identification numbers may have a + or - sign after the birthdate. You should ignore it gracefully.

Also, the numbers may sometimes be written with a four-digit year. However, the checksum digits would still be based on the version with a two-digit year. I suggest normalizing the input using a regular expression.

Suggested implementation

from itertools import cycle
import re

def control_number(nbr_string):
    def sum_digits((digit, multiplier)):
        sum = int(digit) * multiplier
        return sum if sum < 10 else sum - 9
    # Keep just the relevant digits
    nbr_string = re.sub(r'\d{2}?(\d{6})[+-]?(\d{3})$', r'\1\2', nbr_string)

    total = sum(map(sum_digits, zip(nbr_string, cycle([2, 1]))))
    return str(10 - total % 10)
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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