2
\$\begingroup\$

Originally written in C (which was abysmal, you may check here if you want), I rewrote my simple credit card validation program using Python

def main():
    # Prompts user for card number
    nums = int(input("Number: "))
    if len(str(nums)) > 16 or len(str(nums)) < 12:
        print("INVALID")
    else:
        card_classify(card_luhn(nums), nums)

# Luhn's Algorithm
def card_luhn(nums):

    tmp = str(nums)
    digit_sum = 0
        
    # Loops through the digits that needs to be multiplied by 2
    for i in range(2, len(tmp) + 1, 2):
        digits = 2 * int(tmp[-i])

        # Condition if resulting product has 2 digits
        if_sum = 0
        if len(str(digits)) == 2:
            for digit in str(digits):
                if_sum += int(digit)
                digits = 0

        if_sum += digits
        digit_sum += if_sum

    # Loops through the digits that does not need to be multiplied by 2
    for i in range(1, len(tmp) + 1, 2):
        digits = int(tmp[-i])
        digit_sum += digits

    if digit_sum % 10 == 0:
        return 0
    else:
        return 1


# Classifies the card whether AMEX, VISA, or MASTERCARD
def card_classify(check, nums):

    num = int(str(nums)[:2])

    if check != 0:
        print("INVALID")

    # AMEX condition (first two digits from left are either 37 or 34)
    elif num == 37 or num == 34:
        print("AMEX")

    # VISA condition (first digit from left is 4)
    elif str(num)[:1] == "4":
        print("VISA")

    # MASTERCARD condition (first two digits from left are between 50 and 56)
    elif num > 50 and num < 56:
        print("MASTERCARD")

    else:
        print("INVALID")

if __name__ == "__main__":
    main()

Could someone please review my code if it follows good practice and style? I want to be more Pythonic as I continue to learn.

I also think that my implementation of Luhn's Algorithm could be improved, considering I looped over the card number twice.

\$\endgroup\$
3
  • \$\begingroup\$ Suggested reading: codereview.stackexchange.com/questions/256078/… \$\endgroup\$
    – Reinderien
    Sep 14 at 14:58
  • \$\begingroup\$ @Reinderien I see that you have no main() function in your implementation. Coming from C, I still find it hard not to put one. What, in your opinion, is more Pythonic? Or does that not matter at all (i.e. does not affect performance and readability)? \$\endgroup\$ Sep 14 at 19:55
  • \$\begingroup\$ It's a better idea to have a main, to promote code reusability and keep the global namespace clean. So that answer from February was a little hasty and should ideally have one. \$\endgroup\$
    – Reinderien
    Sep 14 at 20:08
4
\$\begingroup\$

Getting the First 2 Digits of a Number

A solution for this in constant space and time is already documented in this answer:

from math import log10

# go from this
num = int(str(nums)[:2])

# to this
def leading_digits(num):
    return num // 10 ** (int(math.log10(num)) - 1)

And checking for a type of card lends itself to using a dictionary

Credit Card Types

Replace your if/elif statements with a dictionary:

card_types = {
    37: 'AMEX',
    34: 'AMEX',
}

card_types.update({i: 'MASTERCARD' for i in range(51, 56)})
# these all start with 4
card_types.update({i: 'VISA' for i in range(40, 50)})

try:
    card_type = card_types[leading_digits(num)]
except KeyError:
    print("Invalid card type!")

card_luhn

I don't think this is a good name for this function. I think it's better as is_valid_card, which implies returning a boolean, which is technically being done here. I'd change the return to more explicitly be either True or False:

# instead of this
if digit_sum % 10 == 0:
    return 0
else:
    return 1


# do this
return bool(digit_sum % 10)

Checking for Two-Digit Numbers

You already know what the 2-digit numbers are, they are all in the range 10 <= x <= 99. So a comparison test here is the answer:

# go from this
if len(str(digits)) == 2:

# to this
if 9 < digits < 100:

Determining the Number of Digits in an Integer

Here is a good answer for that. It avoids casting to string unnecessarily

Getting each digit in a number

Using the previous point, we can construct a function that will give us each digit in the number provided:

from math import log10, ceil


def get_digits(number):
    if not number:
        yield 0
        return
    elif number < 0:
        # for a solution that processes negative numbers
        # however, for your case, it might be better to
        # raise a ValueError here
        number *= -1

    num_digits = ceil(log10(number))

    for _ in range(num_digits):
        digit = (number % 10)

        yield digit

        number //= 10

This has a few benefits:

  1. You aren't casting to string, avoiding unnecessary overhead
  2. This gives you the digits in the order you want (right to left)
  3. No more index lookups

We can use itertools.islice to get an iterator from digit 2 onward in steps of 2:

# go from this:
for i in range(2, len(tmp) + 1, 2):

# to this
# islice takes arguments:
#  iterator, start, stop, step
for digit in islice(get_digits(nums), 2, None, 2):

Unfortunately, islice doesn't take keyword arguments, so a comment might be useful to remind yourself what the arguments are This same snippet can be used on the odd digits as well.

Looping once

As you mentioned, you only need to iterate once over the digits, and you can use enumerate to track if you're on an even or odd index:

digits_iter = islice(get_digits(card_number), 1, None)

for i, digit in enumerate(digits_iter, start=1):
    if i % 2:
        # odd
    else:
        # even

Refactoring card_luhn

from math import log10, ceil
from itertools import islice
from typing import Iterator


def get_digits(number: int) -> Iterator[int]:
    """Yields the digits of an integer from right to left"""
    if not number:
        yield 0
        # stops the generator
        return
    elif number < 0:
        # for a solution that processes negative numbers
        # however, for your case, it might be better to
        # raise a ValueError here
        number *= -1

    num_digits = ceil(log10(number))

    for _ in range(num_digits):
        digit = (number % 10)

        yield digit

        number //= 10


def is_valid_card(card_number: int) -> bool:
    """Uses Luhn's algorithm to determine if a credit card number is valid"""
    digit_sum = 0

    digits_iter = islice(get_digits(card_number), 1, None)
    for i, digit in enumerate(digits_iter, start=1):
        # process odd digits
        if i % 2:
            digit_sum += digit
            continue

        doubled = 2 * digit

        # Condition if resulting product has 2 digits
        if 10 <= doubled <= 99:
            digit_sum += sum(get_digits(doubled))
        else:
            digit_sum += doubled


    return bool(digit_sum % 10)

Quite a few changes here. First, note I'm checking if the index is odd or even with the i % 2 operation. The continue keyword allows me to skip the rest of the loop body and go on to the next iteration. Next, I renamed digits to doubled, since digits isn't really what it is, it's twice the value of a digit. I'm using the int comparison operation to test if doubled is in a given range, which is fast and very readable. I've gotten rid of the if_sum temporary variable, simply sum the digits returned by the iterator and add that sum to digit_sum. Last, I'm returning a bool, so you get an explicit True or False from the function. I've also added type hints and some short docstrings to help with readability.

card_classify

Let's reorder these to classify_card. I don't think check needs to be a variable here. Call the is_valid_card function inside that function. It's easier to see what's happening that way:

def classify_card(card_number: int) -> None:
    """Classifies a card as AMEX, VISA, MASTERCARD or INVALID"""
    # test if the card number is valid
    if not is_valid_card(card_number):
        print("INVALID")
        return

    card_types = {
        37: 'AMEX',
        34: 'AMEX',
    }

    card_types.update({i: 'MASTERCARD' for i in range(51, 56)})
    # these all start with 4
    card_types.update({i: 'VISA' for i in range(40, 50)})

    try:
        card_type = card_types[leading_digits(num)]
    except KeyError:
        print("INVALID")
    else:
        print(card_type)

main

I'd postpone your integer conversion of the input string, since it's really easy to test the length while it's still a str:

def main():
    card_num = input("Number: ")

    if len(card_num) not in range(12, 17):
        print('INVALID')
    else:
        classify_card(int(card_num))

And I would move main towards the bottom of the script.

\$\endgroup\$
5
  • 2
    \$\begingroup\$ Awesome! I find this comprehensive review really well-explained. Thank you \$\endgroup\$ Sep 13 at 4:33
  • 2
    \$\begingroup\$ Hmmmm. You seem to want to micro-optimize beyond what's useful or probably even measurable (have you profiled the difference between string conversion and logarithms?), and some of your suggestions will harm performance rather than hurt it. For instance, rather than pushing for islice, you should simply have a tuple of integers and use regular [2:] slicing on it, which is both more legible and performant. \$\endgroup\$
    – Reinderien
    Sep 14 at 14:55
  • 1
    \$\begingroup\$ Have you checked that your algorithm is actually correct? It does not seem to pass the testcases here rosettacode.org/wiki/Luhn_test_of_credit_card_numbers#Python \$\endgroup\$ Sep 17 at 19:27
  • \$\begingroup\$ @Reinderien Looking back, I think you might be right. I'll have to edit this to be a better answer, can't delete it since it's accepted \$\endgroup\$
    – C.Nivs
    Sep 17 at 19:39
  • \$\begingroup\$ @N3buchadnezzar hm, good catch, I was going to take this down anyways, but I'll rerun test cases when I update this to be a more satisfactory review \$\endgroup\$
    – C.Nivs
    Sep 17 at 19:39
1
\$\begingroup\$

So C. Nivs gave a great review of your code, despite some minor flaws in the improvements. Take his advice at heart and you will be a great coder in no time. This review focuses more on how to properly implement Luhn's algorithm.

Reeinventing the wheel

While reinventing the wheel can be good sometimes, it is also important to look for existing resources before starting. If we look at Rosetta Code's webpage we find the following piece of code

>>> def luhn(n):
    r = [int(ch) for ch in str(n)][::-1]
    return (sum(r[0::2]) + sum(sum(divmod(d*2,10)) for d in r[1::2])) % 10 == 0
 
>>> for n in (49927398716, 49927398717, 1234567812345678, 1234567812345670):
    print(n, luhn(n))

Only two lines! The wikipedia page for Luhn's algorithm gives a similiar albiet longer code snippet

function checkLuhn(string purportedCC) {
    int nDigits := length(purportedCC)
    int sum := integer(purportedCC[nDigits-1])
    int parity := (nDigits-1) modulus 2
    for i from 0 to nDigits - 2 {
        int digit := integer(purportedCC[i])
        if i modulus 2 = parity
            digit := digit × 2
        if digit > 9
            digit := digit - 9 
        sum := sum + digit
    }
    return (sum modulus 10) = 0
}

Be careful though. Short code != readable code.

Any fool can write code that a computer can understand. Good programmers write code that humans can understand. (M. Fowler)

Make sure to always strife for code that is readable. Your code is on the verbose side, while the Rosetta code is very terse.

Test, test, test

Hmm, the code from Rosetta looks a bit cryptic (terse). Let us compare it with my first draft

from typing import Annotated

CreditCard = Annotated[int, "An integer representing a credit card number"]


def is_card_valid_1(number: CreditCard) -> bool:
    """Uses Luhn's algorithm to determine if a credit card number is valid

    1. Reverse the order of the digits in the number.
    2. Take the first, third, ... and every other odd digit in the reversed
       digits and sum them to form the partial sum s1
    3. Taking the second, fourth ... and every other even digit in the reversed digits:

       1. Multiply each digit by two and sum the digits if the answer is greater
          than nine to form partial sums for the even digits
       2. Sum the partial sums of the even digits to form s2

    If s1 + s2 ends in zero then the original number is in the form of a valid
    credit card number as verified by the Luhn test.

    For example, if the trial number is 49927398716:

    Reverse the digits:
      61789372994
    Sum the odd digits:
      6 + 7 + 9 + 7 + 9 + 4 = 42 = s1
    The even digits:
        1,  8,  3,  2,  9
      Two times each even digit:
        2, 16,  6,  4, 18
      Sum the digits of each multiplication:
        2,  7,  6,  4,  9
      Sum the last:
        2 + 7 + 6 + 4 + 9 = 28 = s2

    s1 + s2 = 70 which ends in zero which means that 49927398716 passes the Luhn test

    Example:
        >>> is_card_valid_1(0)
        True
        >>> any(map(is_card_valid_1, range(1,10)))
        False
        >>> [is_card_valid_1(i) for i in [59, 60]]
        [True, False]
        >>> [is_card_valid_1(i) for i in [49927398716, 49927398717]]
        [True, False]
        >>> [is_card_valid_1(i) for i in [1234567812345678, 1234567812345670]]
        [False, True]
    """
    digits = map(int, reversed(str(number)))
    check_sum = 0
    for i, digit in enumerate(digits):
        if i % 2:
            # The sum of the digits of a 2 digit number is number - 9
            # (15 = 1+5 and 15 - 9 = 6)
            digit = double if (double := 2 * digit) < 9 else double - 9
        check_sum += digit
    return check_sum % 10 == 0


if __name__ == "__main__":
    cards0 = [0, 1, 59, 60, 596, 567]
    cards1 = [79927398713]
    cards2 = [49927398716, 49927398717, 1234567812345678, 1234567812345670]

    print([is_card_valid_1(card) for card in cards0])
  • Count how many lines of my code is comments versus actual code.
  • Everything after Example: are doctests. This means I automatically add test cases, and check whether my implementation is correct.
  • I have some more test cases in my __main__ guard, just for testing the implementation.

See for yourself if you can figure out how my implementation works without me telling you. If my code is well written it will take you a short while. If it takes a while, it is a good sign that it could be improved.

Improvements on the first draft

I have yet to reach a consensus on this, but the following part sticks out to me

for i, digit in enumerate(digits):
    if i % 2:
        # The sum of the digits of a 2 digit number is number - 9
        # (15 = 1+5 and 15 - 9 = 6)
        digit = double if (double := 2 * digit) < 9 else double - 9
  1. It requires a comment that streches over two lines, and even then it is unclear at best.
  2. We know every other digit is even. even so we still check every iteration if i % 2. This is again a minor gripe.

We could "solve" the first problem by rewriting it using divmod(x,y) = x // y, x % y

for i, digit in enumerate(digits):
    check_sum += sum(divmod(2 * digit, 10)) if i % 2 else digit

Which still needs a comment explaining what it does. If speed is an concern (it really should not be, you are using Python and this is a miniscule micro-optimization) we could do

# Calculates the digit sum of 2*i for the i'th digit [2*9 = 18 and 1+8=9 so 9 maps to 9]
double_check_sum = {0: 0, 1: 2, 2: 4, 3: 6, 4: 8, 5: 1, 6: 3, 7: 5, 8: 7, 9: 9}
for i, digit in enumerate(digits):
    check_sum += double_check_sum[digit] if i % 2 else digit

The second problem could be solved by exploiting Pythons slice notation.

# Calculates the digit sum of 2*i for the i'th digit [2*9 = 18 and 1+8=9 so 9 maps to 9]
double_check_sum = [0, 2, 4, 6, 8, 1, 3, 5, 7, 9]
even, odd = sum(digits[::2]), sum(double_check_sum[odd] for odd in digits[1::2])

Where we converted our double_check_sum to a list, and now iterates separately over respectively the even and odd digits. To summerize our final function could look something like this

def is_card_valid_4(number: CreditCard) -> bool:
    digits = [int(i) for i in reversed(str(number))]
    # Calculates the digit sum of 2*i for the i'th digit 
    # [2*9 = 18 and 1+8=9 so 9 maps to 9]
    double_check_sum = [0, 2, 4, 6, 8, 1, 3, 5, 7, 9]
    even, odd = sum(digits[::2]), sum(double_check_sum[odd] for odd in digits[1::2])
    return (even + odd) % 10 == 0

Without the docstrings, which is not too shaby. If we wanted to be really pedantic, we could improve it further by extracting the constant and defining a sub function

# Calculates the digit sum of 2*i for the i'th digit
# [2*9 = 18 and 1+8=9 so 9 maps to 9]
DOUBLE_DIGIT_SUM = [sum(int(i) for i in str(2 * i)) for i in range(10)]


def is_card_valid_4(number: CreditCard) -> bool:
    digits = [int(i) for i in reversed(str(number))]
    even, odd = sum(digits[::2]), sum(DOUBLE_DIGIT_SUM[i] for i in digits[1::2])
    return (even + odd) % 10 == 0

Note that DOUBLE_DIGIT was calculated explicitly to increase the readability and remove the "magic list". Since DOUBLE_DIGIT is only calculated once we can afford to be slow.

Pythonic

Another approach is also grounded in the divmod approach

def is_card_valid_5(number: CreditCard) -> bool:
    check_sum = 0
    while number:
        number, even_digit = divmod(number, 10)
        number, odd_digit = divmod(number, 10)
        # sum(divmod(2*7, 10)) = sum((1, 4)) = 5 [calculates the digit sum of twice the number]
        check_sum += even_digit + sum(divmod(2 * odd_digit, 10))
    return check_sum % 10 == 0

I'll let you figure out the details here. When does the iteration stop? What does the sum(divmod(2 * odd_digit, 10)) do? Does it work for single and double digits? Does it iterate from the last digit as required in the algorithm?

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