4
\$\begingroup\$

I am practicing test-driven development in Python. Feedback about both the tests and the code is appreciated. I'm using luhn algorithm and regex to check credit card numbers for validity.

Here are the tests:

from unittest import TestCase import luhn_check class TestLuhnCheck(TestCase): def setUp(self): pass def test_known_sum(self): self.assertEquals(67, luhn_check.check_sum("7992739871")) def test_check_digit(self): self.assertEquals(3, luhn_check.find_check_digit("7992739871")) def test_check_valid(self): self.assertFalse(luhn_check.is_luhn_valid("abcdefg")) self.assertTrue(luhn_check.is_luhn_valid("79927398713")) def test_check_invalid(self): self.assertFalse(luhn_check.is_luhn_valid("79927398714")) def test_regex(self): self.assertTrue(luhn_check.check_regex("346340033233241")) self.assertFalse(luhn_check.check_regex("996340033233241")) def test_check_cc(self): #amex self.assertTrue(luhn_check.is_valid_cc("346340033233241")) #discover self.assertTrue(luhn_check.is_valid_cc("6011066253550425")) #visa self.assertTrue(luhn_check.is_valid_cc("4485332611430235")) #mc self.assertTrue(luhn_check.is_valid_cc("5463393720504875")) #bad: self.assertFalse(luhn_check.is_valid_cc("abcdefg")) self.assertFalse(luhn_check.is_valid_cc("11abcdefg")) self.assertFalse(luhn_check.is_valid_cc("946340033233241")) self.assertFalse(luhn_check.is_valid_cc("546339372050487500"))

and here is the module:

import re def sum_digits(numeric_string): return sum([int(d) for d in numeric_string]) def valid_length_cc(cc_candidate): return len(cc_candidate) == 16 or len(cc_candidate) == 15 def check_sum(numeric_string): odds = numeric_string[:-1][::-2] evens = numeric_string[::-2] return sum([int(i) for i in odds]) + sum([sum_digits(str(int(i) * 2)) for i in evens]) def find_check_digit(numeric_string): return (check_sum(numeric_string) * 9) % 10 def is_luhn_valid(candidate): if candidate.isdigit(): check_digit = int(candidate[-1:]) return (check_sum(candidate[:-1]) + check_digit) % 10 == 0 else: return False def is_valid_cc(candidate): return check_regex(candidate) and valid_length_cc(candidate) and is_luhn_valid(candidate) def check_regex(candidate): # are the digits correct? pattern = "^((4\d{3})|(5[1-5]\d{2})|(6011)|(7\d{3}))-?\d{4}-?\d{4}-?\d{4}|3[4,7]\d{13}$" r = re.compile(pattern) return bool(r.match(candidate))
\$\endgroup\$
1
  • \$\begingroup\$ +1, good question. Just like to comment that I think this is a good time to use unit tests like this. \$\endgroup\$ Commented Dec 25, 2014 at 1:11

2 Answers 2

Reset to default
6
\$\begingroup\$

Deprecation warning

This code works in both Python 2 and 3. However, in Python 3, unittest.assertEquals() is deprecated in favour of .assertEqual(), so use .assertEqual() for better compatibility.

Length check

valid_length_cc would be better written as return 15 <= len(cc_candidate) <= 16. It should also be moved further down, near the definition of is_valid_cc, since it has nothing to do with the Luhn algorithm code.

However, I recommend dispensing with the function altogether. The expected length varies according to the card type. You could just use the regex check to do the length validation for you.

Regex check

Your regex allows optional hyphens, but nowhere else in the code do you handle hyphens gracefully.

You incorrectly allow the second character of a card number that starts with 3 to be a comma.

Support for Discover cards appears to be incomplete.

The leading ^ anchor is redundant if you use re.match().

As previously mentioned, the regex should vary completely according to the card type. I would therefore write it this way for clarity:

def check_regex(candidate): PATTERN = re.compile(r'(4(?:\d{12}|\d{15})' # Visa r'|5[1-5]\d{14}' # Mastercard r'|6011\d{12}' # Discover (incomplete) r'|7\d{15}' # Petroleum and misc. r'|3[47]\d{13}' # American Express r')$') return bool(PATTERN.match(candidate))

Luhn algorithm check

In my opinion, is_luhn_valid() would be better named check_luhn(), to be consistent with check_regex().

find_check_digit() would be better named compute_check_digit(). You'll never "find" the check digit by looking for it. I'm not sure that it's a useful function to have at all, though, as is_luhn_valid() should be good enough.

Rather than slicing the string in is_luhn_valid(), I recommend feeding it intact to check_sum(), since it's really part of the same checksumming kind of operation. For compatibility with your existing code, I've introduced an includes_check_digit mode to check_sum() that defaults to False.

For simplicity, the sum_digits() function could be replaced by a lookup table.

When calling sum(), you only need a generator expression, not a list comprehension. You can therefore write sum(… for …) instead of sum([… for …]).

Suggested implementation

import re def check_sum(numeric_string, includes_check_digit=False): if not includes_check_digit: numeric_string += '0' EVEN_VALUES = [0, 2, 4, 6, 8, 1, 3, 5, 7, 9] evens = numeric_string[:-1][::-2] odds = numeric_string[::-2] return sum(int(i) for i in odds) + sum(EVEN_VALUES[int(i)] for i in evens) def find_check_digit(numeric_string): return 9 * check_sum(numeric_string) % 10 def check_luhn(candidate): return candidate.isdigit() and check_sum(candidate, True) % 10 == 0 def check_regex(candidate): PATTERN = re.compile(r'(4(?:\d{12}|\d{15})' # Visa r'|5[1-5]\d{14}' # Mastercard r'|6011\d{12}' # Discover (incomplete?) r'|7\d{15}' # What's this? r'|3[47]\d{13}' # American Express r')$') return bool(PATTERN.match(candidate)) def is_valid_cc(candidate): return check_regex(candidate) and check_luhn(candidate)
\$\endgroup\$
2
\$\begingroup\$

These are on top of 200_success' suggestions because I agree with them and there's no point me repeating them.

You don't need to write setUp in the tests if it does nothing, so remove that.

I find this very confusing:

odds = numeric_string[:-1][::-2] evens = numeric_string[::-2]

Better is to reverse the string 

numeric_string = numeric_string[::-1]

and do obvious slices:

odds = numeric_string[1::2] evens = numeric_string[::2]

I would also convert to integers at that point, not in the return:

numeric_string = [int(i) for i in numeric_string[::-1]] odds = numeric_string[1::2] evens = numeric_string[::2] return sum(odds) + sum(EVEN_VALUES[i] for i in evens)

FWIW, I think

sum(sum_digits(2 * i) for i in evens)

is clearer than the cached version. sum_digits should in turn be

def sum_digits(number): return sum(map(int, str(number)))

I would change check_sum to checksum to make it clear that it returns a value and not a boolean.

I would use re.VERBOSE to improve the formatting of the regex. Maybe:

def check_regex(candidate): # are the digits correct? PATTERN = re.compile(r""" ( 4(\d{3})? \d{12} | # Visa 5[1-5] \d{14} | # Mastercard 6011 \d{12} | # Discover (incomplete) 7 \d{15} | # Petroleum and misc. 3[47] \d{13} # American Express )$ """, re.VERBOSE) return bool(PATTERN.match(candidate))
\$\endgroup\$
0

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.