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
>>>
```
1 Answer 1
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.
- 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.
- 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). - 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 haveCurrency(74.49)
to divide amongst 12 people.Currency(74.49) / 12
outputsCurrency(6.21)
. You give each person that amountCurrency(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.
-
\$\begingroup\$ thank you very much for the review. This has given me quite some insights. Let me work to implement your suggestions. \$\endgroup\$Bruno Vermeulen– Bruno Vermeulen2019年06月20日 11:54:58 +00:00Commented Jun 20, 2019 at 11:54
-
\$\begingroup\$ Try to avoid using
type(...) not in [type1, type2]
, instead useisinstance(..., (type1, type2))
. See also the documentation ofisinstance
on that behalf. \$\endgroup\$AlexV– AlexV2019年06月20日 12:28:13 +00:00Commented Jun 20, 2019 at 12:28 -
\$\begingroup\$ @AlexV yes, I included that point in the answer \$\endgroup\$spyr03– spyr032019年06月20日 13:34:11 +00:00Commented 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\$2019年06月20日 14:18:35 +00:00Commented 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– spyr032019年06月20日 14:24:15 +00:00Commented Jun 20, 2019 at 14:24
Explore related questions
See similar questions with these tags.
decimal
\$\endgroup\$/
rather than integer division//
- are you by any chance writing this for Python 2? \$\endgroup\$