@artfulrobot @NCatalani would you mind reviewing this?

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

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.

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.

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

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

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?

@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!