Simple OOP currency converter

Question

I am learning about OOP so it would be great if you can give me feedback on how to improve my code and OOP design.

This is a currency converter. Firstly, it will call a method to add 2 currencies and commission information and then it converts to another currency whose input is source currency and amount:

import pytest


class Bank:

    def addRate(self,first, second, rate):
        self.first_currency = first
        self.second_currency = second
        self.rate = rate

    def addCommission(self,commision):
        self.commission = commision


    def convert(self, currency, amount):
        if currency == self.first_currency:
            return (amount / self.rate) * (1-self.commission)
        else:
            return (amount * self.rate) * (1-self.commission)

@pytest.fixture()
def bank():
    return Bank()

def test_Bank_addRate(bank):

    bank.addRate("USD","GBP",2)
    assert bank.first_currency == "USD"
    assert bank.second_currency == "GBP"
    assert bank.rate == 2

def test_Bank_addCommision(bank):
    bank.addCommission(0.015)
    assert bank.commission == 0.015


def test_Bank_convert(bank):
    bank.addRate("USD", "GBP", 2)
    bank.addCommission(0.015)
    assert bank.convert("USD", 100) == 49.25

Answer 1

First: welcome to CodeReview! I hope that you get good feedback.

About your code. Your usage of Python is not terrible; most of your problems are conceptual, about the decisions you've made about your class. Looking at the method signatures from the outside, one would guess that addRate() can be called multiple times per single Bank instance, and that convert would then allow for any currency to be converted. That isn't the case; instead, your "Bank" class is actually closer to an "ExchangeRate" class.

There are two ways to go, here. Either make your class an ExchangeRate, in which case

• you need to rename the class
• convert, rather than accepting the name of a currency, should perhaps accept a forward boolean. Or, if you want to keep passing the name of a currency, it's important to make the argument name more clear, i.e. dest_currency.
• Rename your addCommission to setCommission. "add" implies having more than one.
• Rename your addRate to setRate. "add" implies having more than one.

Or, keep a "Bank" class, in which case:

• you need to change the way that you store your exchange rates, probably to a dictionary-based system instead of a single float member
• convert would accept both a source and destination currency
• addRate's signature would stay the same, but its contents would change to use your dict.

Answer 2

There are a few smells in your code.

1. Currency exchange rates are usually not inverses of each other. Your code assumes that GBP / USD is one number, and that number is a property of your Bank object. That is a smell, because a. banks don't own exchange rates, and b. there should at least be a GBP->USD rate as well as the USD->GBP rate.

2. What if you want to do GBP->EUR? do you create another bank?

3. In real life, currency exchange rates change over time. How will your bank object reflect that?
4. Your bank is mutable (this could be part of the answer to the question above, but then the next question is Who will change the bank if the exchange rates change?). Mutable means: when a method takes a bank as parameter, there is nothing to stop it from saying bank.addRate("RUB", "YEN", 1.71), and that would change the original object, which is a. bad, and b. in any case not what you'd expect an addRate method to do.
5. Your code is striving for symmetry. Is that a good thing? You might have an Amount(amount, currency) class one day, and then your bank would probably be asked to bank.convertTo(amout, targetCurrency) which is distinctly not symmetric.

A typical mistake programmers new to oop often make is to model structures instead of behaviours. The tendency is to look at the bones of what you want to model, not at the muscles. Hence you come up with a Bank class, because a bank is a thing, and a Currency class, because a currency is a thing, and then it's quickly unclear what these classes actually are supposed to do. Try to think more in terms of What is being done, and less in terms of Who is doing it.