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
- It requires a comment that streches over two lines, and even then it is unclear at best.
- 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?
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\$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\$