Skip to content
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

Money contexts #4

Closed
BenMorel opened this issue Sep 14, 2017 · 16 comments
Closed

Money contexts #4

BenMorel opened this issue Sep 14, 2017 · 16 comments

Comments

@BenMorel
Copy link
Member

BenMorel commented Sep 14, 2017

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

Monies can have different scales: for example USD uses 2 decimals by default, but some apps may require additional precision and be able to represent 1.123456 USD. Additionally, some arithmetic operation on monies may require rounding: for example dividing 1.00 USD by 3.

When performing operations on monies, a MoneyContext controls the scale and the rounding. The available implementations are:

  • DefaultContext: adjusts the scale to the currency's default fraction digits (2 for EUR/USD/GBP/…, 0 for JPY), using rounding if necessary.
  • FixedContext: adjusts the scale to a given value, using rounding if necessary.
  • RetainContext: uses the scale of the left operand. This is the default context for arithmetic operations: plus(), minus(), multipliedBy(), dividedBy(). For example, 0.1234 USD + 1.00 USD equals 1.1234 USD, while 1.00 USD + 0.1234 USD uses rounding to fit the result in 2 decimals.
  • ExactContext: tries to adjust the scale to fit the result. For example, 0.99 USD / 2 would equal 0.495 USD, while 1.00 USD / 3 would throw an exception.

The first 3 contexts require a MoneyRounding implementation:

  • MathRounding uses brick/math's rounding modes directly
  • CashRounding uses rounding modes and steps, for example every 5 cents: 0.05, 0.10, 0.15, etc.

Contexts are very flexible and powerful, but it's a lot to digest when you're new to the library; even when you're comfortable with them, they're currently a pain to deal with.

For example, when dividing a money, you will most probably need rounding (unless you're really lucky that the results fits in the scale of your Money).

To use rounding, you need to pass a MoneyContext:

use Brick\Math\RoundingMode;
use Brick\Money\MoneyRounding\MathRounding;
use Brick\Money\MoneyContext\RetainContext;
use Brick\Money\Money;

$rounding = new MathRounding(RoundingMode::UP);
$context = new RetainContext($rounding);
echo Money::of('10.00', 'USD')->dividedBy(3, $context); // 3.34

This is too long. We need a shortcut for these simple use cases that we use daily.

Maybe these methods should accept not only a MoneyContext , but also a MoneyRounding, and even a RoundingMode constant directly.

This means that all three statements below would be equivalent:

$money->dividedBy(3, RoundingMode::UP);
$money->dividedBy(3, new MathRounding(RoundingMode::UP));
$money->dividedBy(3, new RetainContext(new MathRounding(RoundingMode::UP)));

So our example above could be simplified to:

use Brick\Math\RoundingMode;
use Brick\Money\Money;

echo Money::of('10.00', 'USD')->dividedBy(3, RoundingMode::UP); // 3.34

We may then argue that the constructors of the MoneyContext classes that accept a MoneyRounding should also accept a RoundingMode constant directly for convenience.

I hope that this could make it easy to use and less verbose for everyday use cases, while still allowing for more complex examples.

Thoughts on this? Alternative ideas? Are you happy with the overall context thing?

Note that depending on the implementation chosen in #3 Scales and Money classes, using contexts could be avoided entirely for standard (default scale) monies, which would only use rounding modes.

This was referenced Sep 14, 2017
@BenMorel
Copy link
Member Author

As contexts are quite complicated to work with (steep learning curve and verbose code), another idea could be to draw on Joda Money and create more than one method per operation.

For multiplication, they have:

  • multipliedBy() that returns a money whose scale is the sum of the scales of the two operands (similar to BigDecimal) — this is equivalent to our ExactContext
  • multiplyRetainScale() that retains the scale of the left operand, and uses rounding — this is equivalent to our RetainContext

This could pretty much replace the concept of contexts. Sure, you cannot do things like multiply to a given scale, but even this quite unusual requirement could be achieved by using something like ->toScale(...)->multiplyRetainScale(...).

@jkuchar
Copy link

jkuchar commented Sep 16, 2017

What about moving MoneyContext into currency, more on this idea in #3.

@BenMorel
Copy link
Member Author

Although interesting (I had never thought of that), I'm not really comfortable with the idea of moving the context to the Currency:

  • first of all, I believe that a single Currency instance should exist within a given project, for a given currency code. I see the currency as something static.

  • my other concern is that in effect, it embeds the context into the Money class. So you write a function that accepts a Money. The Money instance you receive has a Currency and therefore a Context, and effectively dictates how it's supposed to be scaled and rounded. If your function needs to perform something else, what do you do? Would you have to create another Money from this one, with another Currency, just to be able to apply a custom rounding?

    function addVat(Money $amount)
    {
        $context = (... some context with RoundingMode::HALF_EVEN ...);
        $currency = new Currency('USD', 2, $context, ...);
        return $amount->withCurrency($currency)->multipliedBy($this->vatRate);
    }

    It feels really counter-intuitive. I want to be able to at least do something as simple as:

    function addVat(Money $amount)
    {
       return $amount->multipliedBy($this->vatRate, RoundingMode::HALF_EVEN);
    }

    And IMO, the multipliedBy() method should have a well-defined default behaviour when called without a second parameter (such as retain scale, exception if rounding required), a default behaviour that does not depend on some internal context. I think that anything other would be opening a big can of worms.

@jkuchar
Copy link

jkuchar commented Sep 17, 2017

first of all, I believe that a single Currency instance should exist within a given project, for a given currency code. I see the currency as something static.

It is static in real world, lets keep it static also in this model. It will feel more natural. It looks like we are missing some concept that allows us to distinguish between cash and cashless versions. (more in #3 )

therefore a Context, and effectively dictates how it's supposed to be scaled and rounded

I didn't thought about rounding at all. I agree that default rounding behaviour must not depend on some internal context. I just want to make possible to create Money what will respect minimal step, as you wrote about it in #3.

@VasekPurchart
Copy link

VasekPurchart commented Sep 18, 2017

Hello, I see I am quite late to the party and I don't know if this fits more to this thread or #3, but here are my thoughts.

I like the idea of contexts, but I think the contexts are applicable to operations, whereas in #3, you seem to be getting to the point where you want to assign the context in "constructor" (Money::of).

My reasoning behind this is that I think some of the problems we are discussing stem from the fact, that there are two concepts intertwined together - first is the money representation (value) and then there are the operations.

From my experience I think you want the value to be as precise as possible, especially when money is concerned, because then you don't "lose" anything. And since this is "static", the context is not really needed (I mean rounding context etc.).

On the other hand, when operations are concerned, there might be some limitations, as mentioned in the contexts list at the top of this issue. Every usecase has its own specifics and you can even want to combine a few of them. I mean you may want to do an arithmetic operation and want to fail, if the contexts (scales) do not match, or you may want to do a few steps and then check the value at the end, if it matches. In the second case this falls under the "operations need context" too, because actually you want to specify the context for the "getResult" method. Here it can throw an exception if it is not a context, that should be "final".

Even if a currency has the precision of 0.01, there are cases where you may want to be "more precise" during a series of computation. If you have a total, which is at the correct scale, but for example have to divide it evenly to some items (order items), you need to be precise in the algorithm, otherwise 1000 / 3 would not equal 1000 again when put together.

The thing is, I am not sure, if you are able to "correctly" assign context to the operations. I mean there can be sensible default, but it would obviously not fit every usecase. For example I find the RetainContext (which you mentioned as default for the basic arithmetic operations) as quite confusing, because it works differently for A + B vs B + A. And I think this could happen a lot, if you deal with data, that comes from creating the Money instances based on user data (meaning not something produced by the system directly). For me using 0.1234 USD + 1 USD should have always the same result, no matter what. And there are two scenarios which adhere to this: either throw exception when the scales do not match, or take the larger one (always 1.1234 USD), because then later in the "getResult" the default can be DefaultContext which would throw an exception if the "current" scale was bigger than the prescribed one (but you would be able to override it again with a custom one).


The ideas about values vs operations could be taken even further, maybe they should be "separate"? I mean it is surely comfortable to have plus() straight on Money, but when it comes to advanced usecases, it can easily get in the way, as is illustrated I think in #3, where you were discussing how to simplify the context parameters. I ways playing with this idea when reading that - what if the operations were on a separate object - a "service"? Which comes natural to me, because usually when there are concerns, which cannot be encapsulated by a given object (because they rely on something which is not its concern), you need something outside. So something like:

<?php

$rounding = new MathRounding(RoundingMode::UP);
$context = new RetainContext($rounding);
$operations = new MoneyOperations($context); // the name is not a keeper :)

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

echo $operations->divideBy($money, 3); // 3.34


// OR

$rounding = new MathRounding(RoundingMode::UP);
$context = new RetainContext($rounding);
$operations = new MoneyOperations(); // the name is not a keeper :)

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

echo $operations->divideBy($money, 3, $context); // 3.34

And then my next step in thinking was whether the operations could be part of the contexts themselves, because the behavior depends on them tigtly? (maybe even some operations even do not make sense in some contexts?)

<?php

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

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

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

I think this fits nicely together in the aspect which data is assigned where and could be even really comfortable, because you could have a factory (as you mention in #3 (comment)), which you could have configured by your DI to directly return an already appropriately configured instance of Context.

The "default" usecases would not be hindered by this either I think, because you could easily keep the methods on Money and the default would be constructed inside, as it is now. Or you could even allow to pass the context as a parameter either and then just delegate to the context (this would be I guess only up to the decision if it is good to have two very similar ways to achieve same thing).

To sum this up, this is keeping the context with the operations, not with the Money instance itself


Maybe these methods should accept not only a MoneyContext , but also a MoneyRounding, and even a RoundingMode constant directly.

I am not a big fan of methods, which accept multiple types, in the long term they always bring some headaches, work poorly with static analysis tools etc., so I am definately for making multiple named methods for them (like multiplyByWithScale()).


Btw I have only read #3 and #4 so far, will check back when I read the rest, if this makes sense, but had to write it down, before I would forget my thoughts.

@BenMorel
Copy link
Member Author

Hello, I see I am quite late to the party

Better late than never, welcome to the party! Interesting and challenging thoughts, thanks for sharing them.

First I want to clarify what we call the context, as there have been conflicting views so far:

  • in the current implementation, it's scale and rounding. Every operation defines what the scale of the result is, and the rounding mode used to fit this scale, if necessary;
  • what's being discussed in Scales and Money classes #3 is indeed to embed a context comprised of scale and step (1 cent, 2 cents, 5 cents, etc.) in Money, but definitely not the rounding mode. The creator of a Money instance defines what its capabilities are (scale, and optional step for cash amounts), and every operation returns a similar Money; the caller is in control of rounding only.

I think some of the problems we are discussing stem from the fact, that there are two concepts intertwined together - first is the money representation (value) and then there are the operations.

This is true. If Money only had to be an anemic value object with no operations, an amount (any scale) and a currency would be all we need.

From my experience I think you want the value to be as precise as possible, especially when money is concerned, because then you don't "lose" anything. And since this is "static", the context is not really needed (I mean rounding context etc.).

I believe this would be a job for an external calculator implementation indeed; this has been briefly brought up in #3. This calculator would be based on BigRational so that you don't have to provide any context until you actually want to get the result as a Money. This is not incompatible with current discussions and I have in mind to provide one at some point (@jkuchar said he has already built and used something similar, so there are definitely use cases for this).

The thing is, I am not sure, if you are able to "correctly" assign context to the operations. I mean there can be sensible default, but it would obviously not fit every usecase.

That's what I want to provide in Money itself: sensible defaults for the everyday life use-cases that I work with in my projects. Very often I don't need complex calculations, but just say, calculate a VAT:

    $priceBeforeTax = Money::of('12.33', 'EUR');
    $vatRate = '0.2';

    $vat = $price->multipliedBy($vatRate, RoundingMode::DOWN);
    $price = $priceBeforeTax->plus($vat);

While I think it's important to provide advanced tools for advanced use cases, IMO the utmost priority is to keep the use cases that we use 90% of the time easy to write, read and understand.

For example I find the RetainContext (which you mentioned as default for the basic arithmetic operations) as quite confusing, because it works differently for A + B vs B + A.

One could argue indeed that commutativity is broken, but not really: the scale of the result might be different, but the value is the same (isEqualTo() would return true): USD 1.00 + USD 1.2000 = USD 2.20, while USD 1.2000 + USD 1.00 = USD 2.2000. If the result cannot fit in the current scale, an exception is thrown, and only providing a rounding mode can make the 2 results differ. And as soon as you provide a rounding mode, you should expect commutativity to be broken.

The ideas about values vs operations could be taken even further, maybe they should be "separate"? I mean it is surely comfortable to have plus() straight on Money, but when it comes to advanced usecases, it can easily get in the way (...) what if the operations were on a separate object - a "service"?

I think the two ideas are not incompatible: as I said above, I definitely want to have operations on Money itself; failure to do so will make the library hardly usable for common use cases, where you'd get lost in details for the simplest thing like adding two monies.

I would, however, definitely consider providing an external, BigRational-based calculator implementation.

To sum this up, this is keeping the context with the operations, not with the Money instance itself

It's an interesting idea and it definitely makes sense in theory, but as you've seen in the first post of this issue, I find allowing contexts in every operation quite cumbersome, and more importantly, I can't seem to find a use case for this!

I'm really curious to see a single use case where you start with a scale, perform an operation, and want the result in another scale. Say you have USD 21.34, want to divide it by 3, and suddenly want the result to be USD 7.1 or USD 7.113333? My opinion is that if you wanted the result in another scale, your original money would have had this scale from the very beginning. And if you wanted a high precision result as an intermediate step for further calculations, then a BigRational-based calculator is what you need.

A real-life use case is an online shop, with products and a default scale price: EUR 19.90. All calculations that I will ever perform when adding products to the cart, multiplying the quantity, adding shipping fees and taxes, should end up with the same scale.

Another real-life use case is a Forex trading website: I decide that I want to represent the EUR/USD pair with 4 decimals, so I choose a scale of 4 and start with USD 1.2345. Why would I want to suddenly change the scale for something else? Ok, why not: let's imagine I make a trade, buying 11,250 EUR at USD 1.2345, and selling it later at USD 1.2400. My profit would most probably be calculated in scale 4: (USD 1.2400 - USD 1.2345) * 11250 = 61.8750 USD. Now there could be a need to add this profit to the customer's balance in the default currency scale (USD 61.87, rounding down), but then it could be as simple as an explicit toScale() call or something:

$buy = Money::of('1.2345', 'USD');
$sell = Money::of('1.2400', 'USD');
$qty = '11250';

$profit = $sell->minus($buy)->multipliedBy($qty); // USD 61.8750
$balanceChange = $profit->toDefaultScale(RoundingMode::DOWN); // USD 61.87

I think it's important to start with the use cases, and work backwards to the API. I think I've made a mistake trying to accommodate too many fictional use cases so far, neglecting the actual use cases. I'm trying to avoid making the same mistake for the release!

I am not a big fan of methods, which accept multiple types, in the long term they always bring some headaches, work poorly with static analysis tools etc., so I am definately for making multiple named methods for them (like multiplyByWithScale()).

I'm on the same page (I love being able to type-hint my scalar parameters and my return types since PHP 7!) and would put a type hint on every parameter if I could. But I have to be pragmatic here, take for example Money::of(), that accepts a BigNumber, an int, a float or a string as amount, and a Currency or a string as second parameter.

If I wanted to provide a method for every type, I would end up with 8 methods:

function ofBigNumberAndCurrency(BigNumber $amount, Currency $currency);
function ofBigNumberAndCurrencyCode(BigNumber $amount, string $currencyCode);
function ofIntAndCurrency(int $amount, Currency $currency);
...

Or the cleaner approach would be to only accept BigNumber and Currency:

function of(BigNumber $amount, Currency $currency);

But then the simplest use cases become verbose:

$money = Money::of(BigDecimal::of('12.34'), Currency::of('USD'));

And one could argue that BigDecimal::of() should be split into ofString(), ofInt() and ofFloat().


Sorry this was long, looking forward to your reply, I'm very interested in potential use cases that I would've overlooked!

@VasekPurchart
Copy link

what's being discussed in #3 is indeed to embed a context comprised of scale and step (1 cent, 2 cents, 5 cents, etc.) in Money, but definitely not the rounding mode. The creator of a Money instance defines what its capabilities are (scale, and optional step for cash amounts), and every operation returns a similar Money; the caller is in control of rounding only.

Yes, I have read through that, what I am trying to convey here is that I think that as little as possible should be tied with the actual instance of the "value", because I think there there is no intrinsic relation of the Money and even the scale and step. These are all part of the operation - even if the operation is only "getting the result", because you can have different cases for what is the result supposed to be - cash vs cashless vs arbitrary.

That's what I want to provide in Money itself: sensible defaults for the everyday life use-cases that I work with in my projects.

I was in no way suggesting that there should be no such defaults, or trying to remove the convenience methods, I am just trying to say, that these should not be at the expense of control and more special usecases. This is including loosing strictness (by allowing for example passing strings instead of strictly defined types etc). I was only trying to brainstorm a solution which would be designed for the control, but could have the convenience on top of that, by specifying the defaults.

Btw I think authors overestimate the need for "optimized for writing" in the sense that it is a problem to create instance of something and pass a value to that. I think this is in a way lingering feeling from the past, because not so many used IDEs. Nowadays it is sometimes much more comfortable to have types strictly required everywhere, because your IDE will guide you.

I'm on the same page (I love being able to type-hint my scalar parameters and my return types since PHP 7!) and would put a type hint on every parameter if I could. But I have to be pragmatic here, take for example Money::of(), that accepts a BigNumber, an int, a float or a string as amount, and a Currency or a string as second parameter.

Had started to write examples almost exactly the same as yours and I think that of course, the combination of all methods is impractical (and even if there would be more parameters). But I think this should be solved exactly with the Money::of(BigDecimal::of('12.34'), Currency::of('USD')); approach. Yes, it is more verbose, but for me, its verbose for writing this example, but actually more readable fro reading. And you should end up doing much more reading with your code than writing it :) On top of that I think these examples are kind of unnatural (at least for code where I was using money-like objects and operations). As I mentioned in #5, a lot of time the "context" values would come as variables and most of the time I think this goes for values as well. And then the same thing applies - you want to have these converted to the instance of something as soon as possible in the flow.

Are you receiving this via an API, then going trough controlled and using in a service to do a computation on it? I think the values should be converted while deserializing/validating the values in the API. Which means you already should have a BigDecimal defined here. And with the currency either the same, or its not given by the user and it comes from the configuration and then again, you want to convert as early as possible. This leads to code, which is both much more readable, and in the end more effective and less complex, because you will encounter the possible errors much sooner and can report them back directly. So for me the actual example near the logic would be more like:

<?php

function getFinalPrice(Money $priceBeforeTax)
{
    // the context would be configuer based on needed granularity
    $vat = $price->multipliedBy($this->vatRate, $this->context);
    return $priceBeforeTax->plus($vat);
}

I came to the same realization when I was writing documentation for the Enums. The Enums were actually being used on several projects and for years before, but when I was writing the docs, it seemed weird - then I realized, that that is because the examples are nowhere close to the usage in a real app, because you have to put everything on a few lines, what would mostly, in a good written app, be spread out in a few different layers.

One could argue indeed that commutativity is broken, but not really: the scale of the result might be different, but the value is the same (isEqualTo() would return true): USD 1.00 + USD 1.2000 = USD 2.20, while USD 1.2000 + USD 1.00 = USD 2.2000. If the result cannot fit in the current scale, an exception is thrown, and only providing a rounding mode can make the 2 results differ. And as soon as you provide a rounding mode, you should expect commutativity to be broken.

On the point of different scales by default in #3 (comment). This applies also to the commutativity - i think its misleading to think that it is clear, which instance is "the first". So the scales should really either match or be "suppressed" by providing appropriate context which would handle them differently. This also points me back to my aforementioned solution where steps would be defined strictly for operations, because no matter how many operands there are for the operation, these would be united by the operation context.

It's an interesting idea and it definitely makes sense in theory, but as you've seen in the first post of this issue, I find allowing contexts in every operation quite cumbersome, and more importantly, I can't seem to find a use case for this!

I still think this can be worked out by multiple methods, e.g. keeping the of "magical" (able to digest more types etc.), but also allowing to specify the needed parameters.

I'm really curious to see a single use case where you start with a scale, perform an operation, and want the result in another scale. Say you have USD 21.34, want to divide it by 3, and suddenly want the result to be USD 7.1 or USD 7.113333? My opinion is that if you wanted the result in another scale, your original money would have had this scale from the very beginning. And if you wanted a high precision result as an intermediate step for further calculations, then a BigRational-based calculator is what you need.

I had a similar problem to this. Its some time ago, so I don't remember the particulars, but we were representing the given currency in credits. These credits were tied to the currency at some ratio. But for example for CZK it was 1 CZK = 1 credit. But then you needed to be able to pay with combinations of credits and "cash". Also you could have multiple items in the cart and you could of course after the order was complete cancel some items from the order. This resulted in the need to be able to return the credits in a fair fashion. But since the credits themselves were in the end indivisible, but you needed to make several "attempts" (there were several other rules what can/can't be applied somewhere) to combine them in a good way (1000 credits in 3 items -> 334, 333, 333), you had to operate outside of the "general scale" (we were not considering the "cents" for CZK at all, but that is irrelevant).

This was just one case of more, where we really needed to make several "steps of operations" where you could absolutely not allow any rounding, otherwise it would in the end lead to either "losing money" or "creating more money", which was being bad either for the customer, or the company (even if these were fractions).

I am not saying this is "standard", but I would think that a lot of places, where you need to perform multiple operations, you want to stay precise "until the end" and them perform a check/rounding only on the result (by providing particular context at each phase, as I described in the previous post). So this does need to be default, but with what I have shown above, it might even be, because if the default in arithmetic operations was to keep the precision (which sounds reasonable to me), but the "get result" (any of the methods) would have a checking/rounding default, I think it would work well for both simple and more complex cases.

@BenMorel
Copy link
Member Author

BenMorel commented Sep 19, 2017

I think that as little as possible should be tied with the actual instance of the "value", because I think there there is no intrinsic relation of the Money and even the scale and step.

At the very minimum, a Money is composed of a BigDecimal and a Currency. BigDecimal inherently has a scale, so Money will have a scale anyway! Only the step can be argued on.

These are all part of the operation (...)

I understand your thinking, but I'm afraid it will be hard to reach a consensus here. I think I will try to create another post summarizing our different views on the subject, and investigate which path other Money libraries from other languages have chosen to follow. This may help enlighten us all.

But I think this should be solved exactly with the Money::of(BigDecimal::of('12.34'), Currency::of('USD')); approach.

If we end up with a constructor that only assigns values and does not make any assumptions on their contents or relationship (if type-hinting is enough to ensure a valid object), what about making the constructor public with strict types, and keeping of() losely typed as it is today, for convenience?

a lot of time the "context" values would come as variables and most of the time I think this goes for values as well. And then the same thing applies - you want to have these converted to the instance of something as soon as possible in the flow.

That is not how I'm using Money at the moment. I appreciate the convenience of losely typed parameters (the poor man's overloading), and usually I'm creating the Money early enough in the flow, and/or the parameters come from a trusted source and I know I won't get an unexpected exception.

It's interesting to note that even libraries in strongly typed languages such as Java, use overloading here for convenience. See Joda Money:

  • of(CurrencyUnit currency, BigDecimal amount)
  • of(CurrencyUnit currency, double amount)
  • ...

They could've just instructed users to first create a BigDecimal, and only provide them with the first method, but they decided to overload it for convenience. I see no reason not to do the same in PHP, even if that means loose typing. As I said above though, if I a strongly typed public constructor makes sense, I may provide it as well.

because you have to put everything on a few lines, what would mostly, in a good written app, be spread out in a few different layers.

True, and I will be happy to provide a good foundation for using Money in complex and nicely structured apps. But I do not want to force users to use it in a verbose way, that can't be explained in a few lines of code, or can't be used in a quick & dirty app as well. Money should IMO provide sensible defaults so that people just playing with the API for everyday monies can, without having to work with a context, get calculation results that ideally "just make sense", and/or are well documented and do not come as a surprise.

So the scales should really either match or be "suppressed" by providing appropriate context which would handle them differently.

You're on the same page as @jiripudil in #3 on this one.

This also points me back to my aforementioned solution where steps would be defined strictly for operations

Now you're not on the same page anymore :) As far as I understand, he doesn't mind having the step in Money, but suggests we throw an exception when adding monies with different scales or steps, by default.

I won't commment on these points for now, I'll create another post specific to these, probably tomorrow.

(1000 credits in 3 items -> 334, 333, 333)

This is a fairly common problem, that can be solved easily: round down, and compute the remainder afterwards:

$amount = Money::of('1000', 'CZK', 100 /* step, if we decide it goes here */); // CZK 1000.00
$amountPerItem = $amount->dividedBy(3, RoundingMode::DOWN); // CZK 333.00
$remainder = $amount->minus($amountPerItem->multipliedBy(3)); // CZK 1.00

Now you decide what you do with the remainder: add it to one of the items, or do something else. Did I miss something?

Note that we could even add a convenient divideAndRemainder() method for this:

$amount = Money::of('1000', 'CZK', 100 /* step, if we decide it goes here */); // CZK 1000.00
[$amountPerItem, $remainder] = $amount->divideAndRemainder(3); // CZK 333.00, CZK 1.00

Now you get a quotient and a remainder Moneys that are compatible (scale & step) with your original money, with no context, no rounding involved, how good is that? :)

@BenMorel
Copy link
Member Author

(...) you may want to do a few steps and then check the value at the end, if it matches. In the second case this falls under the "operations need context" too, because actually you want to specify the context for the "getResult" method.

What about adding a startCalculation() method, that would create a BigRational-based calculator from a Money:

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

No operation would require any kind of context, as the result would be rational, and only the getResult() method would convert it back to a Money.

If we store the scale & step in the Money, the resulting Money would share the same characteristics as the original Money (at least by default).

@VasekPurchart
Copy link

At the very minimum, a Money is composed of a BigDecimal and a Currency. BigDecimal inherently has a scale, so Money will have a scale anyway! Only the step can be argued on.

Ah, I see (I did not study the current implementation, I am basing my discussion here on the facts provided in the issues). Then that's kind of out of the window, but if this is the case, then I think it should really behave strictly.

If we end up with a constructor that only assigns values and does not make any assumptions on their contents or relationship (if type-hinting is enough to ensure a valid object), what about making the constructor public with strict types, and keeping of() losely typed as it is today, for convenience?

Yes, would agree with that.

I see no reason not to do the same in PHP, even if that means loose typing. As I said above though, if I a strongly typed public constructor makes sense, I may provide it as well.

Agreed, as I mentioned above, I am not against convenience methods, but they should only stand on top of a more solid basis, which would be exactly this.

Money should IMO provide sensible defaults so that people just playing with the API for everyday monies can, without having to work with a context, get calculation results that ideally "just make sense", and/or are well documented and do not come as a surprise.

Agreed, it was never my intention to do otherwise, as with the last paragraph. Sorry if it seemed like that from my previous comments. If you can point to a specific problem, where this would occur in my suggestions, I can elaborate on what I meant by that (or maybe how I did not think that through :) ).

(1000 credits in 3 items -> 334, 333, 333)

This is a fairly common problem, that can be solved easily: round down, and compute the remainder afterwards'

Yes, this is obvious, the problem was in the there were several other rules what can/can't be applied somewhere, which meant it was not so simple :( There were several rounds of dividing, recounting and trying a different combinations in the end. Unfortunately this was like 3 years ago, so I don't remember the details as well. Only the pain :) (the algorithm plus handling all the rounding etc., cause we had only floats then).

Now you get a quotient and a remainder Moneys that are compatible (scale & step) with your original money, with no context, no rounding involved, how good is that? :)

It is great, since this is a really common problem, I was already thinking about that, but of course this is only another sugar which can be added on top of the things we are currently discussing.

Also there could be even a method for the original problem, something like:

$amount = Money::of('1000', 'CZK', 100 /* step, if we decide it goes here */); // CZK 1000.00
$amounts = $amount->distributeFailry(3);
// would result in an array of instances with 334, 333, 333

Or something similar to the allocate method here: https://github.com/moneyphp/money


Continuing with the other points in #7

@BenMorel
Copy link
Member Author

allocate() is great, I agree it would be a nice addition too.

@BenMorel
Copy link
Member Author

allocate() is now implemented in the adjustment branch.

@jkuchar
Copy link

jkuchar commented Sep 24, 2017

I have implemented something very similar, so I confirm there are use-cases for allocate() and split() methods.

@BenMorel
Copy link
Member Author

Thanks for sharing it! Do you have a use-case for non-integer ratios? I can see that your implementation accepts floats, too.

@jkuchar
Copy link

jkuchar commented Sep 24, 2017

Nothing serious. I have been splitting amounts into rations based on totals in VAT summary, because I needed to split rounding back into VAT summary (as required by Czech law). I can simply use unscaled value instead of floats. Another rationale under this was that scale does not need to be that precise, so I do not care (in my use-case) about precision that much. (I usually allocate values less then 1 CZK 😄)

However as I said I do NOT have any use-case where floats must be used (where integers aren't enough). It was mostly for ease of use.

@BenMorel
Copy link
Member Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants