15
\$\begingroup\$

I am learning Python and in particular learning about classes. Since I'm self teaching, I'd appreciate any feedback and constructive criticism.

I have a couple specific questions:

  1. In Python you can have multiple functions and classes in a single file (module), but is it considered bad practice to do so?
  2. Python doesn't have private, so is there a point to having accessor methods like I do below?
  3. My IDE suggest I put a comment beneath each function signature. Is it still good to do this when the functions are short and simple, like withdraw(x) in a bank account?

BankAccount.py

# bank account class
class BankAccount:
    def __init__(self, name, balance=0.00):
        self.name = name
        self.balance = balance

    def deposit(self, amount):
        """make a deposit"""
        self.balance += amount

    def withdraw(self, amount):
        """make a withdraw"""
        if amount > self.balance:
            raise ValueError("insufficient funds")
        self.balance -= amount

    def get_balance(self): #are accessor methods needed in Python?
        """check the balance"""
        return self.balance


def main():
    customer1 = BankAccount('Alex')
    print(customer1.get_balance())
    customer1.deposit(100)
    customer1.withdraw(30)
    print(customer1.get_balance())

    customer2 = BankAccount('Sam', 200)
    print(customer2.get_balance())

if __name__ == "__main__":
    main()
\$\endgroup\$
  • 2
    \$\begingroup\$ It’s just a tiny note, but I think you can get rid of your bank account class comment, as it doesn’t seem to add any insight to the code class BankAccount. \$\endgroup\$ – Paul D. Waite Apr 15 '17 at 19:32
  • \$\begingroup\$ Comments like ` """make a withdraw"""` are completely pointless. They add no new information, and just add clutter \$\endgroup\$ – Alexander Apr 15 '17 at 22:24
  • \$\begingroup\$ Ok great. I thought it was customary in Python to always have a comment for a function definition. \$\endgroup\$ – northerner Apr 15 '17 at 22:40
16
\$\begingroup\$

Python doesn't have private anything but we still have a need to separate inners of a class from the public interface.

By convention, attributes or methods whose name starts with a single underscore are considered internals of the class and, thus, not part of the public API. There is no mechanism in place to restrict their access, so user that know what they are doing can still use them; but if anything goes wrong, it will most likely be their fault, not yours.

Also, in the context of versionned libraries, internals are not subject to backward compatibility and can change without notice on a new release.

But instead of methods as getter or setters, we prefer properties that allows the same kind of flexibility (read-only vs. read-write) with the simplicity of attributes access.

Appart from that, I would add either __str__ or __repr__ (or both) as methods in your class to simplify printing accounts:

class BankAccount:
    def __init__(self, name, balance=0.00):
        self.name = name
        self._balance = balance

    def deposit(self, amount):
        """make a deposit"""
        self._balance += amount

    def withdraw(self, amount):
        """make a withdraw"""
        if amount > self._balance:
            raise ValueError("insufficient funds")
        self._balance -= amount

    @property
    def balance(self):
        """check the balance"""
        return self._balance

    def __repr__(self):
        return '{0.__class__.__name__}(name={0.name}, balance={0.balance})'.format(self)

    def __str__(self):
        return 'Bank account of {}, current balance: {}'.format(self.name, self.balance)

Example usage being:

customer1 = BankAccount('Alex')
print(repr(customer1))
customer1.deposit(100)
customer1.withdraw(30)
print(customer1)
customer2 = BankAccount('Sam', 200)
print(customer2.balance)

As regard to the "one class per file" rule, it may make sense in languages where a file does not bear much meaning.

But in python a file is a module. It is perfectly fine and expected to put related entities in them as they define a namespace.

Beside, where would you put simple functions and constants using a "one class per file" approach? As class attribute of some sort of "namespace" class? Yuck! Module already serve this purpose, don't reinvent the wheel.

\$\endgroup\$
  • \$\begingroup\$ Adding a property is absolutely non-pythonic, IMO. Data-encapsulation is not required in such a simple use-case. One would neither use setters/getters nor properties. In fact, in such a simple use-case using properties even increases code length[1]. Plenty people agree with this opinion. [2] It's up to OP to decide, but I really wanted to give a second option on this. [1]blaag.haard.se/What-s-the-point-of-properties-in-Python [2]python-course.eu/python3_properties.php \$\endgroup\$ – FMaz Apr 16 '17 at 19:44
  • \$\begingroup\$ @FynnMazurkiewicz I do agree. That's why I didn't "try to encapsulate" the name attribute. But my point was not whether or not to encapsulate, but instead, if "you need to encapsulate, you should do it with properties" ; and both your links agree. \$\endgroup\$ – Mathias Ettinger Apr 16 '17 at 20:09
  • \$\begingroup\$ Absolutely correct. I was specifically talking about the fact that you added a property for the balance. \$\endgroup\$ – FMaz Apr 16 '17 at 20:34
13
\$\begingroup\$

In this answer I'm not going to review your code, but instead discuss the design of a book-keeping system. (I know that you're just using a bank account as an exercise, but an important part of programming is thinking about design like this.)


The code in the post implements account objects with methods for transactions. For example here's a transaction affecting the account customer1:

customer1.deposit(100)

But where did this money come from? Real book-keeping systems are double entry — that is, every transaction affects two accounts. If the money came from the account customer2 then there must be a corresponding withdrawal on that account so that the accounts balance.

But now we have to worry about atomicity of transactions. In order to process the transaction, we have to make two calls:

customer2.withdraw(100)
customer1.deposit(100)

but what if there's a power cut after the withdrawal and before the deposit? This would cause the money to be lost in transit.

For these reasons, the usual way to represent financial data is in the form of a ledger, which is a table of transactions:

FROM       TO         AMOUNT
---------  ---------  ------
customer2  customer1     100
customer1  cash           30

(Typically there would be other fields like the date and the authorizing user, to provide an audit trail.)

If the ledger supports atomic append (that is, trying to append a transaction to the ledger either succeeds or fails as a whole) then money can't be lost.

In a ledger system you would likely still have objects representing accounts, but an account balance would be a cache of the balance you get by applying the transactions in ledger.

\$\endgroup\$
  • 1
    \$\begingroup\$ Thanks for the feedback. When I was google searching other solutions, I noticed some had implemented the class as Customer instead of BankAccount. Which title is better for with respect to design? \$\endgroup\$ – northerner Apr 15 '17 at 20:50
  • \$\begingroup\$ @northerner: One customer might have multiple accounts, and some accounts might not belong to customers (in my example above "cash" is an account). So "account" is right and "customer" is wrong. \$\endgroup\$ – Gareth Rees Apr 15 '17 at 22:09
  • \$\begingroup\$ "Gareth Rees" is quite right in what he says, but then it really depends on which problem you are practically trying to resolve. Your deisgn impacts the implementation also. If the operation (deposit and withdraw money) are not meant to be performed by a customer but rather refer to the state of his account, then Garetg Rees is right: Name it "Account" ... well, "BankAccount" is a better name because "Account" is confusing (Facebook account?). In this case, your self.name = name should not exist as it is and where it is now. Use self.banck_account_number instead. @northerner \$\endgroup\$ – Billal Begueradj Apr 16 '17 at 7:00
  • \$\begingroup\$ This is a great way to extend the current functionality, but I don't think it can replace it. Banks are not the only ways of holding money. If I have cash that I'm depositing, it didn't come from a bank account. It's possible that it was once in a bank, but it wasn't necessarily the same bank even. If two people from the same bank want to transfer money, it's great to be able to do that in one transaction, but I think his current withdraw and deposit code is still an important piece. \$\endgroup\$ – zondo Apr 16 '17 at 10:40
  • \$\begingroup\$ Not sure if it's within scope of the OP's goal, but you may want to mention the Factory pattern for constructing such ledger objects. (Using psudocode) new Transaction().drawFrom(Customer1.checking).depositTo(Customer2.savings).create() \$\endgroup\$ – Tyzoid Apr 16 '17 at 18:46
9
\$\begingroup\$
  1. In Python you can have multiple functions and classes in a single file (module) but is it considered bad practice to do so?

You can write as many functions and classes within a Python module as long as they serve the same purpose. For instance, you can have in a module these functions:

  • increase_speed()
  • decrease_speed()
  • brake()
  • stop_vehicle()
  • start_vehicle()
  • turn_left()
  • turn_right()

What is considered a bad practice is to add a function like eat_mushrooms() which obviously needs to be in a separate module because of reasons you may have guessed already.

  1. In Python they don't have private. So is there a point to having accessor methods like I do bellow?

As highlighted in the previous answer, the conversion is that a private data's name must start with an underscore _ and use @property annotation instead of setters and getters you have learnt elsewhere.

  1. My IDE suggest I put a comment beneath each function signature. Is it still good to do this when the functions are short and simple, like withdraw(x) in a bank account?

A well written code does not need lot of comments (or function documentation). That is also the case with small functions (and functions have to be small according to C. Martin in Clean Code)

About your code

  1. a) That comment (# bank account class) is not adding any value to your code because the class name highlights that idea already. So do not repeat yourself. That comment is a distraction for the reader of your code.
  2. b) Usually you should treat __init__() as other functions: leave one blank line between the class name and this initializer.

  3. """make a withdraw""" useless and should be removed for the same reason as in 1.a)

  4. """make a deposit""" useless and should be removed. Please read 2.
  5. For the __init__() use as less parameters as you can (C. Martin in Clean code) and from @GarethRees's answer, you should create an other function to set the customer's account like this:

    @property
    def customer_name(self):
        return self._customer_name
    
    @customer_name.setter
    def customer_name(self, customer_name):
        self._customer_name = customer_name
    
  6. Use customer_name instead of name because name is quite confusing (name of a car? country?)
  7. In __init__() you have a named parameter with a default value (balance = 0.00), because of the reasons mentioned in 5 you can get rid of that parameter. So your initializer becomes:

    def __init__(self):
       self._balance = 0.00
    
  8. When your run customer1 = BankAccount("Alex") it is clear that BankAccount() is not a good class name for this context. So Customer() could be a better name. That way, customer1 = Customer("Alex") makes a sense.
  9. Consequently to 7., you should rename the remaining functions as follows: deposit_in_bank_account(amount), withdraw_from_bank_account(amount).

From the previous remarks, your class becomes:

class Customer:

    def __init__(self):
        self._balance = 0.00
        self._customer_name = None

    def deposit_in_bank_account(self, amount):
        self._balance += amount

    def withdraw_from_bank_account(self, amount):
        if amount > self._balance:
            raise ValueError("insufficient funds")
        self._balance -= amount

    @property
    def balance(self):
        return self._balance

    @property
    def customer_name(self):
        return self._customer_name

    @customer_name.setter
    def customer_name(self, customer_name):
        self._customer_name = customer_name
\$\endgroup\$
  • \$\begingroup\$ Nice answer! How would you set the customer_name though ? \$\endgroup\$ – яүυк Apr 15 '17 at 17:15
  • \$\begingroup\$ To get it: print(customer1.customer_name). And to set it, type: customer1.customer_name = "Full Name" @Dex'ter \$\endgroup\$ – Billal Begueradj Apr 15 '17 at 17:36
  • \$\begingroup\$ Yep, I got it eventually after I've read the source of the setter. Thanks ^_^ \$\endgroup\$ – яүυк Apr 15 '17 at 17:38
  • \$\begingroup\$ BankAccount is a more accurate name. A customer could have multiple accounts, and multiple customers can use the same account. It's the variable that needs to change its name. \$\endgroup\$ – zondo Apr 16 '17 at 10:44
  • \$\begingroup\$ @zondo I understand what you mean, and you may be right under a certain context. Look to the OP's original code: customer1 = BankAccount('Alex'). I think you may agree with me that a person can not an instance of a bank account. So I concluded that the operations of withdraw and deposit are done by a person, that is why I suggesed the OP to rename that class (and BankCustomer is actually better name than Customer in this case). But in the opposite context, you are very right. \$\endgroup\$ – Billal Begueradj Apr 16 '17 at 14:00

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.