brick / money

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

Scales & contexts #7

Closed BenMorel closed 7 years ago

BenMorel commented 7 years ago

This is kind of a merger of other issues, in particular #3 (Scales and Money classes) and #4 (Money contexts), that happen to be tightly related.

I figured we've reached a point where we have a common view of a few things, and several conflicting ideas on others. I'll try to summarize the current status of the brainstorming.

I hope this issue will help us find some common ground.

What we (seem to) agree on

What we disagree on

Actually there is just one main friction point: operations. Should all operations require a full context (target scale, step, and rounding mode), or should a Money instance dictate what the result scale & step are?

I tried to summarize your points of view here, feel free to correct me if I'm wrong:

What others are doing

This is just an overview of what I could investigate in a reasonable timeframe. If you know other libraries that you think deserve to be mentioned, please let me know.

moneyphp (PHP)

This PHP library offers a single class, Money, that only stores amounts in integer form, so in "minor units". Currencies are only defined by a currency code. No scale is involved. Multiplication and division take an optional rounding mode, defaulting to HALF_UP.

Joda Money (Java)

This popular library offers two implementations, Money and BigMoney:

Java 9 Money API (JSR 354) (Java)

This is the new money interface that is now part of Java from version 9 onwards. Java 9 is due to be released tomorrow, 21 September 2017; you can hardly dream of a fresher API! It's been created by a team of experts, several of them working for Credit Suisse.

This API defines a MonetaryAmount interface that Money classes must implement. MonetaryAmount instances embed a MonetaryContext that defines "the numeric capabilities, e.g. the supported precision and maximal scale, as well as the common implementation flavor."

According to the source code documentation, operations like add(), multiply() and divide() take a single argument, the operand. The result's scale is adjusted just like a BigDecimal would do, but an exception can be thrown if the scale of the result exceeds the max scale defined by the context.

The reference implementation, Moneta, provides several classes:

I gave Moneta a try:

CurrencyUnit usDollar = Monetary.getCurrency("USD");
BigDecimal amount = new BigDecimal("100.00");
System.out.println(Money.of(amount, usDollar).divide(3));
System.out.println(FastMoney.of(amount, usDollar).divide(3));
INFO: Using custom MathContext: precision=256, roundingMode=HALF_EVEN
USD 33.33333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333333
USD 33.33333

These defaults are plain nonsense if you ask me.

Python Money Class (Python)

Monies have the scale of the number they were instantiated with. Adding two monies of different scales will return a Money with the maximum of the two scales. Dividing a Money will return a number with a fixed number of decimals, rounded:

>>> m = Money("100.0", "USD");
>>> m
USD 100.0
>>> m + Money("1.23", "USD");
USD 101.23
>>> m/3
USD 33.33333333333333333333333333

Ping @martinknor, @fortis and @fprochazka again, now is the time to make your voice heard before the first beta release!

VasekPurchart commented 7 years ago

Operations on a Money should be able to round by steps, to support cash increments of 1.00 units (step 100, CZK) or 0.05 units (step 5, CHF); steps for ISO currencies will not be hardcoded in the library, and will be left up to the developer

What will be the default step, if none is provided? Will it match the getAmountMinor()?

Actually there is just one main friction point: operations. Should all operations require a full context (target scale, step, and rounding mode), or should a Money instance dictate what the result scale & step are?

I want to adress a (maybe) misunderstanding of my main point - I didn't mean to require an explicit context for all operations at all times.

The main idea is that context should be tied to operations not values. The resulting implementation can have multiple apperances, I tried to draft some of them in https://github.com/brick/money/issues/4#issuecomment-330275566. But these were just the "backbone". So if I take the last example from there:

<?php

$rounding = new MathRounding(RoundingMode::UP);
$context = new RetainContext($rounding);

$money = Money::of('50', 'USD');

echo $context->divideBy($money, 3); // USD 16.67

I can easily write with the same "infrastructure" also:

<?php

$money = Money::of(50, 'USD'); // USD 50.00
$money = $money->dividedBy(3, RoundingMode::HALF_UP); // USD 16.67

This would just mean, that the Money::divideBy method would create a default context for division. And exactly the same with all the other methods.

The thing I like about this (and why I was proposing it is two things):

1) Any time you want to do something more non-trivial (where the defaults are not enough), you can transparently switch to calling the operations on the carefully constructed Context by your needs. 2) When you need to perform a series of operations, you are not losing money by default

Consider just simple division followed by another operation (working with mathematical rounding):

<?php

$money = Money::of(50, 'USD'); // USD 50.00
$money = $money->dividedBy(3); // USD 16.67
$money = $money->multiplyBy(3); // USD 50.01

Yey! We created some money!

What I wanted to achieve is to minimize the number of places where rounding takes place (if you do not require it explicitely):

<?php

$money = Money::of(50, 'USD'); // USD 50.00
$money = $money->dividedBy(3); // USD 16.66666666666666666666666666 (in an ideal world infinite)
$money = $money->multiplyBy(3); // USD 50.000000000 (there can be a 1 or something else somewhere, depending on the operation)
$money->getAmount(RoundingMode::HALF_UP, /* steps */); // USD 50

This means that you try to keep the precision the best you can (given we do not live in an ideal world) and do the rounding in the last step, when you actually want to do something with the money. This can be storing it in a DB, comparing it to something else etc. Meaning also you have best possible control about what you are getting at the same time, exactly at the place where you need to use it.

Of course the operation in the example is trivial (and easy to work around), but there are of course much more complicated procedures, where rounding in the middle would cause a big problem and the user might not really expect that. After all, you are trying to use a Money library to avoid the problems you would have using floats.

BenMorel commented 7 years ago

What will be the default step, if none is provided? Will it match the getAmountMinor()?

The default step will be 1, which means 0.01 for currencies with a scale of 2. This will be the same behaviour as if no steps were involved.

I didn't mean to require an explicit context for all operations at all times.

Ok :)

The main idea is that context should be tied to operations not values.

I got that, but if you want to provide operations on Money as well, then you do need some "default" context indeed.

So if I take the last example from there:

echo $context->divideBy($money, 3); // USD 16.67

I'd rather pass the context to the Money, then, it makes more sense to have operations in Money itself IMO, and apply the context on the result (which might be a rational internally, and then converted to a BigDecimal by the context); this is what we're already doing!

Consider just simple division followed by another operation (working with mathematical rounding): (...) Yey! We created some money!

It should be well understood that once you apply a rounding mode, some data might be lost. I don't see this as a problem as by default, an exception is thrown.

there are of course much more complicated procedures, where rounding in the middle would cause a big problem and the user might not really expect that

That's what I proposed in #4 (comment), a startCalculation() method where you can chain method calls, terminating with a getResult() method; rounding would be applied at the last step, and the result would be compatible (scale & step) with the original Money:

$money = Money::of('12.34', 'USD');
$money->startCalculation()
    ->divideBy(3)
    ->add(1)
    ->multiplyBy('1.123')
    ->getResult(RoundingMode::DOWN); // USD 5.23

So basically, you can think of built-in operations like dividedBy():

$money->dividedBy(3, RoundingMode::DOWN);

as a shortcut for:

$money->startCalculation()
    ->divideBy(3)
    ->getResult(RoundingMode::DOWN);

But IMO the "shortcut" dividedBy() operation is very useful, when you have a single operation to perform. You should be aware that you should not chain such operations, applying the rounding mode twice. The doc will be clear about that.

BenMorel commented 7 years ago

So after several attempts, here is what I came up with:

https://github.com/brick/money/tree/adjustment

Highlights

Some internal details

The equivalent Adjustments for existing MoneyContexts

Why the MoneyContext name change?

I think Adjustment better conveys the meaning of what it does: it adjusts a rational operation result to make it fit a given scale & step. Because a RoundingMode constant can usually be passed in place of an Adjustment, the term adjustment also better applies to a rounding mode than context does.

Constructor parameters for Adjustments

This will help you understand the examples below:

Examples

    $money = Money::of(10, 'USD'); // USD 10.00
    $money->dividedBy(2); // USD 5.00
    $money->dividedBy(3, RoundingMode::DOWN); // USD 3.33
    $money->dividedBy(32, new ExactResult()); // USD 0.3125
    $money->dividedBy(8, new CustomScale(6)); // USD 1.250000
    $money->dividedBy(3, new CustomScale(2, 5, RoundingMode::UP)); // USD 3.35

    $cashAdjustment = new CustomScale(2, 100);
    $money = Money::of(50, 'CZK', $cashAdjustment); // CZK 50.00
    $money->dividedBy(3, RoundingMode::UP); // CZK 17.00
    $money->dividedBy(3, new DefaultScale(RoundingMode::UP)); // CZK 16.67
    $money->multipliedBy('1.234567', new ExactResult()); // CZK 61.72835

What do you think?

I think this covers most of our needs. It balances ease of use, sensible defaults, with the possibility to perform more advanced operations.

I tried various approaches, including a more formal one where Money would store a Context object, but it was not as convenient to use or easy to understand, and it would have potentially allowed for changing the default behaviour of operations, such as dynamically changing the scale on every operation, or providing a default rounding mode. I think this is opening a can of worms and want to avoid this at all costs: IMO, it's much more solid to have a default behaviour that does not depend on the Money's context, other than its capabilities (scale & step).


A few notes:

Ping @jkuchar and @jiripudil !

jiripudil commented 7 years ago

Hello there, sorry for not responding, I've had a busy week. I've caught up with all the comments and they have changed my point of view a bit. Let me build up on the discussion from #3:

what should we do when doing plus(1), plus('1.2')

Well, you've got me there. Admittedly, my first thought was to convert the value to the money's context, which effectively contradicts to my own arguments about strictness. My second thought was to restrict the input type to Money instances, which was when I've realized I might be basing my opinions more on academic rather than practical values.

I was probably thinking we could get rid of the MoneyContext entirely, keep it embedded in the Money and make it really strict about it. I wouldn't mind writing more code if it was the price for Money having my back covered, but in the end, it would complicate things for the newcomers (which goes against the goal outlined in #4) and likely add a level of complexity to the everyday usage as well.

What do you think?

Love it!

I'm still not 100% convinced that the step belongs to Money, or should only be part of the operations

I'm inclined to agree with the former. As I've argued in https://github.com/brick/money/issues/3#issuecomment-330338136, you apply the step to the money only after you're sure it's cash, and then you want to make sure it stays that way unless explicitly told otherwise.

To push the reflection even further, we can wonder whether the default adjustment should be DefaultScale instead

I'm afraid it would lead to a lot more code if you need something else than the default scale, and eventually create some WTF moments. Consider this:

$adjustment = new CustomScale(4, 1, RoundingMode::HALF_EVEN);
$money = Money::of('1.2345', 'USD', $adjustment);

// I need to pass the adjustment everywhere, but I can live with it
$money = $money->plus('2.2', $adjustment); // -> USD 3,4345

// now, I just need different rounding mode for this next computation...

// oh no, I forgot it creates DefaultScale adjustment if I don't provide one explicitly:
// the operation passes, but I've lost precision :(
$money->dividedBy('4', RoundingMode::UP); // -> USD 0.86

// the method expects 2 parameters; I cannot reuse the $adjustment I have and just override the rounding mode:
$money->dividedBy('4', $adjustment, RoundingMode::UP); // -> Error

// so I end up creating a new adjustment with the desired rounding mode:
$money->dividedBy('4', new CustomScale(4, 1, RoundingMode::UP)); // USD 0,8586

I think this is getting close to – if not beyond – what you initially wanted to avoid in #4

BenMorel commented 7 years ago

Thanks for your review! I believe indeed that the default-scale-by-default would do more harm that good. I'm happy that you like the current implementation; I'm currently investigating yet another approach, based on embedded contexts, that would work pretty much like the current one, only cleaner and that could solve another problem or two. I may have overlooked a point when I said a more formal, embedded context approach had major drawbacks. I'll make a post again soon if it actually leads somewhere.

jkuchar commented 7 years ago

I was quite busy these days, I'm sorry for not responding in more reasonable time.

I'm still not 100% convinced that the step belongs to Money, or should only be part of the operations.

πŸ‘ for @jiripudil 's "you apply the step to the money only after you're sure it's cash, and then you want to make sure it stays that way unless explicitly told otherwise".

Let's call constructor, you expect cash but someone gives you value 1.1235 CZK.

Money::of('1.12345', 2, 100, 'czk'); // throws exception, does not match step

To break the code above up, there two parts of the input:


I like rename of MoneyContext to Adjustment, it makes things much more clear.


Look at the following code

$cashAdjustment = new CustomScale(2, 100);
$money = Money::of(50, 'CZK', $cashAdjustment); // CZK 50.00 (scale=2, step=100)
$money->dividedBy(3, RoundingMode::UP); // CZK 17.00 (scale=2, step=100)
$money->dividedBy(3, new DefaultScale(RoundingMode::UP)); // CZK 16.67 (scale=2, step=1)
$money->multipliedBy('1.234567', new ExactResult()); // CZK 61.72835 (scale=5, step=1)

Particularly at new DefaultScale(RoundingMode::UP), when passed to any operation (constructor including), the resulting money is keeping scale and step. Rounding is performed and thrown away. I I definitely agree with this hehaviour, but not with the API. API does not reveal the intention.

I would think about Audjustment as "one thing", however there are two parts - one is persistent (scale and step) and one is just for lifespan of current operation.

$money = Money::of(50, 'CZK', $cashAdjustment);
/*1*/ $money->dividedBy(3, new DefaultScale(RoundingMode::UP)); // CZK 16.67 (scale=2, step=1)
/*2*/ $money->dividedBy(3, RoundingMode::UP, new DefaultScale()); // CZK 16.67 (scale=2, step=1)

What about splitting rounding and Adjustments, as they have different lifecycle? (more on lifecycle bellow)


From life-cycle point of view, it looks like there are three different life-cycles involved:

This would mean, only place one can change precision is the currency, step will be configurable per money instance, rounding per operation.

Question: Is there any real world use-case to have more precise results then real currency scale? (you can always use rationals beeing temporarily "out of scale")

$czk = Currency::of('czk');
cont CASH_CZK = 100;
$money = Money::of('1.234', $czk); // invalid
$moneyCashless = Money::of('1.23', $czk); // valid
$money = Money::of('1.23', $czk, CASH_CZK); // invalid
$moneyCash = Money::of('1.00', $czk, CASH_CZK); // valid
assert($moneyCashless->getCurrency()->is($czk)); // pass
assert($moneyCash->getCurrency()->is($czk)); // pass

$divided = Money::of('10.00', $czk)->divide(3, new CustomScale(5), Rounding::HALF_UP); // 3.33334
assert($divided ->getCurrency()->is($czk)); // fail as it is invalid amount in CZK

Look at the code of CustomScale, it does not care about currency at all.

If someone want to do mathematical stuff with money (and then they usually do not care much about the currency), they can simply do getAmount() as BigDecimal and then do whatever they want and then convert back to Money. Money should be great for simple arithmetic as that is what money is all about. I would keep things simple.


Do we really need context at operations?

I would always use something like code bellow whereever possible, as I can better imagine what is happenning:

$money->toScale(25)->divide(5, HALF_UP); // this is simplier
$money->divide(5, HALF_UP, new CustomScale(25)); // this I would need to read docs to know that returned object will have scale=25

As I want to have least possible knowledge about currency in code, in code bellow I need to know scale and step. In most cases I'm OK with default scale and I just want to have cash variant of money. What about adding constructor which will just set step?

$cashAdjustment = DefaultScale::withStep(100);
$money = Money::of(50, 'CZK', $cashAdjustment); // CZK 50.00 (scale=2, step=100)

Do we really need to change step so often? Aren't there usually fixed number of money representations we will be working with? I can imagine following lines in my app configuration somwhere:

$czkCash = new Representation(100, Currency::of('czk'));
$czkCashless = new Representation(1, Currency::of('czk'));
$eurCash = new Representation(100, Currency::of('eur'));
$eurCashless = new Representation(1, Currency::of('eur'));

Surely most convenient way would be to have something like this:

$money = Money::of('1.12', Currency::of('czk'), Representation::CASHLESS);
$money = Money::of('1.12', Representation::cashless(Currency::of('czk')));

Bad is that Representation or Money would need to know scales for all currencies used inside the app, otherwise it can end up in error. Maybe simple Representation where someone just provides scale is good-enough for now.


I'm still thinking of when I use Money as a domain-value object, how would I code following business rules in?

I'm not sure where the boundary should go. What should be in money and what should be outside (e.g. in wrapping domain object).

This is more general approach to what step is doing. Solution can be to provide object which can decide which values are allowed and which not, what is previous and what is next value. Then there can be any domain restrictions applied to given variable.

/**
 * Represents mathematical domain (values that can given variable contain)
 */
interface DomainValidator
{
   function getClosestValue(DOWN|CLOSEST|UP): ?BigNumber; // todo: is this enough to implement rounding?
}

$money = Money::of('200.00', Currency::of('czk'), $myCrazyDomain);
$money->plus('88', HALF_UP); // 300.00 CZK

$money->minus('99999999', HALF_UP); // 200 CZK
$money->plus('99999999', HALF_UP); // 5 000 CZK

However I'm really not sure if this should go inside money itself. This makes more sense to me, if have that functionality at all, to have it inside brick\math (for BigInteger and BigDecimal). And to be honest, there will probably only few people who will need this and even less people that will know how to use this.

However it does not make interface more complicated for classic simple use cases:

$czkCash = new CashDomain(Currency::of('czk'), $minimalCashAmount = 100);
$money = Money::of('200.00', $czkCash);

It feels to me like an very immature idea. We will see if it leads us into something useful. (maybe separate issue?)


startCalculation(): rational computations

There should be underlaing method toRational(), which will simply return RationalMoney.

Types involved in startCalculation(). To make sure that there will be proper operation parameters available (e.g. no rounding), we should use separate object for this. This object can be just enclosing RationalMoney, only difference is that is knows restrictions of original Money so it can be simplier converted back (just providing RoundingMode).

$money->startCalculation() // from now working with RationalMoney wrapper
    ->divideBy(3)
    ->add(1)
    ->multiplyBy('1.123')
    ->getResult(RoundingMode::DOWN); // USD 5.23; converted back to Money object
BenMorel commented 7 years ago

Thanks for your review. Your thoughts are actually going in pretty much the same direction as the branch I'm working on (more on this tomorrow, in another post), so this is great news as I think we're finally getting somewhere.

πŸ‘ for @jiripudil 's "you apply the step to the money only after you're sure it's cash, and then you want to make sure it stays that way unless explicitly told otherwise".

Agreed now. My doubts have pretty much vanished.

What about splitting rounding and Adjustments, as they have different lifecycle? (more on lifecycle bellow)

I had the exact same feeling: rounding mode just did not belong here. It's now explicitly provided in every operation, and removed from Adjustment/Context. :+1:

Question: Is there any real world use-case to have more precise results then real currency scale? (you can always use rationals beeing temporarily "out of scale")

There are a few use cases for this: Forex trading, currency exchange platforms, etc.

If someone want to do mathematical stuff with money (and then they usually do not care much about the currency), they can simply do getAmount() as BigDecimal and then do whatever they want and then convert back to Money.

I may have a solution to make everyone happy. More on that tomorrow.

Do we really need context at operations? (...) Do we really need to change step so often?

I don't think so either. In the new branch, operations have no context, just a rounding mode.

I'm still thinking of when I use Money as a domain-value object, how would I code following business rules in? (...) We will see if it leads us into something useful. (maybe separate issue?)

Definitely a separate issue. This is outside the boundary of what I want to have in the first release ;)

There should be underlaing method toRational()

Already there in the new branch :)

This object can be just enclosing RationalMoney, only difference is that is knows restrictions of original Money so it can be simplier converted back (just providing RoundingMode).

I was not sure about this one. For now toRational() creates a generic (without context) RationalMoney; you have to provide the context again in the last operation, but maybe it's annoying indeed.


I'll do another post tomorrow with the details of the new version. I hope we're getting close to the goal now! πŸ‘

BenMorel commented 7 years ago

Here we are, the embedded contexts did actually lead somewhere interesting! I had overlooked this approach in the past, based on the assumption that contexts contained a rounding mode, and thus would allow for a default rounding in operations (which I wanted to avoid at all costs); but they didn't have to.

The code is available at:

https://github.com/brick/money/tree/embedded-context

Contexts

First of all I'm afraid that I renamed Adjustment back to Context :-)

So basically, now Money does not contain a $step property, but a $context property; this maps to a Context instance that may, or may not, contain a step. Contexts are responsible for adjusting a rational operation result to a decimal representation.

Available contexts are:

Contexts do not contain a rounding mode anymore; rounding modes are provided explicitly when calling operations.

Operations

Operations now only accept a RoundingMode constant as a second parameter:

public function plus($that, $roundingMode);
public function multipliedBy($that, $roundingMode);

All operations return a Money with the same Context as the original Money:

$money = Money::of(50, 'USD'); // USD 50.00, DefaultContext
$money->multipliedBy(3); // USD 150.00, DefaultContext
$money->dividedBy(3, RoundingMode::DOWN); // USD 16.66, DefaultContext

$cashCZK = new CashContext(100);
$money = Money::of(50, 'CZK', $cashCZK); // CZK 50.00, CashContext(100)
$money->dividedBy(3, RoundingMode::DOWN); // CZK 16.00, CashContext(100)

$cashCHF = new CashContext(5);
$money = Money::of(50, 'CHF', $cashCHF); // CHF 50.00, CashContext(5)
$money->dividedBy(3, RoundingMode::DOWN); // CHF 16.65, CashContext(5)

$highPrecision = new PrecisionContext(4);
$money = Money::of(50, 'USD', $highPrecision); // USD 50.0000, PrecisionContext(4)
$money->dividedBy(3, RoundingMode::DOWN); // USD 16.6666, PrecisionContext(4)

$exact = new ExactContext();
$money = Money::of(50, 'USD', $exact); // USD 50, ExactContext
$money->dividedBy(4); // USD 12.5, ExactContext
$money->plus('1.234'); // USD 51.234, ExactContext

Adding/subtracting monies

As requested, operations between monies in different contexts now throw an exception:

$money = Money::of(50, 'CZK'); // DefaultContext
$cashMoney = Money::of(30, 'CZK', new CashContext(100));

$money->plus($cashMoney); // MoneyMistmatchException

The exception message suggests what to do if you want to proceed anyway:

MoneyMistmatchException: The monies do not share the same context. If this is intended, use plus($money->getAmount()) instead of plus($money).

I thought that I could just allow another Money with a different Context if a rounding mode is provided. But in the example above, RoundingMode::UNNECESSARY would be enough, and yet we want an exception by default. Comments welcome.

What if...?

What if I want to perform an operation, and get a Money with a different context as a result?

There are a couple options there:

This is quite an edge case anyway, so any kind of support is good enough IMO.

Final concerns

There has to be a few things that I don't like, obviously, but I think this time they're minor issues:

A few notes

What do you think?

I'm personally quite happy with this approach now, which I think is candidate for a release. I'm looking forward to your comments!

jkuchar commented 7 years ago

I think that we should just document the fact that a Context must follow the rounding mode requested by the operation.

:+1: Documenting behaviour in comments is equally strong as requiring them in statically checked part of the interface. Not everything can be checked by type checking.

there is actually one small "infringement" of this rule: ExactContext.

It kind of make sense, as it changes scale of Money to whatever scale it needs. Then any rounding actually does nothing. So yep, rounding unnecessary makes sense. As I would think that rounding happens after scale conversion, it makes sense.

ExactContext is kind of "special", because it does not keep scale of original money. However I do not see anything wrong with this.

I'm personally quite happy with this approach now, which I think is candidate for a release.

I feel it the same way. :-) I have went through code and I have no more comments. It looks great.

BenMorel commented 7 years ago

Excellent, thanks for your feedback!

I just drafted the docs here: https://github.com/brick/money

I may still have one or two design decisions pending; if I don't feel like resolving them alone, I will open new issues and let you know here!

BenMorel commented 7 years ago

Version 0.1.0 released! See the announcement.