moneyphp / money

PHP implementation of Fowler's Money pattern.
http://moneyphp.org
MIT License
4.6k stars 440 forks source link

Why is divisor argument in mod method of type Money #687

Closed helhum closed 9 months ago

helhum commented 2 years ago

Hey, forgive me if this is a dummy question. I might be missing something totally obvious.

But I was asking myself, why the argument for the mod method is of type Money. Shouldn't the$divisor argument here be the same as for the divide method?

Thanks!

UlrichEckhardt commented 2 years ago

Interesting:

So, depending on the type of calculation, division should support a divisor of type Money or unitless number. The library provides allocate() and allocateTo() instead.

The modulo operation is different, its left and right operands should be the same type. At least that's what I think... can you provide an example usage where the parameters don't fit?

helhum commented 2 years ago

Thanks for looking into this!

  • If you divide new_price / old_price, you get the price increase, a unitless number usually represented as percentage.
  • If you divide wins / shareholders, you get the amount each shareholder gets of the gains.

Yeah right. Both calculations make sense and have a use case. In your first example a ratio of two money values are calculated, in the second the equal share of money.

The modulo operation is different, its left and right operands should be the same type. At least that's what I think...

When looking a the example in the docs and looking at how the remainder is defined:

The remainder (r) is defined as: num1 = i * num2 + r, for some integer i

For the example it means:

8.30€ % 3.00€ = 2.30€ because: 8.30€ = 2 * 3.00€ + 2.30€

However this calculation is valid as well:

8.30€ % 3 = 2.30€ because: 8.30€ = 2€ * 3 + 2.30€

can you provide an example usage where the parameters don't fit?

The use case for my calculation could be:

"If we want split the bill equally between 3 people, what is the raining remainder, that can't be split equally"

While mathematically totally correct, I can't come up with what the use case of the documented example is. Can you help me out?

Anyway, for my use case I could of course do:

$bill = Money::EUR(830);      // € 8.30
$bill->mod(Money::EUR(300));  // € 2.30

But that does not seem to carry the intention right. I'd rather want to write this:

$bill = Money::EUR(830);   // € 8.30
$bill->mod(3);             // € 2.30

It does not matter much, though. Thanks again for taking the time and for providing/ maintaining this useful library. It helps a lot.

If you'd consider the use case I presented, I could come up with a PR that allows the mod method to work with both, a money object or a number.

helhum commented 2 years ago

If you'd consider the use case I presented, I could come up with a PR that allows the mod method to work with both, a money object or a number.

Was easy enough to give it a shot. Feel free to close both this ticket and the PR, if you think it does not fit.

frederikbosch commented 1 year ago

Rather than having another allowed -mod() parameter I think a new method would be more appropriate, e.g. ->allocateForRemainder(int $n)

helhum commented 1 year ago

Rather than having another allowed -mod() parameter I think a new method would be more appropriate, e.g. ->allocateForRemainder(int $n)

Thanks for your feedback. What exactly do you mean with "another mod() parameter? You mean adding new allowed types to the one parameter? allocateForRemainder is not something I personally would understand, but yes, a new method would work, too of course.

lokrain commented 1 year ago

Hey there,

In the given example: 8.30€ % 3.00€ = 2.30€

Possible logic:

  1. 8.30€ = 2 * 3.00€ + 2.30€
  2. 8.30€ = 2€ * 3 + 2.30€

For me(and maths), mod operation should not "think" about currency, but instead only use numeric parameters.

If you tell the user of this package that he can use the currency, then the use case of 8.30€ % $3.00 appears. While you can use an exchange package, I would prefer an Exception here.

The easiest implementation for you, best option for me and least bug prone would be to restrict the function to only accept a number.