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

Precise money #335

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Precise money #335

wants to merge 13 commits into from

Conversation

frederikbosch
Copy link
Member

Adds PreciseMoney. Fixes #7.

@frederikbosch
Copy link
Member Author

frederikbosch commented Nov 29, 2016

Few considerations before continuing:

  1. Is this the right path to start with?
  2. What to do with the PHP Calculator? It will have strange results when multiplying and dividing, so I left it out of the PreciseMoney calculators. However, I did implement some changes for tests to succeed (all floating number bug related changes).

What should be next?

  1. What to do with formatting and parsing? Do we want to support it at all?
  2. What to do with conversion? Do we want to support it at all?
  3. ...

@m4tthumphrey
Copy link

Is this still active? I presume I am right in thinking that this is to allow us to perform operations using more decimal places? Ie GBP 1.1535 * 0.4251 etc?

@frederikbosch
Copy link
Member Author

@m4tthumphrey You can already do that.

new Money(11535, new Currency('GBP'));
new Money(4251, new Currency('GBP'));

You should implement your own currency with subunits set to 4. See the BitcoinCurrencies how to do this. If there are more currencies involved than only the pound, use the Aggregate Currencies class to turn multiple currencies it into a single repository.

PreciseMoney could be used if you do not want to round your amounts. But as you can see, this branch has not been merged yet. Use at your own risk.

@m4tthumphrey
Copy link

I was more enquiring than actually looking to use it!

I have just looked into the custom currencies and I see how it works now! Thanks!

@gmponos
Copy link
Contributor

gmponos commented Aug 4, 2017

Hello,

What's the status of this? Is it gonna be merged?

@frederikbosch
Copy link
Member Author

No progress. I expect to look at in the end of the year. If someone could give some feedback, that could - of course - help to have the feature earlier.

@frederikbosch frederikbosch mentioned this pull request Aug 21, 2017
5 tasks
@frederikbosch
Copy link
Member Author

@sagikazarmark I am planning to update this PR. The considerations I mentioned earlier on are still valid.

Is this the right path? Meaning: should we create a new class PreciseMoney that supports unlimited decimals? Or should we embed the functionality in Money? The current solution is the former. However it has cons. First of all, we cannot reuse the formatters/parsers/currencies within PreciseMoney. But that is - of course - only a con if the domain requires the functionality. And there is a way of overcoming this by creating conversion methods: Money <-> PreciseMoney.

So the questions are?

  1. Create a new class PreciseMoney or embed in Money?
  2. If answer is create a new class PreciseMoney, how to support formatters/parsers/currencies?
  3. Should we create a toMoney(int $decimals) in PreciseMoney?
  4. Should we create a toMoney(Currencies $currency) method in PreciseMoney?
  5. Or should there be something like PreciseToMoneyConverter?
  6. Or should be add methods to Money, like fromPreciseMoney()?
  7. What if PreciseMoney was not be currencies aware, so PreciseMoney = f(amount) instead of PreciseMoney = f(amount, currency)?

@frederikbosch
Copy link
Member Author

frederikbosch commented Aug 21, 2017

My initial answers are as follows.

  1. Create a new class PreciseMoney or embed in Money?

New class

  1. If answer is create a new class PreciseMoney, how to support formatters/parsers/currencies?

Convert PreciseMoney to Money (one direction).

  1. Should we create a toMoney(int $decimals) in PreciseMoney?

Yes

  1. Should we create a toMoney(Currencies $currency) method in PreciseMoney?

No

  1. Or should there be something like PreciseToMoneyConverter?

No

  1. Or should be add methods to Money, like fromPreciseMoney()?

No, we should add fromMoney(Money $money): PreciseMoney and toMoney(int $decimals): Money to PreciseMoney only. Money does not have to know the existence of PreciseMoney.

  1. What if PreciseMoney was not be currencies aware, so PreciseMoney = f(amount) instead of PreciseMoney = f(amount, currency)?

Bad idea.

@frederikbosch
Copy link
Member Author

@sagikazarmark Could you emphasize your thoughts. Maybe just start with some initials thoughts. Then we will iterate towards the optimal solution.

The scale (precision) parameter was missing from the comparison operation, leading to wrong results.
@willemstuursma
Copy link
Contributor

Hi @frederikbosch.

Here at Mollie we think adding a PreciseMoney type is a great idea, but you should not be able to convert from PreciseMoney to Money.

@frederikbosch
Copy link
Member Author

@willemstuursma Cool to see you drop in. Could you emphasize why we should not be able to convert to Money? How to support formatters/parsers/currencies for PreciseMoney without conversion possibility?

@willemstuursma
Copy link
Contributor

@frederikbosch Why don't you drop by our office next week and we can discuss it with out team - maybe a few beers and we can see how Mollie can help bring this to fruition.

@frederikbosch
Copy link
Member Author

@willemstuursma Great idea. However, I am on honeymoon leave from Tuesday. So, it will have to wait until November.

@willemstuursma
Copy link
Contributor

Oh congrats! Well, send me an email when you're back and enjoy your honeymoon together!

@sagikazarmark
Copy link
Collaborator

@frederikbosch congrats! Let's talk about this later!

Fix comparison operation
@pcbulldozer
Copy link

Hi all. Not sure where to see if there is any activity on this (and the other release) items?
Looking for a Money library around which to wrap a series of financial calculators and models - this one works well, bar the precision aspects. I'd have thought the quickest (perhaps not the most elegant) way to get there would have been to allow for instance-specific registration of a calculator (since the calculators can already be created with higher precision) and then to expose a NO_ROUNDING mode on the multiply and divide methods. I see @frederikbosch comments above - I'd probably have gone the other way and expand the functionality of Money, rather than add PreciseMoney to the mix, but I think I can see his motivation.
Either way, great library, great potential, so hoping these items are on the cards for the near future!

@willemstuursma
Copy link
Contributor

@pcbulldozer in the mean time, you can also create high precision currencies and use those.

@pcbulldozer
Copy link

thanks @willemstuursma will be giving this some thought today! appreciate the response.

@arubacao
Copy link
Contributor

arubacao commented Nov 5, 2018

@willemstuursma Did you take a look and mind sharing your solution?

@nwehrhan
Copy link

nwehrhan commented Sep 4, 2020

Just checking in on this thread. I would love to see this PR come to fruition. Not sure what the current status on this is? Did you guys ( @frederikbosch and @willemstuursma ) come to a solution/agreement on how to implement this? Is there something I could do to help? I see there are currently conflicts, but not sure if this idea is abandoned.

@frederikbosch frederikbosch added this to the 4.1.0 milestone May 5, 2021
@BonBonSlick
Copy link

So, what do other do with high fraction 8+ numbers?

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.

Deal with decimals using PreciseMoney
10 participants