Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
tirosh opened this issue Mar 2, 2024 · 1 comment
Closed

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

tirosh opened this issue Mar 2, 2024 · 1 comment

Comments

@tirosh
Copy link
Member

tirosh commented Mar 2, 2024

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.

## round-robin allocation of any remaining pennies
if result.size > 0
while remaining_amount != 0
index = 0
amount_to_distribute = [1, remaining_amount.abs].min
if remaining_amount > 0
result[index] += amount_to_distribute
remaining_amount -= amount_to_distribute
else
result[index] -= amount_to_distribute
remaining_amount += amount_to_distribute
end
index += 1
if index > result.size
index = 0
end
end
end

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: #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
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants