I wrote a Python code that is validating Credit Card Numbers. Some of the advanced programmers told me my whole code is messy. How can I make my code cleaner?
#!/usr/bin/python
def CheckNumber(CreditCardNumber) :
#------------------------------------------------------------------#
print("\nYour Credit Card Number is : " , CreditCardNumber , "\n")
#------------------------------------------------------------------#
CreditCardNumber = list(map(int, CreditCardNumber))
x = len(CreditCardNumber) - 2
y = len(CreditCardNumber) - 1
z = 0
WorkingList = []
#------------------------------------------------------------------#
while x >= 0 :
WorkingList.append(CreditCardNumber[x] * 2)
x -= 2
#------------------------------------------------------------------#
while y >= 0 :
WorkingList.append(CreditCardNumber[y])
y -= 2
#------------------------------------------------------------------#
WorkingListStr = list(map(str , WorkingList))
WorkingListStr = "".join(WorkingListStr)
del WorkingList[:]
while z < len(WorkingListStr) :
WorkingList.append(WorkingListStr[z])
z += 1
WorkingList = list(map(int , WorkingList))
#------------------------------------------------------------------#
Mod10 = sum(WorkingList) % 10
#------------------------------------------------------------------#
if Mod10 == 0 :
return True
else :
return False
#---------------------------------------------------------------------------#
CardNumber = input("Credit Card Number : ")
CheckNumberStr = str(CardNumber)
#---------------------------------------------------------------------------#
First4 = CheckNumberStr[0] + CheckNumberStr[1] + CheckNumberStr[2] + CheckNumberStr[3]
First2 = CheckNumberStr[0] + CheckNumberStr[1]
#---------------------------------------------------------------------------#
if len(CheckNumberStr) > 12 and len(CheckNumberStr) < 17 :
# "Visa Card" Checking
if CheckNumberStr[0] == "4" :
CheckBool = CheckNumber(CheckNumberStr)
if CheckBool == True :
print("This is a Valid \" Visa \" Credit Card !")
else :
print("This is not a Valid \" Visa \" Credit Card")
# "MasterCard" Card Checking
if CheckNumberStr[0] == "5" and CheckNumber[1] < "6" and CheckNumber[1] > "0" :
CheckBool = CheckNumber(CheckNumberStr)
if CheckBool == True :
print("This is a Valid \" MasterCard \" Credit Card")
else :
print("This is not a Valid \" MasterCard \" Credit Card")
# "Diners Club Card" Checking
if CheckNumberStr[0] == 3 and CheckNumberStr[1] == 6 or CheckNumberStr[0] == 3 and CheckNumberStr[1] == 8 :
CheckBool = CheckNumber(CheckNumberStr)
if CheckBool == True :
print("This is a Valid \" Diners Club \" Credit Card")
else :
print("This is not a Valid \" Diners Club \" Credit Card")
# "Discover" Card Checking
if First4== "6011" or First2 == "65" :
CheckBool = CheckNumber(CheckNumberStr)
if CheckBool == True :
print("This is a Valid \" Discover \" Credit Card")
else :
print("This is not a Valid \" Discover \" Credit Card")
# "JCB" Card Checking
if First2 == "35" :
CheckBool = CheckNumber(CheckNumberStr)
if CheckBool == True :
print("This is a Valid \" JCB \" Credit Card")
else :
print("This is not a Valid \" JCB \" Credit Card")
# "American Express" Card Checking
if First2 == "34" or First2 == "37" :
CheckBool = CheckNumber(CheckNumberStr)
if CheckBool == True :
print("This is a Valid \" American Express \" Credit Card")
else :
print("This is not a Valid \" American Express \" Credit Card")
#----------------------------------------------------------------------------#
else :
print("Sorry , but this is not a Credit Card Number !")
#----------------------------------------------------------------------------#
1 Answer 1
You are having a lot of code duplication in the second part, for all the different card vendors. Use the different if
blocks just to determine the vendor and store it in a variable, and then do the checking and printing afterwards.
number_str = input("Credit Card Number: ") # already str in Python 3
if 12 < len(number_str) < 17 :
first2 = number_str[0:2]
first4 = number_str[0:4]
vendor = None
if number_str[0] == "4" :
vendor = "Visa"
if number_str[0] == "5" and "0" < number_str[1] < "6":
vendor = "MasterCard"
if first2 in ("36", "38"):
vendor = "Diners Club"
if first4 == "6011" or first2 == "65":
vendor = "Discover"
if first2 == "35":
vendor = "JCB"
if first2 in ("34", "37"):
vendor = "American Express"
if vendor is not None:
if check_number(number_str):
print('This is a Valid "%s" Credit Card!' % vendor)
else :
print('This is not a Valid "%s" Credit Card' % vendor)
else:
print('Unknown vendor')
else:
print("Sorry, but this is not a Credit Card Number!")
Minor points:
- use proper Python naming conventions (no upper-case variables, no camel case) so IDEs can highlight them properly
- don't compare
bool
variables toTrue
orFalse
- use comparison chaining,
in
, and slicing instead ofand
- you were comparing
int
withstr
in a few places
Alternatively, instead of all those if
conditions, you could use a dictionary mapping regular expressions to card vendors. Example:
>>> vendors = {"4": "Visa", "5[1-5]": "Mastercard", "36|38": "Diners Club",
... "6011|65": "Discover", "35": "JCB", "34|37": "American Express"}
>>> number_str = "6011343887"
>>> next(v for k, v in vendors.items() if re.match(k, number_str))
'Discover'
This way, the entire block of code becomes this:
vendors = {"4": "Visa",
"5[0-6]": "Mastercard",
"36|38": "Diners Club",
"6011|65": "Discover",
"35": "JCB",
"34|37": "American Express"}
number_str = input("Credit Card Number : ")
if 12 < len(number_str) < 17 :
try:
vendor = next(v for k, v in vendors.items() if re.match(k, number_str))
if check_number(number_str):
print('This is a Valid "%s" Credit Card!' % vendor)
else :
print('This is not a Valid "%s" Credit Card' % vendor)
except StopIteration:
print('Unknown vendor')
else:
print("Sorry, but this is not a Credit Card Number !")
For check_digits
(aka CheckDigits
) I'd suggest using for
loops instead of while
. Also, the entire z
loop seems unneccessary, as it only converts from int
to str
and back to int
to separate the digits. The same can be done using map
. Also, you should move the print
from inside the check_digits
function to the main script.
def check_number(number) :
number = list(map(int, number))
digits = []
for x in range(len(number) - 2, -1, -2):
digits.append(number[x] * 2)
for y in range(len(number) - 1, -1, -2):
digits.append(number[y])
digits = list(map(int, ''.join(map(str, digits))))
return sum(digits) % 10 == 0
Explore related questions
See similar questions with these tags.