Simple OOP currency converter - follow-up

Question

This is a follow up question on this post

This is what I changed on my previous code:

1. I use Exchanger and PairCurrency class instead of Bank

2. I renamed addRate and addCommission to setRate and setCommission respectively

3. Exchanger object contains multiple PairCurrency objects which can be added to Exchanger by addPairCurrency() method.

This is my code and tests:

import pytest

class DuplicatePairError(Exception):
    pass

class Exchanger:
    def __init__(self):
        self.pair_currencies = []
        self.commission = 0.015

    def addPairCurrency(self, pair_object):
        if pair_object in self.pair_currencies:
            raise DuplicatePairError('Cannot add duplicate pair of currency')

        self.pair_currencies.append(pair_object)

    @property
    def last_pair(self):
        return self.pair_currencies[-1]


    def getRate(self, pair):
        for pair_obj in self.pair_currencies:
            if pair_obj.name == pair:
                return pair_obj.rate

    def convert(self, src_dest, amount):
        return amount / self.getRate(src_dest) * (1-self.commission)


    def editRate(self,new_object):
        for pair_obj in self.pair_currencies:
            if pair_obj.name == new_object.name:
                 pair_obj.rate = new_object.rate

class PairCurrency:

    def __init__(self, name):
        self.name = name

    def setRate(self, rate):
        self.rate = rate

    def editRate(self, new_rate):
        self.rate = new_rate

    def __eq__(self, object):
        return self.name == object




def test_canInitializePair_CurrenciesListInExhangeClass():
    exchanger = Exchanger()

    assert exchanger.pair_currencies == []

def test_canSetRateinPairCurrencyClass():
    USD_GBP = PairCurrency('USDGBP')
    USD_GBP.setRate(2)

    assert USD_GBP.name == 'USDGBP'
    assert USD_GBP.rate == 2

def test_canSetCommisionInExchanger():
    exchanger = Exchanger()

    assert exchanger.commission == 0.015

@pytest.fixture()
def USD_GBP():
    USD_GBP = PairCurrency('USDGBP')
    USD_GBP.setRate(2)
    return USD_GBP

@pytest.fixture()
def GBP_USD():
    GBP_USD = PairCurrency('GBPUSD')
    GBP_USD.setRate(0.5)
    return GBP_USD

@pytest.fixture()
def exchanger(USD_GBP,GBP_USD):


    exchanger = Exchanger()
    exchanger.addPairCurrency(USD_GBP)
    exchanger.addPairCurrency(GBP_USD)
    return exchanger

def test_canAddPairCurrencyinExchangerClass(exchanger, USD_GBP, GBP_USD):


    assert USD_GBP == exchanger.pair_currencies[-2]
    assert GBP_USD == exchanger.last_pair

def test_cannotAddDuplicatePair(exchanger, USD_GBP):
    USD_GBP2 = PairCurrency('USDGBP')
    USD_GBP2.setRate(5)



    exchanger = Exchanger()
    exchanger.addPairCurrency(USD_GBP)
    with pytest.raises(DuplicatePairError) as excinfo:
        exchanger.addPairCurrency(USD_GBP2)
    assert str(excinfo.value) == 'Cannot add duplicate pair of currency'


def test_canEditRateInPairCurrencyClass(USD_GBP):


    USD_GBP.editRate(3)
    assert USD_GBP.rate == 3

    USD_GBP.editRate(4)
    assert USD_GBP.rate == 4

def test_canEditRateinExhangerClass(exchanger):
    USD_GBP = PairCurrency('USDGBP')
    USD_GBP.setRate(5)
    exchanger.editRate(USD_GBP)
    assert exchanger.getRate('USDGBP') == 5

def test_canConvertInExchangerClass(exchanger, USD_GBP, GBP_USD):

    assert exchanger.convert(src_dest = 'USDGBP', amount = 100) == 49.25
    assert exchanger.convert(src_dest='GBPUSD', amount=100) == 197

I have few questions:

1. How good is my OOP design?

2. Is my test list fine?

3. Should I remove editRate() in PairCurrency and just keep editRate() in Exchanger?

4. Do my classes violate the SOLID Principle ?

5. What I have to fix to improve my code and OOP design ?

Answer 1

Set?

You have logic preventing duplicate currency pairs. The easier (and more performant) thing to do is to simply represent pair_currencies as a set instead of a list. When you add, it will automatically discard duplicates.

snake_case

The standard is to name methods like your last_pair - i.e., get_rate.

Lookups

You have a loop in getRate and editRate to try and find the correct currency. Instead, you should be using a dictionary, which does not require a loop.

Remove duplicate methods

editRate and setRate do the exact same thing, so delete the former.

Equality test is wrong

Your __eq__ method should be comparing to an object instance, not a string. As such, you should be comparing self.name with object.name.

Answer 2

First of all I would suggest you changing PairCurrency constructor from

class PairCurrency:
    def __init__(self, name):
        self.name = name

to

class PairCurrency:
    def __init__(self, name, ratio):
         self.name = name
         self.ratio

There is literally no reason to make it different. This is not a functional class but rather DataStructure.

It's also much easier to create instances with

us = PairCurrency('US', 3)

than

us = PairCurrency('US')
us.setRate(3)

As stated above, __eq__ method is wrong. It should be:

def __eq__(self, other):
    return self.name == other.name and self.rate == other.rate

Also there is no need to create your own structures like that.

Your PairCurrency class isn't better than just a simple python tuple.

PairCurrency('US', 3) == PairCurrency('US', 3) => true
('US', 3) == ('US', 3) => true

The only reason you would want to do that is to increase verbosity, but this can be achieved by using builtin namedtuples.

from collections import namedtuple

PairCurrency = namedtuple('PairCurrency', 'name rate')
us = PairCurrency('US', 3)
print(us) 
=> PairCurrency(name='US', rate=3)

Sets would work the same with tuples, namedtuples and your custom PairCurrency with custom __eq__.