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

Add NewFromString (#108) #115

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

npinochet
Copy link
Contributor

@npinochet npinochet commented Jul 21, 2022

Half of the PR here: #116

@npinochet npinochet force-pushed the feat/new-from-float-string branch 2 times, most recently from 9a62bed to e315ce2 Compare July 25, 2022 14:37
money.go Outdated
amount = strings.Trim(amount, "0")

var exponent int
if pointIndex := strings.Index(amount, "."); pointIndex != -1 {
Copy link
Contributor

Choose a reason for hiding this comment

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

This implementation does not honor the currency separator. Not all currencies use the dot.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're absolutely right, I totally missed that, thanks for pointing it out

money.go Outdated

var exponent int
if pointIndex := strings.Index(amount, "."); pointIndex != -1 {
exponent = len(amount[pointIndex+1:])
Copy link
Contributor

Choose a reason for hiding this comment

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

You can do

Suggested change
exponent = len(amount[pointIndex+1:])
exponent = len(amount)-pointIndex-1

without the need to create a slice in the middle

money.go Outdated
}

currencyFraction := GetCurrency(currency).Fraction
parsed /= int64(math.Pow(10, float64(exponent-currencyFraction)))
Copy link
Contributor

Choose a reason for hiding this comment

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

The use of floating point math can introduce rounding problems. Money uses int math just to avoid that.
What do you think using an alternate approach to parsing it¡
For example:

func NewFromStr(amount string, currencyCode string) (*Money, error) {
	currency := m.GetCurrency(currencyCode)

	amountInt, err := parsers.ParseNumber(amount, currency.Fraction, currency.Decimal)

	if err != nil {
		 fmt.Errorf("can't parse '%s' to money", amount)
	}

	return New(amountInt, currencyCode)
}

Where:

// ParseNumber parses the string for the representation of a number in
// format "dddd.dd" or "dddddd" (without decimal digit)
// Note: If excess decimals contains invalid characters, they are ignored
func ParseNumber(s string, decimals int, decimal string) (int64, error) {
	dotPos := strings.IndexByte(s, decimal)

	var toParse string

	var digits int

	if dotPos < 0 {
		toParse = s
	} else {
		digits = len(s) - dotPos - 1
		if digits > decimals {
			digits = decimals
		}

                // Remove decimal separator and in excess decimals
		toParse = s[:dotPos] + s[dotPos+1:dotPos+1+digits]
	}

	value, err := strconv.ParseInt(toParse, 10, 64)

	if err != nil {
		return 0, err
	}

        // Add missing decimal positions
	for d := digits; d < decimals; d++ {
		value *= 10
	}

	return value, nil
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the suggestions, this is much better than my parser

money.go Outdated
}

// NewFromString creates and returns new instance of Money from a string.
func NewFromString(amount string, currency string) (*Money, error) {
Copy link
Owner

Choose a reason for hiding this comment

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

@npinochet Thanks for the PR!

NewFromString is a bit tricky to implement. I can think of so many cases we need to take into account, that I'm not really sure that it is a job for the Money package to crack it. When I see the function called NewFromString I expect that it takes the string my project represents/displays the money and it creates the Money instance from it, but from this implementation it looks like it only accepts a single format (xx.xxx). Moreover, it cannot handle the Money.Display() output, which I would expect it to do. Couple of things to consider before implementing it:

Should we clearly document that it only accepts the "number" in a specific format, e.g. "xx.xxx" and nothing else?
When we display money we sometimes separate thousands with comma, should we handle it too?
What about the cases where the money has a currency sign in it '£ xx.xxx' or the currency code 'xx.xxx USD'?
Should we trim or remove any other non numeric characters from a string? Or return an error? e.g. " 12.123 ", "12.beefwellington"?

In my opinion, I would leave up to the caller to parse the string to int/float depending on the project needs and then pass it to money to create a new instance of it.
Should we separate float/string PR's? I'm happy with Float implementation and could merge it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! IMO I too believe the simplest parser should be enough, the user can pre-parse it's input to be valid. A fully compliant parser would be too much for this library scope. I've updated the PR with the suggested changes.
I'll split the PR into float and string tomorrow.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can think on a simple NewFromString(), perhaps only accepting the "." as decimal separator and no other char accepted.

Another function can be money.Parse(amount string, currency string) to take into account locale separators.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The NewFromString function in this PR only accept simple float-like strings, with the only restriction that they have to use the currency.Decimal as the separator. But since one must specify a currency to instantiate a Money, I believe this is acceptable. I'm open to change it to only accept "." decimals though, if it makes the function simpler.

money.go Outdated
@@ -85,6 +88,33 @@ func New(amount int64, code string) *Money {
}
}

// NewFromFloat creates and returns new instance of Money from a float64.
func NewFromFloat(amount float64, currency string) *Money {
Copy link
Owner

Choose a reason for hiding this comment

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

Should we mention in the comments how rounding works?
Please also add new function to README

money.go Outdated
@@ -85,6 +88,33 @@ func New(amount int64, code string) *Money {
}
}

// NewFromFloat creates and returns new instance of Money from a float64.
func NewFromFloat(amount float64, currency string) *Money {
currencyDecimals := math.Pow(10, float64(GetCurrency(currency).Fraction))
Copy link
Owner

Choose a reason for hiding this comment

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

Pow10 is so much more optimised and you don't need casting.

Suggested change
currencyDecimals := math.Pow(10, float64(GetCurrency(currency).Fraction))
currencyDecimals := math.Pow10(GetCurrency(currency).Fraction)

@npinochet npinochet force-pushed the feat/new-from-float-string branch 3 times, most recently from 0d4cada to d89be76 Compare August 2, 2022 02:32
@npinochet npinochet changed the title Add NewFromFloat and NewFromString (#108) Add NewFromString (#108) Aug 2, 2022
@npinochet npinochet force-pushed the feat/new-from-float-string branch from d89be76 to a2b580e Compare August 3, 2022 16:45
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.

3 participants