6
\$\begingroup\$

Relevant Background:

Greetings. This is my first attempt at using the OOP paradigm (only really used R prior to this).

The project prompt came from JetBrains Academy where I am currently working through the Python Developer track. The code works as expected and was accepted for the particular stage I am on, but I have some specific questions that will hopefully improve the program.

Objective:

You should allow customers to create a new account in our banking system.

Program Details:

Once the program starts, you should print the menu:

1. Create an account
2. Log into account
0. Exit

In our banking system, the credit card number's IIN (first 6 digits) must be 400000.

We often see 16-digit credit card numbers today, but it’s possible to issue a card with up to 19 digits using the current system. In the future, we may see longer numbers becoming more common. In our banking system, the customer account number can be any, but it should be unique. And the whole card number should be 16-digit length.

If the customer chooses ‘Create an account’, you should generate a new card number which satisfies all the conditions described above. Then you should generate a PIN code that belongs to the generated card number. A PIN code is a sequence of any 4 digits. PIN should be generated in a range from 0000 to 9999.

If the customer chooses ‘Log into account’, you should ask them to enter their card information. Your program should store all generated data until it is terminated so that a user is able to log into any of the created accounts by a card number and its pin. You can use an array to store the information.

After all information is entered correctly, you should allow the user to check the account balance; right after creating the account, the balance should be 0. It should also be possible to log out of the account and exit the program.

Specific Questions:

  • I'd like to know how to have the code chunk below print just Bye! and not Bye! and then None below it:

     elif account_balance_selection == "0":
         print("\n")
         print("Bye!")
    
     > Bye!
     > None
    
     should be:
    
     > Bye!
    
  • I am using the following code to generate and store the credit card number and pin number.

      def create_account(self):
          credit_card_number = "400000" + format(randint(0000000000, 9999999999), '010d')
          pin_number = format(randint(0000, 9999), '04d')
    
          self.card_numbers.append(credit_card_number)
          self.pin_numbers.append(pin_number)
    

Is there a way to ensure that the numbers that are getting added to card_numbers and pin_numbers are unique? Either during the generation stage, or somehow prior to appending them to the their respective arrays?

Code:

    from random import randint
    
    class BankingSystem:
        def __init__(self):
            self.card_numbers = []
            self.pin_numbers = []
            self.iin = 400000
    
        def main_welcom_screen(self):
                print(
                    "1. Create an account\n"
                    "2. Log into account\n"
                    "0. Exit"
                )
                main_menu_selection = str(input())
                if main_menu_selection == "1":
                    self.create_account()
                if main_menu_selection == "2":
                    self.account_login()
                if main_menu_selection == "0":
                    print("\n")
                    return "Bye!"
    
        def create_account(self):
            credit_card_number = "400000" + format(randint(0000000000, 9999999999), '010d')
            pin_number = format(randint(0000, 9999), '04d')
    
            self.card_numbers.append(credit_card_number)
            self.pin_numbers.append(pin_number)
    
            print("\n")
            print("Your card has been created")
            print("Your card number:")
            print(credit_card_number)
            print("Your pin number:")
            print(pin_number)
            print("\n")
            self.main_welcom_screen()
    
        def account_login(self):
            print("\n")
            print("Enter your card number:")
            entered_card_number = int(input())
    
            print("Enter your PIN:")
            entered_pin_number = int(input())
    
            if str(entered_card_number) in self.card_numbers and str(entered_pin_number) in 
    self.pin_numbers:
                print("\n")
                print("You have successfully logged in!")
                self.account_balance()
            else:
                print("\n")
                print("Wrong card number of PIN!")
                print("\n")
                self.main_welcom_screen()
    
        def account_balance(self):
            print(
                "\n1. Balance\n"
                "2. Log out\n"
                "0. Exit"
            )
            account_balance_selection = str(input())
    
            if account_balance_selection == "1":
                print("\n")
                print("Balance: 0")
                self.account_balance()
    
            elif account_balance_selection == "2":
                print("\n")
                print("You have successfully logged out!")
                print("\n")
                self.main_welcom_screen()
    
            elif account_balance_selection == "0":
                print("\n")
                print("Bye!")
    
    print(BankingSystem().main_welcom_screen())
\$\endgroup\$

2 Answers 2

10
\$\begingroup\$

As is somewhat common for beginner OOP code, it's not particularly OOP; it's an awkward wrapper around a collection of methods that are still procedural. Consider attempting the following:

  • Make an Account class
  • Store the accounts as a dictionary of Account objects by card number, not separated arrays of cards and PINs
  • Saying "PIN number" is redundant ("personal identification number number")
  • iin is never used; delete it
  • Make a convenience function to show a text menu and call an associated function reference
  • str(input()) is redundant; input already returns a string
  • Call input with a prompt string, not a blank
  • It's welcome, not welcom
  • Use f-strings for your digit formatting code
  • Do not use randint; use randrange since it's more natural to express an exclusive maximum here
  • Do not use randint for the PIN; use the secrets module for cryptographic strength
  • You were printing None because of your print(BankingSystem().main_welcom_screen()), which is meaningless because that function does not return anything
  • You have a critical bug where any combination of existing PIN and card number will allow login; but this should be a match to a specific card-PIN pair
  • You have another bug where the calls out of main_welcom_screen recurse back to main_welcom_screen, so eventually you will blow your stack. Don't recurse here.
  • Consider adding validation for your menu choices
  • Consider adding an anti-bruteforce hang after invalid PIN input
  • Do NOT acquire a user's PIN via regular input, which exposes the PIN to over-the-shoulder; use getpass instead

Example implementation

from dataclasses import dataclass
from getpass import getpass
from random import randrange
from secrets import randbelow
from time import sleep
from typing import Dict, Tuple, Callable, ClassVar


class Menu:
    MENU: ClassVar[Tuple[
        Tuple[
            str, Callable[['Menu'], bool]
        ], ...
    ]]

    def screen(self):
        prompt = '\n'.join(
            f'{i}. {name}'
            for i, (name, fun) in enumerate(self.MENU)
        ) + '\n'

        while True:
            choice = input(prompt)

            try:
                name, fun = self.MENU[int(choice)]
            except ValueError:
                print('Invalid integer entered')
            except IndexError:
                print('Choice out of range')
            else:
                if fun(self):
                    break


@dataclass
class Account(Menu):
    card: str
    pin: str

    @classmethod
    def generate(cls) -> 'Account':
        return cls(
            card=f'400000{randrange(1e10):010}',
            pin=f'{randbelow(10_000):04}',
        )

    def dump(self):
        print(
            f"Your card number: {self.card}\n"
            f"Your PIN: {self.pin}"
        )

    def balance(self):
        print('Balance: 0')

    def logout(self) -> bool:
        print('You have successfully logged out!')
        return True

    def exit(self):
        print('Bye!')
        exit()

    MENU = (
        ('Exit', exit),
        ('Balance', balance),
        ('Log out', logout),
    )


class BankingSystem(Menu):
    def __init__(self):
        self.accounts: Dict[str, Account] = {}

    def create_account(self):
        account = Account.generate()
        print('Your card has been created')
        account.dump()
        self.accounts[account.card] = account

    def log_in(self):
        for _ in range(3):
            card = input('Enter your card number: ')
            pin = getpass('Enter your PIN: ')

            account = self.accounts.get(card)
            if account is None or account.pin != pin:
                print('Wrong card or PIN')
                sleep(2)
            else:
                print('You have successfully logged in!')
                account.screen()
                break

    def exit(self) -> bool:
        print('Bye!')
        return True

    MENU = (
        ('Exit', exit),
        ('Create an account', create_account),
        ('Log into an account', log_in),
    )


BankingSystem().screen()
\$\endgroup\$
7
  • \$\begingroup\$ Thank for this. I am looking forward to reviewing this in more detail. A lot to learn! \$\endgroup\$
    – Eric
    Feb 12, 2021 at 23:50
  • 1
    \$\begingroup\$ But I always use my PIN number in the ATM machine.... \$\endgroup\$
    – James K
    Feb 13, 2021 at 13:44
  • \$\begingroup\$ Your Account class is missing a balance and the card numbers are not unique. Your interactive menu system is great, but it distracts from the actual exercise: write an Account class. Mixing data with the menu system is also not a good idea ("dataclass Account extends Menu"?) \$\endgroup\$
    – danzel
    Feb 13, 2021 at 20:35
  • \$\begingroup\$ @danzel The missing balance is deliberate: it's the exact same as what the OP posted. The same goes for uniqueness: I've deliberately left out the machinery that would be required to generate unique card numbers. As for mixing data with the menu, I agree; but - baby steps. \$\endgroup\$
    – Reinderien
    Feb 13, 2021 at 20:49
  • \$\begingroup\$ @Reinderien generating the card number inside the Account class is a design flaw since neither the Account class nor its instances should know of any other card number. It's up to the BankingSystem to choose a (new and unique) card number. \$\endgroup\$
    – danzel
    Feb 13, 2021 at 21:06
7
\$\begingroup\$

Is there a way to ensure that the numbers that are getting added to card_numbers and pin_numbers are unique? Either during the generation stage, or somehow prior to appending them to the their respective arrays?

AFAIK the card number does not have to be random, just unique. So I would generate the numbers in a sequential manner, and store them to a database with a unique constraint.

In SQL you could perform a SELECT MAX statement to retrieve the largest card number and increment the range, but watch out for race conditions (use database locks).

For this, you need some persistence. For a self-contained system (vs distributed systems used in the real world) using a small SQLite database would be a good option.

Generating a random card number like what you're doing offers no guarantee that it will be unique, especially since you don't check the number against the list of previously generated numbers. A collision is extremely unlikely, since the range is very large but the code is nonetheless flawed in this aspect.

Say that we stick to your approach but store the card number as integer rather than string. Then you can retrieve the maximum number with the max function:

max(self.card_numbers)

and increment it by 1 or 10 or whatever.

Could be something like:

def generate_card_number():
    # list is empty, first number shall be 4000000000000000
    if len(self.card_numbers) == 0:
        credit_card_number = 4000000000000000
    else:
        # increment
        credit_card_number = max(self.card_numbers) +1
    
    self.card_numbers.append(credit_card_number)
    # or return a value
    return credit_card_number

As for the PIN number what you are doing seems reasonable. It has to be random but not unique, since it's perfectly conceivable more than one customer will share the same PIN, which is definitely unavoidable after a few thousand customers.

Note that in the real world the last digit in a credit card number is a check-digit generated according to the Luhn algorithm. So the generation rules are a bit more complicated actually.

\$\endgroup\$
2
  • \$\begingroup\$ Thank you very much for this perspective and insight. The next step in the project is to implement the Luhn algorithm! \$\endgroup\$
    – Eric
    Feb 14, 2021 at 21:34
  • \$\begingroup\$ Note that in the real world, some attacks (including social engineering ones) are easier if the numbers can be predicted. Like TCP sequence numbers, they may benefit from hashing in some secret data to make the numbers unguessable. \$\endgroup\$ Aug 16, 2022 at 16:56

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.