brick / money

A money and currency library for PHP
MIT License
1.65k stars 102 forks source link

Scales and Money classes #3

Closed BenMorel closed 7 years ago

BenMorel commented 7 years ago

Part of the list of things I want to work on before tagging a release, as mentioned in #2. Comments welcome.

Currently, a single class, Money, allows to work with default scales (1.50 USD) but also with arbitrary scales (1.123456 USD). I like this flexibility, but it comes at a price:

To be honest, this kind of problem never occurred to me so far, as I have full control over my code from A to Z, and usually only deal with default scale monies. Still, it has always made me feel uncomfortable, and I'm afraid that it could lead to potential issues in third-party libraries dealing with monies. Should we expect them to do their own scale check, or to trust that the caller will give a money with the correct scale? Or should we provide a way to enforce at least default scale monies?

I can see 2 options here:

When I started this library, I didn't like Joda's Money/BigMoney approach very much. I believed in a generic Money class that would handle all use cases. Now that I have it somehow, I realize that there might be more drawbacks than advantages.

In most projects, needless to say that we'll be using default scale monies way more often than arbitrary scale ones. So it does make sense to have a special case (class) for them. This way, one can be sure when dealing with a Money, that it's always the default scale.

As a final note, I also checked the approach introduced by the new Java Money API (JSR-354) and its reference implementation. It made me curious as they use a common interface (MonetaryAmount) for all monies. I thought this was good, until I realized that it is so generic that implementations have to store the context in the Money class itself. The consequence of this is that when you accept a Money, you get not only a value and a currency, but also how it's supposed to be rounded. I don't believe this is good, as I prefer to receive only a monetary amount, and decide how I will perform the calculations and roundings in every operation I write. It's also worth noting that for now, Joda Money, which is very popular in the Java world, doesn't implement JSR-354; this may change as Java 9 (the first version to include the Money API) is not out yet, but last time I checked, the lead developer was not keen on the spec.

Note that if I were to follow the Money/BigMoney path, this would affect these two other issues:

I'm really looking forward to your opinion on this one, which I consider the most important issue of all.

jkuchar commented 7 years ago

Approach with two money classes feels wrong to me, as it looks to me more like an issue with currency then with money itself.

I use brick\money with Czech Crowns (CZK). Minimum amount you can pay in cash is 1 CZK, in cashless operation you can transfer 0,01 CZK as a minimum. I would like to never mix these together unintentionally, as the result will always be in the cashless scale.

I have never used higher decimal precision money objects then what real currency actually supports. Once I've needed to make it "absolutely" precise (for some internal computing) and wanted to round just the result, I have made variant of Money on top of BigRational, as this makes the point where rounding happens explicit (the place where you convert rational into Money).

It looks to me that scaling should be tightly coupled to its currency. As 1,23 CZK is valid amount for a cashless operations, it is invalid for cash. There must be explicit point where I convert one into another. I would never want to make operations across them, without explicitly saying so (and then counting with consequences - result will be in higher precision version of currency).

It makes sense for me to change Currency's default precision into hardly require precision. If you want to restrict your precision just for cash amounts or make more precise version with 10-decimals, you can always create own currency for that.

This approach is also kind of weird as those "virtual" currencies actually does not exists in real world and in this model variants of the currency has no relation. This will restrict operations across money in different precision at all. This seems too strict to me and it also "bends" reality.

What about splitting currency and it's precision. This will result in creating family of currencies that share the one real world currency and allows to distinguish between different currency and the same currency with different precision. As result there will be no simple way how to do operations across different currencies and there can be flag which allows to deal with money in the same currency, but in different scale. Results of operations will always be in the currency with the highest precision which has been involved in performed operation.

As making code, that counts with operations on different precisions explicit, operations on different scales will then be always intentional. This will avoid errors mentioned in the initial post of this issue and will not introduce unnecessarily new class.

It would look something like this:

$cashCZK = Currency::of('CZK', 0);
$cashlessCZK = Currency::of('CZK', 2);

$coffeePrice = Money::of("25", $cashCZK);
$couvert = Money::of("50.35", $cashlessCZK);

$coffeePrice->plus($couvert); // error
$total = $coffeePrice->plus($couvert, Money::ALLOW_DIFFERENT_SCALES);
echo $total; // CZK 75.35
assert($total->getCurrency() === $cashlessCZK); // succeeds
BenMorel commented 7 years ago

Thanks for your comment, and for detailing your interesting use case! I had never dealt with CZK so far. As I can see in the ISO currency list (which is the one Brick\Money uses), CZK is defined with 2 minor units, which is what you call the cashless version.

As for cash payments, CZK is not the only currency to not have coins for the smallest unit. Actually many of them do, and it's not always rounded to 1 whole unit of currency, but may be rounded to 0.05 (CHF), 0.10 (NZD), etc. Actually I can see in Wikipedia that Sweden did have 0.50 coins until recently, and even smaller units in the past.

So as far as I can see, the "cash rounding" problem is broader than a scale problem. If all cash roundings where in 1 or 0.10 units, it could be solved with scale. Cash roundings in 0.05 units cannot be solved with scale.

Did you check CashRounding? It was designed specifically to solve this problem.

In the case of CHF (0.01 scale, 0.05 coins), you would use CashRounding with a step of 5. In the case of CZK (0.01 scale, 1.00 coins), you would use CashRounding with a step of 100:

$rounding = new CashRounding(100, RoundingMode::DOWN);
$context = new RetainContext($rounding);

echo $money = Money::of(100, 'CZK'); // CZK 100.00
echo $money->dividedBy(3, $context); // CZK 33.00

So while it does still have the "cashless" scale, it does not contain any minor units that cannot be represented in real life.

While I understand your whole thinking about only creating monies in scale 0 when you're dealing with cash, this thinking could not be applied to Switzerland: when dealing with cash, they cannot represent 0.01 CHF, but still need to be in scale 2 to represent coins of 0.05 CHF. So this use case is actually very specific to Sweden!

If we wanted to add support for monies that can only represent cash, we would need to embed the CashRounding into the money itself, which although possible (that's what you suggest by embedding it into Currency), seems to me like a bad idea as I said in #4: now the Money dictates how it's supposed to be rounded when it's multiplied or divided. I believe that the person who writes the multipliedBy() or dividedBy() line of code should choose the rounding mode, not the person who constructed the Money.

It makes sense for me to change Currency's default precision into hardly require precision. If you want to restrict your precision just for cash amounts or make more precise version with 10-decimals, you can always create own currency for that.

I don't like the idea of having multiple Currency objects that target the same currency code, this feels wrong to me.

What about splitting currency and it's precision (...)

Unless I missed something, your code above could easily implemented without touching the Currency: we would just need to make Money fail not only when adding monies with different currencies, but also when adding monies with different scales. Do we really want to do that, though?

As far as I'm concerned, the main problem is to be able to quickly identify (type safety would be a must) whether or not the Money I accept as a function parameter is the kind of money I expect (scale-wise).

After that, once I got this Money, I know that all the operations I perform will stay in the same scale, so I'm safe.

BenMorel commented 7 years ago

Reacting to my own comment:

If we wanted to add support for monies that can only represent cash, we would need to embed the CashRounding into the money itself, which although possible (...), seems to me like a bad idea

This is actually based on the current assumption that CashRounding contains the rounding mode.

If we were to implement CashRounding's "steps" into Money directly, you could do something like:

$money = Money::of(500, 'CZK', 100 /* steps */); // CZK 500.00
echo $money->dividedBy(3, RoundingMode::DOWN); // CZK 166.00

or for CHF:

$money = Money::of(500, 'CHF', 5 /* steps */); // CHF 500.00
echo $money->dividedBy(3, RoundingMode::DOWN); // CHF 166.65

Basically the Money dictates what it can contain (and all monies derived from it will follow the same rules), but the caller remains in control of rounding.

This is interesting.

BenMorel commented 7 years ago

I started playing a bit with this idea.

Some new thoughts:

I quite like the idea, but quite dislike the lengthy of() parameter list.

You have a point, saying that the Currency could dictate the Money's scale (and the step, then). As long as we don't move the rounding mode to the Currency, and the caller is still in control of rounding, I don't mind. It still bothers me a little to have several Currency objects for a single currency code, though.

jkuchar commented 7 years ago

Love it! I think this is the right way to go. Rounding is in control of caller of operation and creator of money can enforce rectrictions he needs to be kept. Yay! πŸ’―

It still bothers me a little to have several Currency objects for a single currency code, though.

It is just the first idea. Having more currency instances feels artificial to me, this model is still missing some concept. Lets break out the two first lines:

/* 1 */ $cash = Money::of(150, 'CZK', 2, 100);
/* 2 */ $cashless = Money::of('2.50', 'CZK', 2, 1);

What bothers me on code above is that user that construct money object must know that CZK has scale 2 and 100 step for cash and 1 for cashless operations. These three parameters are closely related. If we find out some proper name for those three (together), it can help a lot.

What about having currency and form? Is there any currency on the Earth that has more forms than cashless and cash? However this will require us to find cash steps for all currencies in ISO list. :-/

$cash = Money::of(150, 'CZK', MoneyForm::CASH);

This requires storing scale and steps for all variants of given currency inside Currency object. Is that something that is real to achieve?

BTW: CZK used to have cash scale 1, 10, 20, 50 and now 100. :-)

BenMorel commented 7 years ago

What about having currency and form? Is there any currency on the Earth that has more forms than cashless and cash? However this will require us to find cash steps for all currencies in ISO list. :-/

I wouldn't open the Pandora box and try to maintain such a list. ISO provides default scales and nothing more, so I would stick with this and leave the cash/cashless gory details up to the developer, who knows best how to deal with his currency. Plus there can be many "forms" for a money: for example, Forex traders may use 4 or 5 decimals for the dollar. Steps however, will probably be used for cash only, and even though there might be just one cash "form" per currency, I wouldn't try to hardcode it.

BTW: CZK used to have cash scale 1, 10, 20, 50 and now 100. :-)

That's exactly why I don't want to maintain such a list: even if we managed to get it right today (I know we won't), things evolve rapidly!

Having more currency instances feels artificial to me, this model is still missing some concept.

What about reusing the name Context, but for something else: an object that would contain a scale and a step only:

$cashContext = (new Context)->withStep(100);
$precisionContext = (new Context)->withScale(6);
Money::of(150, 'CZK'); // scale 2, step 1
Money::of(150, 'CZK', $cashContext); // scale 2, step 100
Money::of(150, 'CZK', $precisionContext); // scale 6, step 1

At least, you don't have to provide the scale, you can just pass a custom step.

Another idea could be a wrapper for Currency, that you could pass in place of the currency code / Currency object as a second parameter to Money::of():

$czk = Currency::of('CZK');
$cashCZK = (new CurrencyContext($czk))->withStep(100);
$money = Money::of(150, $cashCZK);

But then what's the point of wrapping a Currency in this object at all: we only need its currency code, and its default scale only as a default when constructing the object (edit: this is wrong, see my next post). So in the end this object contains:

Which is pretty much what we could fit in Currency itself. Back to your original idea.

This is starting to get less ugly to me: being able to use a "modified" currency that would have a different scale/step than the default ISO currency:

Money::of(150, 'CZK'); // ISO currency: scale 2, step 1

$cashCZK = Currency::of('CZK')->withStep(100);
Money:of(150, $cashCZK); // custom currency: scale 2, step 100

It's still CZK after all, it's just a restricted version in terms of numeric capabilities.


Side note: I have no idea whether it even makes sense to use a step together with a custom scale: if you're using a step, you're most probably using the default scale. Why would you ever use scale = 6 with step != 1? Allowing to use both doesn't hurt, as it doesn't make the implementation more complex, but it's good for the API not to be too broad if it's useless.

BenMorel commented 7 years ago

So as I said in #6:

this is a point against allowing multiple instances of a given currency with different scales (#3): we need to know, from the Currency object, what the official scale is, for ofMinor() / getAmountMinor() to work properly.

I think we'll keep a single Currency instance just for this reason.

Furthermore, let's not complicate it too much: we actually only ever need to provide a custom scale/step when creating a money, so we're basically just talking about making Money::of() cleaner. Let's not introduce too much bloat around this.

Maybe the scale/step Context object is not too bad?


edit: the below 2 alternative ideas do not provide a solution to the MoneyBag::getValue() issue that I raised in the next post; I therefore think that this should take the form on an object to be passed in the Money::of() call, and that MoneyBag::getValue() could use as well. Anyway, here they are:


Yet another option could be to leave Money::of() as it is, and allow the step to be changed afterwards:

$money = Money::of(150, 'CZK')->withStep(100);

Or build these custom monies through a factory:

$cashCZKFactory = MoneyFactory::forCurrency('CZK')->withStep(100);
$money = $cashCZKFactory->createMoney(150);

Not that I like factories very much, but it deserves to be mentioned.

BenMorel commented 7 years ago

Another question arises while playing with the code:

How should Money::total() behave, when scales (and steps) are mixed?

Similar question for MoneyBag::add() and MoneyBag::subtract(); the difference here is that you start with an empty MoneyBag, so there is no such thing as "first money in the list" to get the scale & step from.

I'm thinking that internally MoneyBag should just store BigDecimals, and the (optional) choice of a scale and step should be performed in the last step, when converting to a Money in a given Currency.

But how do we allow these parameters to be passed? The current signature is:

public function getValue($currency, CurrencyConverter $converter);

Maybe we do need this kind of Context (scale,step) object, that could be handy here as well, to create the kind of object you want:

$bag = new MoneyBag();
$bag->add(Money::of(100, 'USD'));
$bag->add(Money::of(50, 'EUR'));
$bag->getValue('CZK', $currencyConverter, $cashContext);

Ideas welcome.

jiripudil commented 7 years ago

I would stick with this and leave the cash/cashless gory details up to the developer, who knows best how to deal with his currency. Plus there can be many "forms" for a money: for example, Forex traders may use 4 or 5 decimals for the dollar

:+1:

I like the let-the-developer-deal-with-it approach. Even with CZK, in a real-world scenario you want to keep the default scale when summing up order items, and only after that round to the nearest step for cash representation, so that 12.20 + 14.40 is 26.60 rounded up to 27, and not 12.20 and 14.40 both rounded down, adding up to 26. Only the developer knows what precision their use case requires.

Some kind of scale/step context class (or method as in $money->withStep(100)) sounds like a good idea. If not provided, the default scale of the currency should be used, and operations on two money objects with incompatible scale/step contexts should probably throw an exception – if you work with arbitrary scales, you probably know what you're doing and the possibility of an exception shouldn't surprise you, and if you simply rely on the default scale and some third-party function catches you unprepared, getting an explanatory exception is still better than unmet expectations without warning.

I don't know about Money::total() nor MoneyBag, I haven't yet used either of them.

BenMorel commented 7 years ago

Only the developer knows what precision their use case requires.

:+1:

operations on two money objects with incompatible scale/step contexts should probably throw an exception

I'm not sure about this one. Say I have USD 1.20 + USD 1.5000. While I agree that it would be weird to encounter this without being prepared to it, why throw an exception when the result can fit in the original money's scale without rounding?

Also, if you threw an exception even for different steps, this would mean that you could not add CZK 3.00 with step 100 and CZK 2.00 with step 1 without converting them first?

VasekPurchart commented 7 years ago

Say I have USD 1.20 + USD 1.5000. While I agree that it would be weird to encounter this without being prepared to it, why throw an exception when the result can fit in the original money's scale without rounding?

I am not sure, which cases you mean precisely by "can fit", but if you mean that USD 1.20 + USD 1.5000 is ok, but USD 1.20 + USD 1.5001 would throw exception, then from my point of view this is about consistency - the values can (and probably at some point will) come from user, and you do not know what range of inputs are coming. Meaning you would have to make sure beforehand. Otherwise you are risking the situation that everything is working "just fine", until the moment that it isn't. And it does not need to be an unexpected input from a form, it cal well also be an accumulated part of something, which reaches that point after some time.

Throwing exception at different scales (by default) would "warn" you exactly of these cases ahead. Or you could provide a different context, thereby saying "I know what I am doing" :)

BenMorel commented 7 years ago

if you mean that USD 1.20 + USD 1.5000 is ok, but USD 1.20 + USD 1.5001 would throw exception,

Exactly.

the values can (and probably at some point will) come from user, and you do not know what range of inputs are coming

If you're processing user input, you should be creating a Money from this input? Then at the time you create this Money, you're in control of the scale & step!

I personally wouldn't do $money->plus($userInput) straight away, I would create a Money first. But in any case your code should look like:

// Creating a Money first

try {
    $userInput = Money::of($userInput, $currency);
} catch (ArithmeticException $e) {
    // invalid user input
}
$money = $money->plus($userInput); // safe

// Without creating a Money first

try {
    $money = $money->plus($userInput);
} catch (ArithmeticException ,$e) {
    // invalid user input
}

What's wrong with that?

VasekPurchart commented 7 years ago

I agree with the "creating another instance of Money first". But I don't know if I am missing something, but where would the scale in this example come from?

BenMorel commented 7 years ago

of() would use the currency's default scale, unless instructed otherwise:

Money::of('1.1', 'USD'); // USD 1.10
Money::of('1.123', 'USD'); // RoundingNecessaryException

And plus() would use the scale of the left operand (at least by default), and throw an exception as well if rounding is necessary to fit the scale.

jiripudil commented 7 years ago

I'm not sure about this one. Say I have USD 1.20 + USD 1.5000. While I agree that it would be weird to encounter this without being prepared to it, why throw an exception when the result can fit in the original money's scale without rounding?

Also, if you threw an exception even for different steps, this would mean that you could not add CZK 3.00 with step 100 and CZK 2.00 with step 1 without converting them first?

You could take "incompatible" in the broader sense and just proceed with the operation if the result can fit in the target scale/step, that's one less worry for the developer. But if you are strict about converting user input to Money with desired scale, why not be strict about getting Money from or passing it to (third-party) code? "Only the developer knows what precision their use case requires," so it should be their responsibility to convert the Money to the desired precision, and it's nice to notify them if they forget to do so somewhere.

If USD 1.20 + USD 1.5000 is ok while USD 1.20 + USD 1.5001 is not, and if both can happen at the same place in the code given different input, the developer needs to handle the possibility anyway. It doesn't have to be user input, it can be a higher-precision computation where dummy data gives me 1.5000 and Money lets it pass, whereas production data yield 1.5001 and blow up the app. I'd prefer Money to fail fast and warn me that I have forgotten to make sure the scale fits – it's an easy fix for me and I can sleep in peace at night.

The argument seems even stronger with third-party code – you have full control over your code, and this way you could take control over the effects of third-party code as well. And vice versa, the third-party code could be sure you use it correctly, i.e. with money of the expected scale.

As to incompatible steps, it feels like a similar situation. You set a specific step to Money because you want to handle it in a specific way, and if a Money comes that is not meant to work in such mode of operation, it's probably by accident, not by design.


I don't know how much code bloat this would add, but I suspect little to none. I think we're still talking about edge cases here:

To be honest, this kind of problem never occurred to me so far, as I have full control over my code from A to Z, and usually only deal with default scale monies

Me too – I work with default-scale default-step Money most of the time, so caution (i.e. try-catch) is in place only when deviating from those defaults or working with third-party code. In the first case, throwing an exception on incompatible contexts prevents me from shooting myself in the foot accidentally, in the latter it gives me some assurance about what to expect from third-party code.

BenMorel commented 7 years ago

If USD 1.20 + USD 1.5000 is ok while USD 1.20 + USD 1.5001 is not, (...) I'd prefer Money to fail fast and warn me that I have forgotten to make sure the scale fits – it's an easy fix for me and I can sleep in peace at night.

You have a point, but I'm a bit afraid of the consequences of making Money anally strict. At the very least, I think it should accept monies that have a scale smaller than that of your Money, as these will never require rounding.

As to incompatible steps, it feels like a similar situation. You set a specific step to Money because you want to handle it in a specific way, and if a Money comes that is not meant to work in such mode of operation, it's probably by accident, not by design.

Maybe. That is if we store the step in Money, anyway; @VasekPurchart in #4 is against this; it would be interesting if you could challenge each other's ideas?

jkuchar commented 7 years ago

I must say that I have originaly meant that incompatible scales/steps, would throw an exception always - on any incompatibility as @jiripudil proposes. (doesn't matter on which side is lower precision)

And it still seems like an good idea to me. @BenMorel, could you think of an usecase when this behaviour could make usage cumbersome?

I could imagine just one, there are two monies and you do not know scale. You want to add them together ending with the higher scale. This would let you to covert them to the same scale and then add. Or use some external math opration as proposed by @VasekPurchart. But to be honest, this still seem quite artificial to me. This is very advanced use of the library and would not mind if code will me more complex for this. E.g. constucting context and passing it in the operation or performing operation provided by context or really coverting them to the same higher scale (this seems most natural to me)

If i would need this in my app I would make a function for this Money toHigherScale(Money, Money). Then operations on this will be again (mentaly) easy to do.

BenMorel commented 7 years ago

Ah, didn't get that, sorry. The use case I can think of, in particular, is your CZK cash/cashless example: Wouldn't you encounter this:

$cashless = Money::of('99.90', 'CZK');
$cash = Money::of('50', 'CZK', /* step */ 100);

$total = $cashless->plus(cash); // CZK 149.90

Will you never mix cashless and cash in an operation?

What about allowing scales smaller than the Money's scale, and steps compatible with it? Something like:

A.scale >= B.scale
A.step % B.step == 0

Sure, A + B and B + A may return different scales, and one can throw an exception and the other not.

VasekPurchart commented 7 years ago

You could take "incompatible" in the broader sense and just proceed with the operation if the result can fit in the target scale/step, that's one less worry for the developer.

Yes, that's exactly where I was heading.

Or use some external math opration as proposed by @VasekPurchart. But to be honest, this still seem quite artificial to me. This is very advanced use of the library and would not mind if code will me more complex for this. E.g. constucting context and passing it in the operation or performing operation provided by context or really coverting them to the same higher scale (this seems most natural to me)

I wasn't really proposing anything external to be mandatory, will show an example in the #7 issue.

BenMorel commented 7 years ago

I must say that I have originaly meant that incompatible scales/steps, would throw an exception always - on any incompatibility as @jiripudil proposes. (doesn't matter on which side is lower precision)

This is when doing plus(Money) by the way, what should we do when doing plus(1), plus('1.2'), should we throw an exception as well because the "scale" doesn't match, even though the parameter is not a Money?

BenMorel commented 7 years ago

Most of the points here have been either resolved, or are being discussed in #7.