search

brick / money

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

Fix Money::allocateWithRemainder() #62

Closed BenMorel closed 1 year ago

BenMorel commented 2 years ago

Fixes #55

BenMorel commented 2 years ago

@artfulrobot @NCatalani would you mind reviewing this?

Do you think we should change the implementation of allocate() as well?

bendavies commented 1 year ago

just a FYI, i ran this testcase through moneyphp/money Money::allocate(...), and it returns 2 cents and 1 cents, while brick/money will return 3/0.

BenMorel commented 1 year ago

Thank you @bendavies, I don't think returning 2 & 1 cents makes any sense, and I do believe now that the behaviour described by @artfulrobot, implemented here, is the correct one.

I'm not sure what to do with allocate() now, I feel like this algorithm (distributing the remainder over the first monies) is rarely, if ever, what you really need. What do you think about deprecating/removing this method, @bendavies @artfulrobot @NCatalani?

IMO, allocateWithRemainder() is the only sensible method. What you do with the remainder is very application-specific, and I don't think that the library should offer options here.

artfulrobot commented 1 year ago

I agree with all that @BenMorel said there; the tests pass; looks good!

To clarify: yes, I'd suggest deprecating allocate().

bendavies commented 1 year ago

Thank you @bendavies, I don't think returning 2 & 1 cents makes any sense, and I do believe now that the behaviour described by @artfulrobot, implemented here, is the correct one.

yes, i agree - i was just highlighting the difference.

I'm not sure what to do with allocate() now, I feel like this algorithm (distributing the remainder over the first monies) is rarely, if ever, what you really need. What do you think about deprecating/removing this method, @bendavies @artfulrobot @NCatalani?

well, allocate is part of fowlers money pattern, so that would be an argument for keeping it?

BenMorel commented 1 year ago

@bendavies

well, allocate is part of fowlers money pattern, so that would be an argument for keeping it?

Sure, but I'd be curious what @martinfowler thinks today, I guess the algorithm he presented in his PoEAA book was just an example of what was possible, but I'm not sure which real-life application it has or if it was actually useful to someone as is.

Moving forward, I'll merge the allocateWithRemainder() method, and let's keep the allocate() method as it is for now. We can always discuss either deprecating it, or renaming it so that it is explicitly tells what it really does later on!