# Simple OOP currency converter

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:

self.first_currency = first
self.second_currency = second
self.rate = rate

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()

assert bank.first_currency == "USD"
assert bank.second_currency == "GBP"
assert bank.rate == 2

assert bank.commission == 0.015

def test_Bank_convert(bank):
assert bank.convert("USD", 100) == 49.25

• Sometimes, the elegant implementation is just a function. Not a method. Not a class. Not a framework. Just a function. – juhist Jul 2 '19 at 13:12
• Your code defines a Currency class but it doesn’t seem to be using it — what is its purpose? – Konrad Rudolph Jul 2 '19 at 13:14
• @KonradRudolph, thank you. I removed it – Nguyen Jul 2 '19 at 16:02
• @juhist Thank you for your advice. I agree but now I am learning about OOP so I want to focus on it – Nguyen Jul 2 '19 at 16:04

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.

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.

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.

• Thank you for anwering me. Let me answer your question: 1. I will change Bank class to ExchangeRate. I will create a dictionary rate = {"GBPUSD": 2, "USDGBP: 0.5} 2. So you mean I should create a dictionary for rate ? So I don't have to call another ExchangeRate everytime ? 3. I think I should add method: changeRate 4. I should raise exception as soon as addRate was called 2nd time for same pair currency and add new method: changeRate() which can be called multiple time 5. I don't quite understand the point. I think I should create a class for storing currency amounts – Nguyen Jul 3 '19 at 0:17
• Hi, I re-implement my code and created follow-up question here. Can you take a look ? codereview.stackexchange.com/questions/223411/… – Nguyen Jul 3 '19 at 12:19
• 4. My point is that rates change over time, so you don't want them to be constant, but clients should not be allowed to change them, so you want them to be immutable. Think about how to solve this. – wallenborn Jul 3 '19 at 14:50