1
\$\begingroup\$

I am doing some projects so I can teach myself programming. The one that I finished states: "Credit Card Validator - Takes in a credit card number from a common credit card vendor and validates it to make sure that it is a valid number (look into how credit cards use a checksum)." Would be glad if i can get a constructive feedback on this one.

import sys

cardnumb_input = input('Enter your credit card number: ')


def list_tweak():
    try:
        cardnumb = [int(i) for i in cardnumb_input]
        cardnumb[0::2] = [x * 2 for x in cardnumb[0::2]]
        cardnumb = [str(i) for i in cardnumb]
        cardnumb = list(''.join(cardnumb))
        cardnumb = [int(i) for i in cardnumb]
        return cardnumb

    except ValueError:
        print('Please enter only number in format: xxxxxxxxxxxxxxxx')
        sys.exit(1)


def valid_or_not():
    total = sum(list_tweak())
    if total % 10 != 0:
        print('It seems that this credit card number is not valid.')
    else:
        print('This credit card number is valid. ')


valid_or_not()
\$\endgroup\$
7
\$\begingroup\$

What should the type signature of these functions be?

I would really separate the IO from data. In addition, I would display data that you already have. I would not have valid_or_not print stuff out. Instead I would have it return a value. But what value?

Furthermore, don't read global variables. valid_or_not and list_tweak should take paramters.

We'll get to printing stuff in the end.

What do you want to do with an invalid credit card number?

You are both throwing an error, and also printing out whether it is a valid card number. I would say there is a couple ways to go, but I prefer this approach:

Return an enum that ends up being either: VALID, INVALID_CHECKSUM, or INVALID_FORMAT. Instead of doing printing.

Rework the flow of valid_or_not.

Consider reworking valid_or_not to do the following:

  1. Check to see if the format is valid, and return INVALID_FORMAT if it is not.
  2. The run list_tweak, but don't do try-except on it, return either INVALID_CHECKSUM or VALID depending on list_tweak.

Naming.

I am not sure what list_tweak means, but I am assuming only because I've seen this problem show up time and time again that it is the Luhn algorithm. Name it something appropriate like luhn_checksum.

IO

So what should we really be doing for input? First we should have a main function (see the link for more information why. Thus we get:

def main():
    cardnumber = input('Enter your credit card number: ')
    print(valid_or_not(cardnumber))


if __name__ == '__main__':
    main()

See all the data is manipulated in valid_or_not which is then displayed, instead of displaying information withing valid_or_not).

\$\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.