Adds NewFromFloatWithRound to avoid precision loss #159
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's GuideAdds a NewFromFloatWithRound method that uses math.Round to avoid precision loss in float-to-money conversion, accompanied by unit tests for positive, negative, and unregistered currency scenarios, and updates to documentation to illustrate its usage. Class diagram for Money creation methodsclassDiagram
class Money {
- int64 amount
- *Currency currency
+ Currency() *Currency
}
class Currency {
- int Fraction
+ get() *Currency
}
Money <.. Currency : uses
class money {
+ New(amount int64, code string) *Money
+ NewFromFloat(amount float64, code string) *Money
+ NewFromFloatWithRound(amount float64, code string) *Money
}
money ..> Money : creates
money ..> Currency : uses
%% Highlight new method
class money {
+ NewFromFloatWithRound(amount float64, code string) *Money
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @rfloriano - I've reviewed your changes - here's some feedback:
- Update the doc comments and README to accurately state that math.Round rounds ties away from zero (not banker's rounding).
- Add a positive mid-point test case (e.g., 1.005 for USD) to verify the expected rounding-away-from-zero behavior.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Update the doc comments and README to accurately state that math.Round rounds ties away from zero (not banker's rounding).
- Add a positive mid-point test case (e.g., 1.005 for USD) to verify the expected rounding-away-from-zero behavior.
## Individual Comments
### Comment 1
<location> `money.go:99` </location>
<code_context>
+// NewFromFloatWithRound creates and returns new instance of Money from a float64.
+// It rounds trailing decimals to the nearest integer as math.Round does.
+func NewFromFloatWithRound(amount float64, code string) *Money {
+ currency := newCurrency(code).get()
+ currencyDecimals := math.Pow10(currency.Fraction)
</code_context>
<issue_to_address>
Consider handling NaN and Inf values for the amount parameter.
math.Round will pass through NaN or Inf values, which can cause unexpected behavior or panics when converting to int64. Please add explicit checks for these cases and handle them appropriately.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
func NewFromFloatWithRound(amount float64, code string) *Money {
currency := newCurrency(code).get()
currencyDecimals := math.Pow10(currency.Fraction)
return &Money{
amount: int64(math.Round(amount * currencyDecimals)),
currency: currency,
}
}
=======
func NewFromFloatWithRound(amount float64, code string) *Money {
if math.IsNaN(amount) || math.IsInf(amount, 0) {
return nil
}
currency := newCurrency(code).get()
currencyDecimals := math.Pow10(currency.Fraction)
return &Money{
amount: int64(math.Round(amount * currencyDecimals)),
currency: currency,
}
}
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `money_test.go:844` </location>
<code_context>
}
}
+func TestNewFromFloatWithRound(t *testing.T) {
+ m := NewFromFloatWithRound(12.34, EUR)
+
+ if m.amount != 1234 {
+ t.Errorf("Expected %d got %d", 1234, m.amount)
+ }
+
+ if m.currency.Code != EUR {
+ t.Errorf("Expected currency %s got %s", EUR, m.currency.Code)
+ }
+
+ m = NewFromFloatWithRound(12.34, "eur")
+
+ if m.amount != 1234 {
+ t.Errorf("Expected %d got %d", 1234, m.amount)
+ }
+
+ if m.currency.Code != EUR {
+ t.Errorf("Expected currency %s got %s", EUR, m.currency.Code)
+ }
+
+ m = NewFromFloatWithRound(-0.125, EUR)
+
+ if m.amount != -13 {
+ t.Errorf("Expected %d got %d", -13, m.amount)
+ }
+
+ m = NewFromFloatWithRound(147.23, BRL)
+
+ if m.amount != 14723 {
+ t.Errorf("Expected %d got %d", 14723, m.amount)
+ }
+}
+
+func TestNewFromFloatWithRound_WithUnregisteredCurrency(t *testing.T) {
</code_context>
<issue_to_address>
Missing edge case tests for rounding behavior at .5 boundaries and for currencies with different fractions.
Please add tests for rounding at .5 boundaries (e.g., 0.125, 0.135, 0.145) and for currencies with 0 and 3 decimal places (e.g., JPY, IQD) to ensure correct handling of these cases.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add
NewFromFloatWithRoundfunction to fix precision loss in float-to-money conversionProblem
The existing
NewFromFloatfunction has precision loss issues due to its behavior of always truncating trailing decimals down (usingint64()conversion). This leads to unexpected results when converting precise decimal values:This behavior is particularly problematic when:
Solution
This PR introduces
NewFromFloatWithRound, a new function that uses proper mathematical rounding (math.Round) instead of truncation:Key Improvements
Precision preservation: Exact decimal values are preserved correctly
Proper rounding behavior: Uses banker's rounding (round half to even) via
math.RoundConsistent with mathematical expectations: Follows standard rounding rules instead of always rounding down
Backward Compatibility
NewFromFloatfunction remains unchanged to maintain backward compatibilityUsage Examples
This enhancement addresses precision issues while maintaining full backward compatibility, giving developers the choice between truncation and proper rounding based on their specific use cases.
Summary by Sourcery
Add NewFromFloatWithRound to handle float-to-money conversion using math.Round instead of truncation, maintaining backward compatibility, and update tests and documentation accordingly
New Features:
Enhancements:
Documentation:
Tests: