search

brick / money

A money and currency library for PHP
MIT License
1.61k stars 96 forks source link

Add (failing) test case for allocateWithRemainder #55

Closed artfulrobot closed 1 year ago

artfulrobot commented 2 years ago

Given ratios 75, 25, and an amount of 0.03 GBP, I would expect Brick not to allocate any money, since it cannot be divided according to the ratios.

i.e. I'd expect: 0, 0, remainder 0.03.

Instead it allocates 0.02, 0, remainder 0.01

Therefore successive allocations of small amounts would result in all the money getting funnelled to the first account, which the owner of the 2nd account might not feel is fair.

artfulrobot commented 2 years ago

Theoretically, I think Brick should refuse to allocate when the lowest common denominator of the ratio (4 in this case) is greater than the number of minor units to allocate (3 in this case).

However I've just noticed this test case https://github.com/brick/money/blob/0.5.3/tests/MoneyTest.php#L437 Where you say that ratios 1, 1, 3, 1 (i.e. 1/6ths and 1/2) applied to 0.02 should result in nothing for the sixths and 0.01 for the half.

This seems wrong to me.

BenMorel commented 2 years ago

@NCatalani Do you want to fix this?

BenMorel commented 2 years ago

I have a working fix at #62. Ignoring the GCD calculations part that should be moved to brick/math, could you please review the implementation and tests, @artfulrobot @NCatalani?

Also, do you think we should change the implementation of allocate() as well?

BenMorel commented 1 year ago

Ping @artfulrobot @NCatalani

artfulrobot commented 1 year ago

@BenMorel I'm happy to take a look, but I'm v busy at mo. I've put it on my do-list though.