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

What about toDecimal? #6

Closed
Ianfeather opened this issue Nov 9, 2015 · 10 comments
Closed

What about toDecimal? #6

Ianfeather opened this issue Nov 9, 2015 · 10 comments

Comments

@Ianfeather
Copy link

I'm implementing this for the first time so perhaps this is something obvious but if I want to eventually output the money as a decimal is this not something which should live within the money object code?

Seems odd that you would safely be able to do calculations whilst it is represented as cents but then you risk causing an issue by having to do the toDecimal implementation yourself.

Thanks in advance for any advice

@caccialdo
Copy link
Contributor

I tried to extend the class in #7. You should check it out.

@caccialdo caccialdo mentioned this issue Nov 11, 2015
@drewclauson
Copy link

I wasn't ready to trust the math calculation of this:

return (this.amount / 100).toFixed(2);

So, since I'm only converting to decimal in order to display data, I'm doing a string manipulation to avoid any possibility of conversion issues (I think I had one scenario where this had actually happened and $4.30 became $4.31, so I decided to stick with string operations)

function moneyToDecimalString(amount, currency) {
    amount = amount.toString();
    let position = amount.length - Money[currency].decimal_digits;
    return [amount.slice(0, position), '.', amount.slice(position)].join('');
}

Just FYI if it's helpful for you. I have no idea which is better performance. My code hasn't been upgraded to work with #7

@caccialdo
Copy link
Contributor

Even outside of performance considerations, your method the currency's decimal_digits prop into account which definitely an issue in my pull request 😞

Your implementation is definitely safe calculation-wise so I might use yours but I've failed to find an example where .toFixed falls short.

@caccialdo
Copy link
Contributor

> 1-0.32
> 0.6799999999999999

> (1-0.32).toFixed(2)
> "0.68"

> 1-0.18
> 0.8200000000000001

> (1-0.18).toFixed(2)
> "0.82"

toFixed seems to be rounding to the closest decimal which I think is fine. If we really need to floor the value, we could use .toFixed(n+1).slice(0, -1).

@drewclauson
Copy link

Now that I think about it, I think it was my backend API that was having the calculation failure and not Javascript... But even so I wanted to take the decimal_digits into account and this was the only way I could think of doing that, although this should work to use toFixed(2) and get the decimal in the right place.

return (this.amount / Math.pow(10, this.currency.decimal_digits)).toFixed(2);

@drewclauson
Copy link

Edit, missed the toFixed parameter change

return (this.amount / Math.pow(10, this.currency.decimal_digits)).toFixed(this.currency.decimal_digits);

@caccialdo
Copy link
Contributor

Thanks for the help. I updated the toString and toDecimal methods in #7 👍

@caccialdo
Copy link
Contributor

#7 was merged and version v0.5.0 contains the method on its prototype.

@Ianfeather
Copy link
Author

Thanks @caccialdo, you're my hero!

@Ianfeather
Copy link
Author

New changes are looking great btw. Thanks all

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

4 participants