7
\$\begingroup\$

Just completed a piece of code - a credit card validation program in python - as a little side project. Having no experience with classes in the past, I decided to employ classes in this project.

I am wondering if you could review my code, both appraising the actual code, but also evaluate my use of OOP/classes.

The requirements for the program are:

The user enters their name, postcode, the card code, and the card date.

The eighth digit of the card code is removed and acts as a check digit

The code is then reversed

The 1st, 3rd, 5th, and 7th digits are multiplied by 2

If the result of the multiplication is> 9, subtract 9 from it.

If the sum of the 7 digits, and the check digit are divisable by 10, the code is valid

The card date must also be in the future.

Finally, output their name, postcode, card number, and whether it is valid or not.


The code:

"""Program used to check if a credit card is authentic."""
# !/usr/bin/env python
# -*- coding: utf-8 -*-
# Check it out
import datetime
class Customer:
 """Class representing the customer and their credit card details"""
 # Constructor
 def __init__(self):
 self.name = input("Name: ")
 self.postcode = input("Postcode: ")
 self.card_date = input("Card date: ")
 self.card_code = input("Card code: ").strip()
 def check_date(self):
 """Checks current date against the credit card's date. If it is valid, returns True; else False."""
 card = datetime.datetime.strptime(self.card_date, "%d/%m/%Y").date()
 if datetime.date.today() < card:
 return True
 else:
 return False
 def check_code(self):
 """Contains the algorithm to check if the card code is authentic"""
 code_list = list(str(self.card_code))
 check_digit = int(code_list[7])
 code_list.pop()
 # The last digit is assigned to be a check digit and is removed from the list.
 code_list.reverse()
 for item in code_list:
 temp_location = code_list.index(item)
 if is_even(temp_location):
 code_list[temp_location] = int(item) * 2
 # Loops over each digit, if it is even, multiplies the digit by 2.
 for item in code_list:
 temp_location = code_list.index(item)
 if int(item) > 9:
 code_list[temp_location] = int(item) - 9
 # For each digit, if it is greater than 9; 9 is subtracted from it.
 sum_list = 0
 for item in code_list:
 sum_list += int(item)
 # Calculates the sum of the digits
 code_total = sum_list + int(check_digit)
 if code_total % 10 == 0:
 return True
 else:
 return False
 # If the code is divisible by 10, returns True, else, it returns False.
 def check_auth(self):
 """Checks the card's authenticity. """
 if self.check_code() and self.check_date():
 print("----------------------")
 print("Valid")
 print(self.name)
 print(self.postcode)
 print(self.card_date)
 print(self.card_code)
 else:
 print("----------------------")
 print("Invalid")
 print(self.name)
 print(self.postcode)
def is_even(number):
 """Function used to test if a number is even."""
 if number % 2 == 0:
 return True
 else:
 return False
if __name__ == "__main__":
 customer().check_auth()
asked Apr 18, 2017 at 9:03
\$\endgroup\$
1
  • 1
    \$\begingroup\$ "%d/%m/%Y" most card I know only care about the month and year \$\endgroup\$ Commented Apr 18, 2017 at 14:18

1 Answer 1

8
\$\begingroup\$

I think there is this code organization issue - you have a class named Customer, but it, aside from the .name attribute, consists of credit-card related logic only.

I would also pass the obtained attributes to the class constructor instead of asking for them inside it:

def __init__(self, name, post_code, card_date, card_code):
 self.name = name
 self.post_code = post_code
 self.card_date = card_date
 self.card_code = card_code

It is a little bit cleaner to do this way since now our class is more generic, it is agnostic of where the attributes are coming from.

Some other code-style related notes:

  • consistent naming: rename postcode to post_code
  • revise the quality and necessity of comments: there is probably not much sense in having a comment # Constructor
  • you can simplify the way you return a boolean result from your methods. For instance, you can replace:

    if datetime.date.today() < card:
     return True
    else:
     return False
    

    with:

    return datetime.date.today() < card
    

And, it's worth mentioning that, generally speaking, if you doing this for production, you should not be reinventing the wheel and switch to a more mature, well-used and tested package like pycard.

answered Apr 18, 2017 at 9:46
\$\endgroup\$
2
  • 1
    \$\begingroup\$ +1 on the passing attributes to the constructor over reading them in with input. I'd also like to also suggest replacing magic numbers with meaningful constants. Giving a good name to a number can replace some comments. You could also consider a Card class, and create a Customer with a name, postcode and CreditCard object. \$\endgroup\$ Commented Apr 18, 2017 at 12:06
  • 2
    \$\begingroup\$ The spec quoted in the question uses "postcode" as one word, so I would expect the variable to be postcode as one word. \$\endgroup\$ Commented Apr 18, 2017 at 15:53

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.