3
\$\begingroup\$

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 !")
#----------------------------------------------------------------------------#
\$\endgroup\$

migrated from stackoverflow.com Feb 2 '18 at 15:36

This question came from our site for professional and enthusiast programmers.

5
\$\begingroup\$

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 to True or False
  • use comparison chaining, in, and slicing instead of and
  • you were comparing int with str 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
\$\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.