5
\$\begingroup\$

I'm a newbie to python object oriented programming, Unfortunately in all the ways, Classes that I write turn into a true mess.

I'd be glad if someone would be able to point me some of my mistakes.

#!/usr/bin/env python3
# -*- coding: utf-8 -*-


def main():

    from decimal import Decimal
    from typing import List


    class BankDespositLimitError(Exception):
        def __init__(self, traceback: str, amount: Decimal, bank_limit: Decimal):
            """Bank does not support the current desposited amount Error

            # Args:
                - traceback (str): The traceback message.
                - amount (Decimal): The amount tried to be desposited
                - bank_limit (Decimal): The top amount the bank support for desposit.
            """

            self.amount = ''.join([character for character in str(amount) if character.isdigit()])
            self.bank_limit = bank_limit
            self.traceback = f'{traceback} \nGiven amount : {self.amount} | Top supported amount : {self.bank_limit}'
            super().__init__(self.traceback)


    class InvalidCurrencyError(Exception):
        def __init__(
            self, traceback: str, inputted_currency: str, avaliable_currencies: List[str]
        ):
            """
            Args:
                traceback (str): The desired error message
                inputted_currency (str): The currency that the user have entered.
                avaliable_currencies (List[str]): The bank avaliable currencies.
            """

            __ac_isstr = list(
                map(lambda element: isinstance(element, str), avaliable_currencies)
            )

            if not all(__ac_isstr):
                raise ValueError("Avaliable currencies must be strings.")

            if not isinstance(inputted_currency, str):
                raise ValueError("inputted currency must be string.")

            if not isinstance(traceback, str):
                raise ValueError("The traceback must be a string.")

            self.inputted_currency = inputted_currency.lower()
            self.avaliable_currencies = [currency.lower() for currency in avaliable_currencies]
            self.traceback = (
                f"{traceback}| \n"
                f"inputted currency : {self.inputted_currency} \n"
                f"avaliable currencies : {', '.join(self.avaliable_currencies)}"
            )

            super().__init__(self.traceback)


    class Account:
        def __init__(
            self, first_name: str, last_name: str, balance: Decimal, currency: str
        ) -> None:
            """`Initialize a bank account ( Simple form )`

            # Args:
                - first_name (`str`): The account owner first name.
                - last_name (`str`): The account owner last name.
                - balance (`Decimal`): the account owner balance.
                - currency (`str`): The account currency (`dollar`, `toman`)
            """

            __bank_limit = Decimal(1000)
            __supported_currencies = ["toman", "dollar"]
            currency = currency.lower()
            first_name = first_name.lower()
            last_name = last_name.lower()

            if not isinstance(first_name, str):
                raise ValueError(
                    f"Expected first_name argument type (str), not ({type(first_name).__name__})"
                )

            if not isinstance(last_name, str):
                raise ValueError(
                    f"Expected last_name argument type (str), not ({type(last_name).__name__})"
                )

            if not isinstance(balance, Decimal):
                raise ValueError(
                    f"Expected balance argument type (Decimal), not ({type(balance).__name__})"
                )

            if not isinstance(currency, str):
                raise ValueError(
                    f"Expected currency argument type (str), not ({type(currency).__name__})"
                )

            if not currency in __supported_currencies:
                raise InvalidCurrencyError(
                    "That currency is not supported by this bank at the moment.",
                    currency,
                    __supported_currencies,
                )

            if balance < 0:
                raise ValueError("The account balance cannot be a negative number.")

            if balance > __bank_limit:
                raise BankDespositLimitError(
                    "The bank not support this amount of first desposit at the moment.",
                    balance,
                    __bank_limit,
                )
            
            self.first_name = first_name
            self.last_name = last_name
            self.full_name = f"{self.first_name} {self.last_name}"
            self.balance = balance
            self.currency = currency    


if __name__ == '__main__':
    main()

\$\endgroup\$
3
  • \$\begingroup\$ The if __name__ main() guard is very nice, I thank you kindly. But import and class definitions within def main(): is just bizarre. Please revise the question so we see imports at top-of-module, and class definitions also at top-level. That will let reviewers focus on the substance of what you have to say, rather than odd details of how you say it. \$\endgroup\$
    – J_H
    Commented Sep 12, 2023 at 0:47
  • 3
    \$\begingroup\$ @J_H If OP learned Python by putting imports and classes inside their def main, that is definitely something that needs to be addressed in an answer, preferably with rationale. Not something to be treated as "please format your question better". \$\endgroup\$ Commented Sep 12, 2023 at 8:07
  • 3
    \$\begingroup\$ The irony is that - despite there being so much code in main() - nothing is ever run. There are no tests or demos that exercise the code. Do you have something like this that you can include? \$\endgroup\$
    – Reinderien
    Commented Sep 12, 2023 at 12:03

1 Answer 1

4
\$\begingroup\$

The good

Docstrings!

Custom exception types with contextual parameters!

Validation!

Decimal for currency!

The bad

It's deposit, not desposit (the bank, despite our occasional grievances, is not a despot).

It's available, not avaliable.

It's not terrible to use List but it's outdated. Update Python and then just use list.

What you're calling a traceback is extremely not a traceback; it's just an error message.

The gymnastics around self.amount = ''.join should go away; just store the member directly.

Python is a duck-typed language (to our collective detriment). That means that "if it walks like a duck and quacks like a duck, it is a duck". It's therefore less important (and usually futile) to do run-time type checking. More important is run-time value checking like your negative and limit checks; and static analysis of types using Mypy.

It doesn't make sense to do any of these checks:

            __ac_isstr = list(
                map(lambda element: isinstance(element, str), avaliable_currencies)
            )

            if not all(__ac_isstr):
                raise ValueError("Avaliable currencies must be strings.")

because in theory, the available list of currencies is enforced by the application or bank, not provided by the user - so of course they'll all be valid. If you were to instantiate a Bank class this might change.

The format of your docstrings is one I don't recognize. Prefer something standard, either RestructuredText (default from PyCharm), Doxygen, etc.

Don't repeat the parameter type in your docstring; keep it to your typehint only.

Add tests.

Consider representing full_name as a @property.

Move your bank limit and supported currency variables to statics on the class.

Don't lowercase your first and last names.

The ugly

Don't wrap your entire program in main() - main() should only have entry-point code. In my demonstration I show it calling into a test. De-tab everything else.

Variables like __ac_isstr should not be double-underscored - that's reserved for a different meaning.

Suggested

#!/usr/bin/env python3

from decimal import Decimal


class DepositLimitError(Exception):
    def __init__(self, deposit: Decimal, bank_limit: Decimal) -> None:
        self.deposit = deposit
        self.bank_limit = bank_limit
        super().__init__(f'Deposit attempted: {self.deposit} | Deposit limit: {self.bank_limit}')


class InvalidCurrencyError(Exception):
    def __init__(self, input_currency: str) -> None:
        self.input_currency = input_currency
        super().__init__(f'Currency {input_currency} is not supported.')


class Account:
    DEPOSIT_LIMIT = Decimal(1_000)
    SUPPORTED_CURRENCIES = {'toman', 'dollar'}

    def __init__(
        self, first_name: str, last_name: str, balance: Decimal = 0, currency: str = 'dollar',
    ) -> None:
        if not first_name:
            raise ValueError('First name must be a non-empty string')
        self.first_name = first_name

        if not last_name:
            raise ValueError('Last name must be a non-empty string')
        self.last_name = last_name

        if currency not in self.SUPPORTED_CURRENCIES:
            raise InvalidCurrencyError(currency)
        self.currency = currency

        if balance < 0:
            raise ValueError('The account balance must be non-negative.')
        if balance > self.DEPOSIT_LIMIT:
            raise DepositLimitError(balance, self.DEPOSIT_LIMIT)
        self.balance = balance

    @property
    def full_name(self) -> str:
        return f'{self.first_name} {self.last_name}'


def test() -> None:
    try:
        Account('', 'Bill')
        raise AssertionError()
    except ValueError:
        pass

    try:
        Account('Joe', '')
        raise AssertionError()
    except ValueError:
        pass

    try:
        Account('Joe', 'Bill', -3)
        raise AssertionError()
    except ValueError:
        pass

    try:
        Account('Joe', 'Bill', 10_000)
        raise AssertionError()
    except DepositLimitError:
        pass

    try:
        Account('Joe', 'Bill', Decimal(500), 'lira')
        raise AssertionError()
    except InvalidCurrencyError:
        pass

    account = Account('Joe', 'Bill', Decimal(600), 'dollar')
    assert account.full_name == 'Joe Bill'


if __name__ == '__main__':
    test()
\$\endgroup\$
4
  • \$\begingroup\$ Yay, unit tests! Those are valuable, very nice. Some folks like -m unittest, some prefer pytest or other runners. Me? I like 'em both, so I tend to inherit from TestCase, giving the flexibility to choose a runner weeks later according to need. (I find $ pytest --cov a very convenient way to measure code coverage.) In these tests, I feel that with self.assertRaises(DepositLimitError): offers slightly clearer exposition. Plus, you can't accidentally omit raise AssertionError() and admit "silent success". \$\endgroup\$
    – J_H
    Commented Sep 12, 2023 at 15:45
  • \$\begingroup\$ Or import pytest: stackoverflow.com/a/29855337/… \$\endgroup\$
    – J_H
    Commented Sep 12, 2023 at 15:48
  • 1
    \$\begingroup\$ Sure? But I tried to keep it simple since OP self-describes as a newbie. \$\endgroup\$
    – Reinderien
    Commented Sep 13, 2023 at 0:14
  • \$\begingroup\$ Greetings, Thanks for your answer sir it was pretty much helpful to me as a beginner, Just in case you asked, the docstring format is the Google docstring format, if we despite the small changes I've inserted to them. \$\endgroup\$
    – KhodeNima
    Commented Sep 13, 2023 at 9:15

Your Answer

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

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