search

RubyMoney / money

A Ruby Library for dealing with money and currency conversion.
http://rubymoney.github.io/money
MIT License
2.73k stars 623 forks source link

Fix for Round-Robin Distribution Logic in Money Allocation #1074

Closed tirosh closed 6 months ago

tirosh commented 6 months ago

Hello everyone 👋,

I've identified a potential issue in the round-robin distribution logic within the money allocation. It looks like the index variable is being reset to 0 at the start of each iteration in the while loop.

https://github.com/RubyMoney/money/blob/092ecc7a711263208ae12fd62e7538f3d7a6348a/lib/money/money/allocation.rb#L43-L63

As a result, the amount_to_distribute is always applied to the first element of the result array (result[0]), preventing the intended round-robin distribution across all elements.

To address this, I've made a change where the index variable is initialized outside the while loop. This adjustment allows the index to retain its value across iterations, ensuring that the amount_to_distribute is correctly applied to each element in the array in a round-robin manner.

This is the corresponding pull request: https://github.com/RubyMoney/money/pull/1073

I noticed this issue hasn't been caught in tests, likely because the remaining_amount usually doesn't exceed 1 after distribution, so it didn't produce erroneous results. However, I thought it would be a good idea to correct this to ensure the functionality works as intended for all possible cases.

@Mongey and @semmons99, since you have been working on this, it may be of particular interest to you. I'm new here, and this would be my first contribution. I'm eager to hear any feedback or suggestions you might have!

Thank you!

semmons99 commented 6 months ago

great investigation. I will move discussion over to your PR and get it merged after review.