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

Feature/allocate #56

Merged
merged 3 commits into from
Sep 2, 2014
Merged

Feature/allocate #56

merged 3 commits into from
Sep 2, 2014

Conversation

sagikazarmark
Copy link
Collaborator

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:

$m = new Money::EUR(15);

// Returns [8, 7]
$m->allocateTo(2);

Please merge #55 before, so I can rebase this if needed

@@ -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
Copy link
Collaborator

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.

Copy link
Collaborator Author

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.

Copy link
Collaborator

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reverting it then.

@mathiasverraes
Copy link
Collaborator

  • Adapt documentation

@sagikazarmark
Copy link
Collaborator Author

Is it possible to merge this PR and collect all documentation in one PR?

@sagikazarmark
Copy link
Collaborator Author

Reverted functionality

mathiasverraes added a commit that referenced this pull request Sep 2, 2014
@mathiasverraes mathiasverraes merged commit 61ee0c6 into moneyphp:nextrelease Sep 2, 2014
@sagikazarmark sagikazarmark deleted the feature/allocate branch September 2, 2014 15:04
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

Successfully merging this pull request may close these issues.

2 participants