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

Documentation regarding creation of Dinero objects from floats in Dinero V2 #761

Open
quophyie opened this issue Oct 25, 2023 · 3 comments

Comments

@quophyie
Copy link

quophyie commented Oct 25, 2023

In FAQ docs in Dinero v2-alpha i.e (https://v2.dinerojs.com/docs/faq/how-can-i-create-dinero-objects-from-floats, follows from #58), the code is written as follows

function dineroFromFloat({ amount: float, currency, scale }) {
  const factor = currency.base ** currency.exponent || scale;
  const amount = Math.round(float * factor);

  return dinero({ amount, currency, scale });
}

Although the above is not incorrect, per se, it gives precedence to currency.exponent whereas I think it should give precedence to the user supplied scale . This is because when the caller calls the function with a scale, the caller would expect the supplied scale to be applied first and then if that fails, then try and apply the default scale of the currency

So I propose,

function dineroFromFloat({ amount: float, currency, scale }) {
 // NOT const factor = currency.base ** currency.exponent || scale;
// I propose it should be
  const factor =  scale || currency.base ** currency.exponent ; 
// OR
// const factor =  currency.base ** (scale ?? currency.exponent) ;
  const amount = Math.round(float * factor);

  return dinero({ amount, currency, scale });
}
@quophyie quophyie changed the title Documentation regarding creation of Dinero objects from floats Documentation regarding creation of Dinero objects from floats in Dinero V2 Oct 25, 2023
@ch1booze
Copy link

ch1booze commented Jul 12, 2024

Hi @quophyie. Recently, I came across an article working with money in software. It should explain why using floats to create currency objects is not encouraged. The Dinero.js v2 docs even explicitly states that it is a bad idea.

@CanRau
Copy link

CanRau commented Jul 12, 2024

You're right @ch1booze though it's sometimes still necessary to convert, for example from 3rd sources which might give you string floats.
So it's good to have a helper like this handy although I'm not yet sure about the proposed change here 😅

@quophyie
Copy link
Author

Hi @ch1booze,
You are probably right about not using floats for money as @CanRau said. However, this proposed change is not about the use of floats for money but rather, what takes precedence in terms of what the factor calculation should be.

The docs and code as provided in the docs give precedence to using the currency.exponent in the calculation of the factor whereas I think that if a caller of the function supplies a scale, they would expect the scale that they have provided to take precedence over currency.exponent i.e. the caller should expect that all the args that they have provided are used in the internal calculation of the factor.

If the caller of the function does not want the scale to be used in the factor calculation, they should not provide it.. in which case, currency.exponent should take precedence. Its just a difference of opinion and probably a cleaner interface

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

3 participants