7
\$\begingroup\$

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
\$\endgroup\$
  • 5
    \$\begingroup\$ Sometimes, the elegant implementation is just a function. Not a method. Not a class. Not a framework. Just a function. \$\endgroup\$ – juhist Jul 2 at 13:12
  • 1
    \$\begingroup\$ Your code defines a Currency class but it doesn’t seem to be using it — what is its purpose? \$\endgroup\$ – Konrad Rudolph Jul 2 at 13:14
  • \$\begingroup\$ @KonradRudolph, thank you. I removed it \$\endgroup\$ – Nguyen Jul 2 at 16:02
  • \$\begingroup\$ @juhist Thank you for your advice. I agree but now I am learning about OOP so I want to focus on it \$\endgroup\$ – Nguyen Jul 2 at 16:04
  • 2
    \$\begingroup\$ Welcome to Code Review! It appears you have modified the code slightly, mostly in response to Konrad's comment above. After getting an answer you should not change your code anymore. This is to ensure that answers do not get invalidated and have to hit a moving target. If you have changed your code you can either post it as an answer (if it would constitute a code review) or ask a new question with your changed code (linking back to this one as reference). Refer to this post for more information \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Jul 2 at 16:08
10
\$\begingroup\$

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.
\$\endgroup\$
4
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ 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 \$\endgroup\$ – Nguyen Jul 3 at 0:17
  • \$\begingroup\$ Hi, I re-implement my code and created follow-up question here. Can you take a look ? codereview.stackexchange.com/questions/223411/… \$\endgroup\$ – Nguyen Jul 3 at 12:19
  • \$\begingroup\$ 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. \$\endgroup\$ – wallenborn Jul 3 at 14:50

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.