3
\$\begingroup\$

Working with currencies I am aware it is not good to work with floating point values due to rounding and maintaining precision. Better is to work with integers. Would the following Currency class make sense, where an amount is defined as an instance of Currency and converted to cents.

class Currency():

    def __init__(self, value):
        self._cents = int(round(value*100,0))

    def __add__(self, other):
        return self.__class__((self._cents + other._cents)/100)

    def __sub__(self, other):
        return self.__class__((self._cents - other._cents)/100)

    def __mul__(self, factor):
        if type(factor) not in [int, float]:
            raise ValueError('factor must be a scalar')

        return self.__class__(factor * self._cents / 100)

    def __rmul__(self, factor):
        if type(factor) not in [int, float]:
            raise ValueError('factor must be a scalar')

        return self.__class__(factor * self._cents / 100)

    def __truediv__(self, divisor):
        if type(divisor) not in [int, float]:
            raise ValueError('divisor must be a scalar')

        if divisor != 0:
            return self.__class__(self._cents / divisor / 100)
        else:
            raise ValueError('Cannot divide by zero')

    def __repr__(self):
        return str(f'{self._cents/100:,.2f}')

    def __str__(self):
        return str(f'{self._cents/100:,.2f}')

    @property
    def dollars(self):
        return self._cents / 100

    @property
    def cents(self):
        return self._cents

Some results

>>> from money import Currency
>>> a = Currency(100)
>>> b = Currency(0.01)
>>> a+b
100.01
>>> a*2
200.00
>>> a/2
50.00
>>> 2*a
200.00
>>> total = Currency(0)
>>> for i in range(10_000_000):
...     total += b
... 
>>> total
100,000.00
>>> a.dollars
100.0
>>> b.cents
1
>>> 
```
\$\endgroup\$
3
  • 1
    \$\begingroup\$ did you check decimal \$\endgroup\$ Jun 20, 2019 at 8:23
  • \$\begingroup\$ I'm surprised you're using full division / rather than integer division // - are you by any chance writing this for Python 2? \$\endgroup\$ Jun 20, 2019 at 8:27
  • \$\begingroup\$ Yes decimal works to maintain precision for a defined number of digits (the default is I think 28 digits), but I would like to know for the specific case where there are 2 decimals, so to get an integer by multiplying with 100. \$\endgroup\$ Jun 20, 2019 at 8:39

1 Answer 1

2
\$\begingroup\$

Since you are using f strings I will presume you are using python 3.6 or later.

def __init__(self, value):
    self._cents = int(round(value * 100, 0))

Good job prefixing cents with an underscore. I think it is a good choice to make the variable "private".

By leaving out the second parameter from round, or by passing None, the returned value will be rounded to an integer. You could instead write

def __init__(self, value):
    self._cents = round(value * 100)

Note that round may not behave as you expect. Depending on the value given it may round the opposite way to what you'd expect.


def __add__(self, other):
    return self.__class__((self._cents + other._cents) / 100)

It is good that you've accounted for subclassing. Since you've had to do some work to make a new class to return, maybe that indicates you could use an alternative constructor that takes cents directly?

Unfortunately you don't do any type checking here, so the error message a user receives is not very helpful.

a = Currency(2.345)
print(a)  # 2.35
a + 2.03  # AttributeError: 'float' object has no attribute '_cents'

I would suggest type checking as you've done in mul, making sure only currency can be added to currency

def __add__(self, other):
    if not isinstance(other, Currency):
        raise TypeError(f"Unsupported type, cannot add {self.__class__} and {other.__class__}")
    ...

def __mul__(self, factor):
    if type(factor) not in [int, float]:
        raise ValueError('factor must be a scalar')

From this stackoverflow answer, isinstance is preferred over type. Also from the same answer, duck typing is preferred over isinstance. Since we don't want to have to update this list every time we find a new type that is acceptable to multiply by, lets just try it and see if it works. Multiplying a currency will then just work unless there is a reason it shouldn't. One exception might be multiplying currency by currency. Should that be allowed?

def __mul__(self, factor):
    try:
        return self.__class__(factor * self._cents / 100)
    except TypeError:
        raise TypeError(f"...")

This isn't perfect, as we may lose useful information about what doesn't work (Did the constructor fail? Did the multiplication fail? etc) but it is good enough.

# Example of currency just working with fractions
a = Currency(8.08)
a * fraction.Fraction(1, 4)   # 2.02

def __truediv__(self, divisor):
    if type(divisor) not in [int, float]:
        raise ValueError('divisor must be a scalar')

    if divisor != 0:
        return self.__class__(self._cents / divisor / 100)
    else:
        raise ValueError('Cannot divide by zero')

Since an exception is raised if you try to divide a currency by 0, I don't see an advantage of making it a ValueError over a ZeroDivisionError. It wold actually remove the error handling since it doesn't add much, and let the user catch it if they want to.

If there is a usecase for it, it may be worth defining div too. It could be as simple as

__div__ = __truediv__

def __repr__(self):
    return str(f'{self._cents/100:,.2f}')

def __str__(self):
    return str(f'{self._cents/100:,.2f}')

This is repeated code. You can deduplicate with either

def __repr__(self):
    return str(self)

or

__repr__ = __str__

@property
def dollars(self):
    return self._cents / 100

@property
def cents(self):
    return self._cents

These do not make sense to me. I would expect dollars to return the number of dollars present, and cents to just return the number of cents

c = Currency(7.89)
c.dollars  # 7.89, I expected 7
c.cents  # 789, I expected 89

You also could implement dollars.setter and cents.setter so you can change just one of them.


Here I will list things I would do differently. That doesn't mean I'm right and you are wrong, it means food for thought, but reject ideas away if they don't suit.

  1. Use decimal. It was made for arithmetic on numbers where precision matters. It also has the work on dealing with other python numbers pretty much done for you. It also lets you choose how to round which may be important if you need banker's rounding.
  2. Add a constructor for making an amount from dollars and cents. Something like Currency(dollars=7, cents=89) would be nice to use. The implementation is a little trickier (what if cents >= 100, what if dollars is negative).
  3. Due to the rounding, working with this class may look unfair to whoever owns the money. Currency(5.05) / 2 * 2 == Currency(5.04) != Currency(5.05). By doing what should be nothing, they've lost a cent! To exaggerate the problem, lets say you have Currency(74.49) to divide amongst 12 people. Currency(74.49) / 12 outputs Currency(6.21). You give each person that amount Currency(6.21) * 12 == Currency(74.52), and you find you are 3 cent out of pocket. This is when auditors come in and start asking questions.
\$\endgroup\$
6
  • \$\begingroup\$ thank you very much for the review. This has given me quite some insights. Let me work to implement your suggestions. \$\endgroup\$ Jun 20, 2019 at 11:54
  • \$\begingroup\$ Try to avoid using type(...) not in [type1, type2], instead use isinstance(..., (type1, type2)). See also the documentation of isinstance on that behalf. \$\endgroup\$
    – AlexV
    Jun 20, 2019 at 12:28
  • \$\begingroup\$ @AlexV yes, I included that point in the answer \$\endgroup\$
    – spyr03
    Jun 20, 2019 at 13:34
  • \$\begingroup\$ 3. Yeah, Decimal also suffers from that... getcontext().prec = 3, Decimal('5.05') / 2 * 2 == Decimal('5.04'). \$\endgroup\$
    – Peilonrayz
    Jun 20, 2019 at 14:18
  • \$\begingroup\$ @Peilonrayz Good point. However, decimal allows you to specify how rounding is done, so the problem can be partially alleviated. \$\endgroup\$
    – spyr03
    Jun 20, 2019 at 14:24

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.