-
Notifications
You must be signed in to change notification settings - Fork 441
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
Feature/allocate #56
Feature/allocate #56
Conversation
@@ -344,12 +344,16 @@ public function divide($divisor, $rounding_mode = self::ROUND_HALF_UP) | |||
/** | |||
* Allocate the money according to a list of ratios | |||
* | |||
* @param array $ratios | |||
* @param mixed $ratios |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really dislike supporting two styles of methods. It creates confusion and bugs. Array is the best choice because it reflects the fact that it is a list of variable length.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does not mean you cannot use arrays. If it is mentioned in the docs you just go with it and don't use any other way. However, IMO using array is the same as using many parameters. At the end, they also become an array.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand why it appeals, but it is really bad practice in php. If we had method overloading, there'd be a case for it. It also has no cognitive overload as allocate([1, 2])
is just as easy to read and understand as allocate(1, 2)
.
Unless you have a problem that is better solved with a variadic definition, I'm sticking with arrays.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reverting it then.
|
Is it possible to merge this PR and collect all documentation in one PR? |
Reverted functionality |
allocate
now supports the following form:$m->allocate(1, 1, 1)
(array can be ommited)Adds
allocateTo
: Divides the amount to N equal values (the remainder is shared between the first K results)Example:
Please merge #55 before, so I can rebase this if needed