-
-
Notifications
You must be signed in to change notification settings - Fork 102
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Scales and Money classes #3
Comments
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 |
Thanks for your comment, and for detailing your interesting use case! I had never dealt with CZK so far. 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. $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
I don't like the idea of having multiple Currency objects that target the same currency code, this feels wrong to me.
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. |
Reacting to my own comment:
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. |
I started playing a bit with this idea. Some new thoughts:
I quite like the idea, but quite dislike the lengthy 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. |
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 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. :-) |
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.
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!
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 $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:
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 |
So as I said in #6:
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 Maybe the scale/step Context object is not too bad? edit: the below 2 alternative ideas do not provide a solution to the Yet another option could be to leave $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. |
Another question arises while playing with the code: How should
Similar question for 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. |
👍 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 I don't know about |
👍
I'm not sure about this one. Say I have Also, if you threw an exception even for different steps, this would mean that you could not add |
I am not sure, which cases you mean precisely by "can fit", but if you mean that 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" :) |
Exactly.
If you're processing user input, you should be creating a I personally wouldn't do // 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? |
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? |
Money::of('1.1', 'USD'); // USD 1.10
Money::of('1.123', 'USD'); // RoundingNecessaryException And |
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:
Me too – I work with default-scale default-step Money most of the time, so caution (i.e. |
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.
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? |
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 |
Ah, didn't get that, sorry. The use case I can think of, in particular, is your CZK cash/cashless example: $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:
Sure, |
Yes, that's exactly where I was heading.
I wasn't really proposing anything external to be mandatory, will show an example in the #7 issue. |
This is when doing |
Most of the points here have been either resolved, or are being discussed in #7. |
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:You never know what kind of money you're dealing with. Say you write a function that accepts a
Money
and performs some basic calculations on it:You're expecting that everyone will call this function with a typical money like
1.23 USD
, but suddenly someone calls your function with1.2345 USD
. Because Money retains the left operand scale by default, your function will return3.1345 USD
. Probably not what you want. You may want to fail in some way here, such as throwing an exception.Sure, you could check
$money->getAmount()->getScale()
, but doing so in every function that accepts a Money? Let's be honest: you don't want to do that.I personally store all my (default scale) monies in the database as integers, representing "minor units" (cents). For example,
1.23 USD
is stored as123
, while500 JPY
is just stored as500
.To do that, I call
$money->getAmountMinor()
that gives me123
for1.23 USD
. The problem is, by doing so I'm assuming that I'm dealing with a standard scale money; if for any reason I get a high precision money such as1.2345 USD
,getAmountMinor()
will return12345
, and if I blindly store this in my database to later retrieve it as a standard scale money, I would end up with123.45 USD
! Same problem as above then, we may need a way to enforce a default scale money at some point for safety reasons.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:
leave it as it is, and maybe at least provide a convenience method such as
isDefaultScale()
so that methods not trusting the caller may more easily do their own check;use type safety to have a way to ensure that you're dealing with a standard scale money. In this scenario, we would have 2 different classes for default scale monies and arbitrary scale monies, which could be implemented in many different ways:
Money
(default scale) andBigMoney
(arbitrary scale). This is the approach used by Joda Money (Java); in their implementation, the 2 classes are not fully interchangeable. For example, whileBigMoney::plus()
accepts either aMoney
or aBigMoney
,Money::plus()
only accepts anotherMoney
.Money
as it is, and add a subclass called something likeDefaultMoney
(hate the name). If you instantiate aMoney
with a default scale, it would return aDefaultMoney
; this way, if you write a function that expects aMoney
, it will accept aDefaultMoney
, but if it expects aDefaultMoney
, it will not accept aMoney
.Money
/BigMoney
classes as above, but have them implement a common interface. This is similar to point 1 because we would have 2 separate classes that do not extend each other, but similar to point 2 because we would have a fully shared interface so thatMoney::plus()
andBigMoney::plus()
will accept both aMoney
and aBigMoney
(is this necessary, after all?)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:
Money::plus()
andMoney::minus()
would not require a MoneyContext anymore, because the constant scale would guarantee that no rounding is necessaryMoney::multipliedBy()
andMoney::dividedBy()
would still require a rounding mode, but not a full MoneyContextMoney
using an integer, whileBigMoney
would still useBigDecimal
.I'm really looking forward to your opinion on this one, which I consider the most important issue of all.
The text was updated successfully, but these errors were encountered: