moneyphp / money

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

Multiply when float will going wrong result #763

Closed ivan88217 closed 10 months ago

ivan88217 commented 11 months ago

When I use float to be a multiplier, then it will be converted to int and got wrong answer

$amount = new Money(10000);
dd($amount->multiply(0.3)); // Money(0)

I change the type hint from int|string to int|string|float and it work functional again

from

public function multiply(int|string $multiplier, int $roundingMode = self::ROUND_HALF_UP): Money

to

public function multiply(int|string|float $multiplier, int $roundingMode = self::ROUND_HALF_UP): Money
wjzijderveld commented 11 months ago

The whole point of this library is to avoid using floats for money 😅 That's why it's using strings internally and has some helpers to allow the usage of integers (it's using strings to allow for numbers greater than PHP_INT_MAX).

> php -r 'echo var_dump(0.1+0.2);'
float(0.30000000000000004)

Also see https://www.php.net/manual/en/language.types.float.php#language.types.float and https://floating-point-gui.de

wilcol commented 11 months ago

The whole point of this library is to avoid using floats for money 😅 That's why it's using strings internally and has some helpers to allow the usage of integers (it's using strings to allow for numbers greater than PHP_INT_MAX).

> php -r 'echo var_dump(0.1+0.2);'
float(0.30000000000000004)

Also see https://www.php.net/manual/en/language.types.float.php#language.types.float and https://floating-point-gui.de

I'm also having the same issue as the OP. If the reason is to avoid floats, why is there a roundingMode parameter present in the multiply function? (int * int = int) But more importantly; why would multiplication with a float not be a valid usecase to cover off in this library? In my case it's being used for VAT calculations. As a quick/dirty fix I'm now casting floats to strings, but it would help to get guidance if the multiply (and divide) function(s) should be used with integers only. The manual (https://www.moneyphp.org/en/stable/features/operation.html#multiplication-division) is also not explicit on this. Happy to hear the thoughts on this from the team.

Thanks for supporting this great library!

wjzijderveld commented 11 months ago

The problem isn't decimals, it's floats, as they they aren't precise and can result in expected results. $amount->multiply("0.3"); works just fine, casting to string isn't a dirty fix, it's the approach that allows the library to handle the calculations while keeping precision.

btw, disclaimer; I'm not part of this library, I've just been using it since Matthias first published it 12,5 years ago.

wilcol commented 11 months ago

Thanks @wjzijderveld for your inputs and thoughts. I understand the unexpected results rounding and truncation can give, that's why I use this library myself as well :) I'm looking at this from a 'clean code' and naming perspective. In my view, it's inconsitent to only accept int | string. Either use string only (and document its possible values), or accept all posible types string | int | float | etc... This makes the method signature explicit without any need to guess... and also keeps the method backwards compatible since no deprecation notices were given... Anyway, interesting food for thought!

ivan88217 commented 10 months ago

Thanks @wjzijderveld for your points. Although I know avoid to use float is right, but I'm just confusing about different behavior from upgrade to new version. It broke my legacy system when I upgrade laravel version from 6 to 9, and I can't find out the document for that.

I'm currently using it to calculate tax rates, so if it can't accept float, things will be a bit troublesome.

frederikbosch commented 10 months ago

No float, it has been explained before.