3
\$\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\$
4
  • \$\begingroup\$ Suggested reading: codereview.stackexchange.com/questions/256078/… \$\endgroup\$
    – Reinderien
    Commented Sep 14, 2021 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\$ Commented Sep 14, 2021 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
    Commented Sep 14, 2021 at 20:08
  • \$\begingroup\$ It wouldn't be so "abysmal" if you'd store the credit card in a string, as you should have. \$\endgroup\$
    – Harith
    Commented Mar 19 at 17:04

3 Answers 3

5
\$\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\$
6
  • 2
    \$\begingroup\$ Awesome! I find this comprehensive review really well-explained. Thank you \$\endgroup\$ Commented Sep 13, 2021 at 4:33
  • 3
    \$\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
    Commented Sep 14, 2021 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\$ Commented Sep 17, 2021 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
    Commented Sep 17, 2021 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
    Commented Sep 17, 2021 at 19:39
4
\$\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\$
2
\$\begingroup\$

Follow the specs

Credit card numbering is a well-defined system, standardized in ISO/IEC 7812. While accessing the standard text is expensive, the outline of its contents is well documented and available on the internet, for example on Wkipedia's Payment card number and ISO/IEC 7812 articles.

In particular, credit card numbers can have 8 to 19 digits, which is a wider range than you check against.

Use the correct data types

Although it's called a credit card number, it's really a string of digits, and should be handled as a string. The difference is that:

  • no arithmetic is expected to be performed on that number as a whole (although some may be performed on individual digits)
  • leading zeroes are significant

Converting that string to an int will discard leading zeroes, which can be problematic: although it's highly unlikely to encounter leading zeroes in a credit card number in the real world, it can be a valid and legitimate CC number.

Note: The same logic applies to other digit strings one may encounter: phone numbers, postal codes, PINs...

Single responsibility

The semantics of validating and classifying are somewhat mixed, with validity and classification information leaking into each other.

A valid CC number can be issued by another company than the handful you picked, yet will be classified as INVALID by your code. Conversely, a user can make a typo when entering their Visa card number, it should be possible to classify the number both as a Visa number and being invalid.

Also, all validation should be done in a dedicated function, instead of doing it at the call site in main().

Write tests

It's always good practice to write tests for your code, testing, if possible, each code path and try to identify edge cases (such as leading zeroes).

Use a unit testing module, write tests early, and run them often as you work on the code.

Document your code

Include doctrings with all your function, modules and classes, so that users can easily know what to expect when calling your code.

My attempt

creditcard.py:

'''
Classification and validation utilities for credit card numbers
'''

from string import digits


MIN_CREDIT_CARD_LEN = 8
MAX_CREDIT_CARD_LEN = 19
ISSUER_PREFIXES = {
    'American Express' : ['34', '37'],
    'Diners Club International' : ['36'],
    'Diners Club US & Canada' : ['55'],
    'Mastercard' : [str(i) for i in range(51, 56)]
                   + [str(i) for i in range(2221, 2721)],
    'Visa' : ['4'] ,
    }


def is_valid(credit_card: str) -> bool:
    '''
    Validate a credit card number.
    Validation takes into account:
        * length
        * characters used
        * conformance to Luhn's algorithm.
    '''
    if len(credit_card) < MIN_CREDIT_CARD_LEN:
        return False
    if len(credit_card) > MAX_CREDIT_CARD_LEN:
        return False
    if any(not char.isdigit() for char in credit_card):
        return False
    return is_valid_luhn(credit_card)


def is_valid_luhn(digits: str) -> bool:
    '''
    Validates a string of digits using Luhn's algorithm
    '''
    total = 0
    parity = len(digits) % 2
    for digit in digits:
        number = int(digit)
        if number % 2 != parity:
            total += number
        elif number > 4:
            total += 2 * number - 9
        else:
            total += 2 * number
    return int(digits[-1]) == (10 - total % 10)


def classify(credit_card: str) -> str:
    '''
    Identify a credit card issuer and return its name or 'Unknown' if not
    recognized.
    '''
    for issuer, prefixes in ISSUER_PREFIXES.items():
        for prefix in prefixes:
            if credit_card.startswith(prefix):
                return issuer
    return 'Unknown'

creditcard_tests.py

import unittest

import creditcard


class CreditCardValidatorTest(unittest.TestCase):
    
    def test_valid(self):
        '''
        Valid credit card numbers should be accepted
        '''
        self.assertTrue(creditcard.is_valid('56851138'))
        self.assertTrue(creditcard.is_valid('2481248298978423'))
        self.assertTrue(creditcard.is_valid('6164061904329715'))
        self.assertTrue(creditcard.is_valid('3613507956002405231'))

    def test_leading_zero(self):
        '''
        Leading zeroes should be preserved as they can be part of a valid
        credit card number
        '''
        self.assertTrue(creditcard.is_valid('0489640649825386'))
        self.assertTrue(creditcard.is_valid('00257244'))
        
    def test_too_long(self):
        '''
        Long credit card numbers should be rejected
        '''
        self.assertFalse(creditcard.is_valid('17074979049476411457'))
        self.assertFalse(creditcard.is_valid('0921491036436927563263160686624207812902755913733142086594542293344528702823109059195262880064037102993465751871589294259615326286013827592449554081918080751219175989641907785856332782918862737128870105790999970925166149230727633784854585714044398353489376'))
    
    def test_too_short(self):
        '''
        Short credit card numbers should be rejected
        '''
        self.assertFalse(creditcard.is_valid('9156186'))
        self.assertFalse(creditcard.is_valid('05'))
    
    def test_empty(self):
        '''
        Empty inputs should be rejected
        '''
        self.assertFalse(creditcard.is_valid(''))
    
    def test_invalid_luhn(self):
        '''
        Numbers that do not verify Luhn's algorithm should be rejected
        '''
        self.assertFalse(creditcard.is_valid('56851238'))
        self.assertFalse(creditcard.is_valid('2481248298978523'))
        self.assertFalse(creditcard.is_valid('3619507956002405231'))
    
    def test_classify(self):
        self.assertEqual('American Express', creditcard.classify('3458330426637464'))
        self.assertEqual('American Express', creditcard.classify('3706942381458678'))
        
        self.assertEqual('Diners Club International', creditcard.classify('3614306476144639'))
        
        self.assertEqual('Diners Club US & Canada', creditcard.classify('5575806258116273'))
        
        self.assertEqual('Mastercard', creditcard.classify('5297767827638758'))
        self.assertEqual('Mastercard', creditcard.classify('2532589251540593'))
        
        self.assertEqual('Visa', creditcard.classify('4911501473412258'))
        
        self.assertEqual('Unknown', creditcard.classify('8276832167166578'))
        
        


if __name__ =='__main__':
    unittest.main()

Note: all credit cards numbers used in tests were generated randomly.

Going further

These considerations are left as an exercise for the reader:

  • End users might include extraneous characters such as spaces or hyphens in between digits, and/or leading and trailing whitespace. A method for normalizing input could be useful.
  • Conversely, reading 10+ digit numbers is hard. A utility for pretty-printing credit card numbers would be nice.
  • Most credit card issuers do use Luhn's algorithm to validate credit card numbers, but some use other algorithm, and some don't use validation at all. These would be wrongly rejected with the current implementation, but researching and gathering all that data was more than what I'm willing to do for a review.
  • Diners Club US & Canada and Mastercard seem to have overlapping prefixes. Figure out how to differentiate between these (if possible) and write tests accordingly.
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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