Repository navigation
chore: Docs, Naming + Tests - #2
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughSummary by CodeRabbitRelease Notes
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughThis pull request introduces a comprehensive set of public APIs across multiple packages: a Calculator type with arithmetic and rounding methods, expanded currency management with Manager and Map enhancements, exchange rate conversion functionality, Money type inspection and comparison methods, and new test utilities. Data references throughout test files are updated from USD to SGD. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (14)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (14)
calculator/calculator.go (7)
31-38: Silent failure on nil receiver may mask errors.When the calculator is nil, SafeMultiply returns
(0, nil)without an error. This silently fails and could mask bugs where callers expect an error to be returned when the result is 0. Consider returning an error to make the failure explicit.Apply this diff:
func (c *Calculator) SafeMultiply(initial int64, multipliers ...int64) (int64, error) { if c == nil { - return 0, nil + return 0, errors.New("calculator: nil receiver") } return SafeMultiply(initial, multipliers...) }Note: Add
"errors"to imports if not already present.
40-47: Missing overflow check for MinInt64 / -1.The Divide method doesn't check for the overflow case when
amount == math.MinInt64andseed == -1. In Go, this causes a runtime panic because the result would exceedmath.MaxInt64.Apply this diff:
func (c *Calculator) Divide(amount Amount, seed int64) Amount { if c == nil || seed == 0 { return 0 } + + if amount == math.MinInt64 && seed == -1 { + return 0 // Overflow protection + } return amount / seed }
49-56: Missing overflow check for MinInt64 % -1.The Modulus method doesn't check for the overflow case when
amount == math.MinInt64andseed == -1. In Go, this causes a runtime panic.Apply this diff:
func (c *Calculator) Modulus(amount Amount, seed int64) Amount { if c == nil || seed == 0 { return 0 } + + if amount == math.MinInt64 && seed == -1 { + return 0 // Overflow protection + } return amount % seed }
67-78: Incorrect overflow check in Absolute method.Line 69 checks
amount < math.MinInt64, which is impossible since no int64 value can be less thanmath.MinInt64. The intended check should beamount == math.MinInt64to protect against overflow when negating the minimum value.Apply this diff:
func (c *Calculator) Absolute(amount Amount) Amount { - if c == nil || amount < math.MinInt64 { + if c == nil || amount == math.MinInt64 { return 0 } if amount < 0 { return -amount } return amount }
80-91: Missing overflow protection in Negative method.The method doesn't protect against negating
math.MinInt64. Whenamount == math.MinInt64, attempting to negate it (line 87 implicitly or line 90's logic) would cause overflow since-math.MinInt64exceedsmath.MaxInt64.Apply this diff:
func (c *Calculator) Negative(amount Amount) Amount { if c == nil { return 0 } + + if amount == math.MinInt64 { + return 0 // Overflow protection + } if amount > 0 { return -amount } return amount }
93-136: Float precision risk in rounding calculation.Line 113 uses
math.Pow(10, float64(exponent))and converts back toint64. For monetary calculations requiring exact precision, using floating-point arithmetic can introduce rounding errors. Consider using integer exponentiation instead.Apply this diff to use integer exponentiation:
- // Calculate the rounding unit: 10^exponent determines the precision level - // e.g. exponent=2 means round to the nearest 100 (cents to dollars for precision 2) - reminder := int64(math.Pow(10, float64(exponent))) + // Calculate the rounding unit using integer exponentiation: 10^exponent determines the precision level + // e.g. exponent=2 means round to the nearest 100 (cents to dollars for precision 2) + remainder := int64(1) + for i := 0; i < exponent; i++ { + remainder *= 10 + }Note: Also fixes the typo from "reminder" to "remainder" in subsequent references.
116-116: Typo: "reminder" should be "remainder".The variable name should be "remainder" to match standard mathematical terminology.
exchange/exchange.go (2)
89-117: Inconsistent error return values.The method returns
-1for a nil receiver (line 92) but0for a rate lookup error (line 101). This inconsistency could confuse callers who check the returned amount to detect errors. Consider returning a consistent sentinel value (e.g.,0) for all error paths, or document the differing return values.func (e *Exchange) ConvertAmount(amount int64, fromCurrencyCode string, fromFraction int, toCurrencyCode string, toFraction int) (int64, error) { if e == nil { - return -1, exception.ErrInvalidExchangeRate + return 0, exception.ErrInvalidExchangeRate }
119-137: Consider extracting shared conversion logic.The conversion calculation (lines 126-134) duplicates the logic in
ConvertAmount(lines 106-114). Extracting this to a private helper would reduce duplication and centralise the rounding behaviour.// convertWithRate performs the actual conversion calculation func convertWithRate(amount int64, fromFraction, toFraction int, rate float64) int64 { fromFractionPow := math.Pow10(fromFraction) toFractionPow := math.Pow10(toFraction) majorUnits := float64(amount) / fromFractionPow convertedMajorUnits := majorUnits * rate return int64(math.Round(convertedMajorUnits * toFractionPow)) }currency/dataset.go (1)
11-27: Fix symbol ordering to prevent partial matching issues.The comment states symbols are "ordered to prevent partial matching issues," but the ordering is inconsistent.
HK$(3 characters, line 19) appears afterS$(2 characters, line 18), violating the longest-first principle needed for greedy matching.Apply this diff to fix the ordering:
func GetSymbols() []Symbol { return []Symbol{ {"CFP", XPF}, // CFP Franc (XPF) - avoids ambiguity with "₣" {"NT$", TWD}, // Taiwan Dollar + {"HK$", HKD}, {"R$", BRL}, {"C$", CAD}, {"A$", AUD}, {"S$", SGD}, - {"HK$", HKD}, {"$", USD}, {"€", EUR}, {"£", GBP}, {"¥", JPY}, {"₹", INR}, {"₣", CHF}, } }currency/map.go (1)
139-151: Fix incorrect sync.Once usage in GetCodes.The
sync.Onceis declared as a local variable within the method, which defeats its purpose. Each call toGetCodes()creates a newoncevariable, so the initialization function runs every time instead of once. Additionally, this method returns a pointer to a local slice, which could lead to unexpected behaviour if callers modify it.Apply this diff to fix the implementation:
-// GetCodes returns a list of all currency codes present in the map. -func (cm Map) GetCodes() *[]string { - var once sync.Once - codes := make([]string, 0, len(*cm.dataset)) - - once.Do(func() { - for code := range *cm.dataset { - codes = append(codes, code) - } - }) - - return &codes +// GetCodes returns a list of all currency codes present in the map. +func (cm Map) GetCodes() []string { + if cm.dataset == nil { + return []string{} + } + + codes := make([]string, 0, len(*cm.dataset)) + for code := range *cm.dataset { + codes = append(codes, code) + } + + return codes }currency/iso.go (1)
38-61: Consider wrapping the error from NewCurrenciesMapFrom.At line 50, when
NewCurrenciesMapFromreturns an error, the code replaces it withexception.ErrNoCurrencyMapDataset. This may mask the underlying cause of the failure, making debugging more difficult.Apply this diff to preserve the original error context:
currencies, err := NewCurrenciesMapFrom(dataset) if err != nil { - return nil, exception.ErrNoCurrencyMapDataset + return nil, fmt.Errorf("failed to create currency map: %w", err) }Import
fmtif not already present.currency/manager.go (2)
78-91: Unnecessary ToUpper on numeric code.At line 85,
strings.ToUpper(code)is called on the numeric code parameter. Since ISO 4217 numeric codes consist only of digits (e.g., "702" for SGD), the ToUpper operation has no effect and can be removed for clarity.Apply this diff:
func (cm *Manager) FindByNumericCode(code string) *Currency { if cm == nil || cm.currencies == nil { return nil } for _, currency := range *cm.currencies.dataset { - if currency != nil && currency.NumericCode == strings.ToUpper(code) { + if currency != nil && currency.NumericCode == code { return currency } } return nil }
112-129: Unnecessary ToUpper on NumericCode.At line 124,
strings.ToUpper(NumericCode)is called when constructing the Currency. Since numeric codes are purely numeric, this operation has no effect. Consider removing it for clarity.Apply this diff:
currency := &Currency{ Code: strings.ToUpper(code), Grapheme: Grapheme, Template: Template, Decimal: Decimal, Thousand: Thousand, - NumericCode: strings.ToUpper(NumericCode), + NumericCode: NumericCode, Fraction: Fraction, }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (30)
calculator/calculator.go(7 hunks)currency/codes.go(1 hunks)currency/currency.go(2 hunks)currency/dataset.go(2 hunks)currency/iso.go(5 hunks)currency/iso_test.go(5 hunks)currency/manager.go(8 hunks)currency/map.go(4 hunks)currency/metal.go(2 hunks)currency/uat_test.go(20 hunks)exception/exception.go(2 hunks)exchange/converter.go(2 hunks)exchange/exchange.go(3 hunks)exchange/exchange_test.go(13 hunks)exchange/real_world_finance_test.go(1 hunks)format/format.go(2 hunks)format/format_test.go(1 hunks)money/aggregator_test.go(1 hunks)money/db_test.go(11 hunks)money/exchange_test.go(1 hunks)money/factory.go(1 hunks)money/json.go(1 hunks)money/json_test.go(13 hunks)money/manager.go(2 hunks)money/manager_test.go(31 hunks)money/money.go(14 hunks)money/money_test.go(40 hunks)money/test_helpers_test.go(2 hunks)parser/parser.go(3 hunks)parser/parser_test.go(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (15)
money/exchange_test.go (7)
money/manager.go (1)
NewManager(21-27)currency/manager.go (1)
NewManager(16-28)exchange/exchange.go (1)
NewExchange(18-22)exchange/real_world_finance_test.go (8)
USD(16-16)EUR(17-17)GBP(18-18)JPY(19-19)CNY(20-20)CAD(21-21)AUD(22-22)CHF(23-23)money/type.go (2)
Money(12-15)Amount(9-9)currency/currency.go (1)
Currency(12-27)exception/exception.go (1)
ErrCurrencyConversionNotFound(12-12)
money/aggregator_test.go (4)
money/manager.go (1)
NewManager(21-27)currency/manager.go (1)
NewManager(16-28)exception/exception.go (1)
ErrInvalidAggregatorProvider(40-40)money/aggregator.go (1)
Aggregator(8-10)
currency/iso_test.go (5)
currency/iso.go (1)
NewISOCodePatternWith(39-61)currency/currency.go (1)
Currency(12-27)exception/exception.go (2)
ErrNoConverterProvided(29-29)ErrNoCurrencyMapDataset(15-15)currency/codes.go (1)
SGD(131-131)currency/dataset.go (1)
Symbol(4-7)
currency/dataset.go (1)
currency/codes.go (6)
XPF(174-174)TWD(149-149)BRL(25-25)CAD(31-31)AUD(12-12)SGD(131-131)
currency/uat_test.go (4)
currency/currency.go (1)
Currency(12-27)currency/codes.go (1)
SGD(131-131)format/format.go (1)
Formatter(10-16)currency/manager.go (1)
NewManager(16-28)
money/json_test.go (4)
money/manager.go (1)
NewManager(21-27)currency/codes.go (1)
SGD(131-131)currency/currency.go (1)
Currency(12-27)money/type.go (2)
Amount(9-9)Money(12-15)
parser/parser_test.go (3)
currency/codes.go (3)
SGD(131-131)GBP(56-56)EUR(53-53)parser/parser.go (1)
Parser(11-13)exception/exception.go (1)
ErrParserNotProvided(44-44)
exchange/real_world_finance_test.go (2)
exchange/exchange.go (1)
NewExchange(18-22)exception/exception.go (1)
ErrCurrencyConversionNotFound(12-12)
money/manager_test.go (3)
currency/codes.go (1)
SGD(131-131)exception/exception.go (6)
ErrEmptyAmountString(31-31)ErrInvalidAmountMultiple(32-32)ErrInvalidAmountFraction(33-33)ErrInvalidAmount(34-34)ErrParserNotProvided(44-44)ErrNoMultipliersProvided(27-27)money/manager.go (1)
NewManager(21-27)
money/db_test.go (4)
money/manager.go (1)
NewManager(21-27)currency/manager.go (1)
NewManager(16-28)currency/codes.go (1)
SGD(131-131)currency/currency.go (1)
Currency(12-27)
exchange/exchange_test.go (2)
exception/exception.go (2)
ErrInvalidExchangeRate(18-18)ErrCurrencyConversionNotFound(12-12)exchange/exchange.go (1)
Exchange(12-15)
parser/parser.go (1)
currency/iso.go (1)
NewISOCodePattern(24-36)
currency/manager.go (3)
currency/map.go (1)
Map(13-16)currency/dataset.go (1)
Symbol(4-7)currency/currency.go (1)
Currency(12-27)
money/money_test.go (3)
currency/codes.go (3)
SGD(131-131)JPY(76-76)EUR(53-53)currency/currency.go (1)
Currency(12-27)currency/manager.go (1)
NewManager(16-28)
calculator/calculator.go (1)
calculator/ration.go (2)
SafeAdd(10-16)Ration(37-87)
🔇 Additional comments (50)
exception/exception.go (1)
8-8: Sentinel errors doc comment is clear and consistentThe comment concisely documents the purpose of the following error block and matches the package’s current structure. No changes needed.
calculator/calculator.go (1)
58-65: Allocation logic is sound.The method correctly guards against nil calculator and zero scale/amount. The division on line 64 could theoretically overflow if
Ration(amount, ration) == math.MinInt64andscale == -1, but this is an unlikely edge case in typical allocation scenarios.exchange/exchange.go (1)
17-22: LGTM!The constructor documentation and implementation are correct.
exchange/converter.go (2)
12-21: LGTM!The constructor properly validates the input and returns an appropriate error when the exchange is nil.
23-34: LGTM!Good defensive programming with nil-receiver and nil-exchange checks. The error types are appropriately differentiated.
format/format_test.go (1)
18-18: LGTM!Comment correctly updated to reflect the SGD reference currency used throughout the test suite.
money/manager.go (2)
52-52: LGTM!Documentation example correctly updated to use SGD.
71-72: LGTM!Documentation examples correctly updated to use SGD for consistency.
currency/codes.go (1)
3-4: LGTM!Good addition of a descriptive header comment for the ISO 4217 currency codes constant block.
money/aggregator_test.go (1)
38-54: LGTM! Test currency update is consistent.The switch from USD to SGD in these error path tests is consistent with the broader codebase changes. Since both currencies have identical fraction values (2 decimal places), the test behaviour remains unchanged.
money/factory.go (1)
7-903: LGTM! Factory pattern is well-structured.The singleton
defaultManagerand factory functions provide a clean, convenient API for creating Money instances. The pattern is consistent across all ~150 currency factory functions, and the Manager type appears designed for concurrent use based onTestManagerConcurrencyin manager_test.go.currency/iso_test.go (1)
36-146: LGTM! Test updates are consistent with SGD migration.All test cases correctly updated to use SGD instead of USD. The tests verify pattern matching, constructor validation, and symbol sorting with SGD-specific data, maintaining full test coverage.
parser/parser.go (2)
15-27: LGTM! Constructors follow idiomatic Go patterns.The
NewParser()andNewParserWith()constructors provide both convenience (default) and flexibility (custom ISO pattern) for creating Parser instances. The implementation is clean and well-documented.
234-245: LGTM! Documentation updated for clarity.The comment updates to reference SGD instead of USD improve consistency with the codebase migration whilst keeping the examples clear.
money/json.go (1)
16-21: LGTM! Precision-preserving JSON structure is well-designed.The
JSONRawDatastruct usingjson.Numberfor the amount field is a good pattern to avoid float64's 2^53 precision limit when unmarshalling large monetary values. The documentation clearly explains its purpose.format/format.go (2)
9-27: LGTM! Documentation improvements enhance clarity.The comment updates provide clearer context, and the NewFormatter comment accurately describes its purpose.
59-67: LGTM! ToMajorUnits implementation is correct.The method correctly converts minor currency units to major units by dividing by 10^Fraction. The edge case where Fraction is 0 (e.g., JPY) is handled properly by returning the amount unchanged.
money/manager_test.go (1)
54-722: LGTM! Comprehensive test updates maintain full coverage.All test cases correctly updated from USD to SGD. Since both currencies use 2 decimal places (Fraction: 2), the arithmetic assertions remain valid. Error path testing, concurrency testing, and edge case handling are all preserved with the currency switch.
currency/currency.go (1)
40-47: LGTM! Equals method is correctly implemented.The nil-safe equality check correctly compares currencies by their Code field. The implementation properly handles nil receivers and nil arguments, returning true only when both are nil.
money/json_test.go (1)
33-33: LGTM! Test currency consistently updated to SGD.All test cases have been systematically updated from USD to SGD whilst maintaining the same test coverage and logic. The changes are consistent across marshal, unmarshal, thread-safety, and parser tests.
Also applies to: 56-56, 75-77, 144-144, 156-156, 175-175, 294-294, 360-360, 377-377, 395-396, 456-456, 465-465, 493-493, 499-499, 508-508, 527-528
currency/map.go (3)
96-108: LGTM! IsEmpty and IsNotEmpty helpers are well-implemented.These helper methods provide clear, expressive checks for map state with proper nil handling.
110-123: LGTM! HasInvalidState provides useful validation.The method correctly checks for nil dataset and iterates through entries to detect nil currency pointers, returning appropriate errors.
125-137: LGTM! FindByCode implements case-insensitive lookup correctly.The method safely handles nil dataset and performs case-insensitive lookup using strings.ToUpper, which is appropriate for ISO currency codes.
currency/uat_test.go (1)
93-93: LGTM! Test data systematically updated to SGD.All test cases have been correctly updated to use SGD as the reference currency, including:
- Currency code updates (USD → SGD)
- Numeric code updates ("840" → "702")
- Symbol updates ("$" → "S$")
The changes maintain test coverage whilst aligning with the new default currency throughout the codebase.
Also applies to: 133-175, 302-309, 349-374, 406-419, 437-453, 464-471, 486-520, 626-653, 697-697, 724-726
parser/parser_test.go (1)
25-29: LGTM! Parser tests consistently updated to SGD.All parser test cases have been updated to use SGD as the test currency, including:
- Symbol parsing tests (S$ prefix/suffix)
- Comma-separated amount tests
- Negative amount tests
- Default currency tests
The test logic remains sound with the currency change.
Also applies to: 43-52, 225-226, 230-230, 545-575, 671-681, 734-739
exchange/exchange_test.go (1)
23-23: LGTM! Exchange tests consistently updated to SGD.All exchange rate tests have been systematically updated from USD to SGD, including:
- Rate addition tests
- Rate retrieval tests (direct and inverse)
- Amount conversion tests
- Concurrent access tests
- Test descriptions
The changes maintain comprehensive test coverage for exchange functionality.
Also applies to: 36-36, 80-95, 101-151, 155-209, 325-333, 376-376
money/test_helpers_test.go (5)
27-37: LGTM! Power of 10 helper is straightforward.Simple and correct implementation for computing 10^exp as int64.
39-47: LGTM! Absolute value helper is correct.Straightforward implementation of absolute value for int64.
49-60: LGTM! Float parsing helper uses big.Rat for precision.Using
big.Rat.SetString()for parsing provides accurate rational representation before converting to float64, which is appropriate for test expectations.
62-79: LGTM! Conversion calculation helper uses precise rational arithmetic.The helper correctly computes expected conversion amounts using
big.Ratfor precise arithmetic:
- Multiplies amount by rate
- Scales by target fraction (toFraction)
- Divides by source fraction (fromFraction)
- Rounds using half-away-from-zero
This approach ensures accurate test expectations for monetary conversions.
81-110: LGTM! Rounding implementation follows half-away-from-zero correctly.The rounding logic correctly implements half-away-from-zero rounding:
- Handles zero case
- Separates sign handling
- Uses quotient and remainder for rounding decision
- Compares
2×remainderagainst denominator for midpoint check- Restores sign after rounding
This is appropriate for monetary calculations where consistent rounding behaviour is essential.
money/exchange_test.go (2)
635-734: LGTM! Comprehensive real-world conversion test with precise expectations.This test provides excellent coverage of real-world conversion scenarios:
- Various currency pairs (USD, EUR, GBP, JPY, CNY, CAD, AUD, CHF)
- Different amount sizes (invoices, purchases, salaries)
- Negative amounts (refunds)
- Inverse rates (JPY to USD using "1/150")
- Tolerance checks for rounding differences
The use of
mustParseFloatMoneyandexpectedConvertAmountMoneyensures precise test expectations with proper handling of fractional differences across currencies.
736-751: LGTM! Test correctly verifies no implicit cross-rate calculation.This test appropriately verifies that the exchange system doesn't automatically compute cross-rates. When only USD→EUR and USD→JPY rates are available, attempting EUR→JPY conversion correctly fails with
ErrCurrencyConversionNotFound.This behaviour is important for systems that require explicit rate configuration.
currency/iso.go (1)
63-83: LGTM!The greedy matching strategy is correctly implemented using stable sort by descending length with alphabetical tie-breaking. This ensures longer symbols are matched first, preventing issues like "$" matching before "US$".
money/db_test.go (1)
9-408: LGTM!All test cases have been systematically updated to use SGD as the reference currency. The updates are consistent throughout the file, maintain existing test logic, and include proper coverage of Value(), Scan(), separator handling, and thread safety scenarios.
currency/metal.go (2)
3-11: LGTM!The Provider interface is well-designed with clear method responsibilities. It provides a clean abstraction for currency provisioning.
13-37: The SGD metadata in DefaultProvider is accurate. ISO 4217 specifies the numeric code as 702 and minor unit (fraction) as 2, both of which are correctly implemented. The decimal separator (.), thousands separator (,), grapheme (S$), and template ($1) are reasonable defaults for SGD, though ISO 4217 does not mandate specific formatting choices—these are locale/style dependent.exchange/real_world_finance_test.go (4)
37-145: LGTM!The test cases provide comprehensive coverage of real-world currency conversion scenarios, including:
- Multiple currency pairs with different fractional digits
- Negative amounts
- Zero-decimal currencies (JPY)
- Appropriate tolerance for floating-point precision
The acknowledgement of the JPY rounding boundary (lines 75-77) demonstrates careful consideration of numerical precision.
148-167: LGTM!The round-trip test effectively validates inverse rate path functionality with appropriate tolerance for precision loss during bidirectional conversion.
169-180: LGTM!This test correctly validates that the exchange system does not automatically chain conversion rates (EUR → USD → JPY), which is important for predictable behaviour and avoiding compounding conversion errors.
182-254: LGTM!The helper functions are well-implemented:
- Use of
big.Ratfor expected value calculations ensures precise arithmetic without floating-point errorsroundRatHalfAwayFromZerocorrectly implements the rounding strategy- All helpers are appropriately scoped and well-tested through their usage
money/money_test.go (1)
16-950: LGTM!All test cases have been comprehensively and consistently updated to use SGD as the reference currency throughout the file. The updates include:
- Constructor tests
- Comparison and arithmetic operations
- JSON marshalling/unmarshalling
- Display formatting (correctly updated to "S$" prefix)
- Edge case and error path coverage
The test logic remains sound and maintains thorough coverage of the Money API.
currency/manager.go (2)
15-67: LGTM!The constructor family provides flexible initialization options:
NewManager()for default usageNewManagerWith()for custom providersNewManagerFor()for provider + dataset with proper validationError handling is appropriate, and the fallback to DefaultProvider when
curris nil (line 50) provides sensible default behaviour.
131-160: LGTM!The getter methods and
Resolve()are well-implemented with:
- Consistent nil-safety checks
- Clear fallback behaviour in
Resolve()- Simple, focused responsibilities
money/money.go (6)
8-24: LGTM!The getter methods
Currency()andAmount()are cleanly implemented with proper nil-checking viaensureMoneyProvided().
26-59: LGTM!The currency comparison methods are well-designed:
SameCurrency()provides boolean checkAssertSameCurrency()provides error-based validation- Proper reuse avoiding duplication
- Comprehensive nil-checking
61-76: LGTM!
CompareAmount()correctly implements three-way comparison with proper currency validation.
78-131: LGTM!The comparison predicate methods (
Equals,GreaterThan,GreaterThanOrEqual,LessThan,LessThanOrEqual) are consistently implemented by delegating toCompareAmount(), ensuring correct behaviour and good code reuse.
133-172: LGTM!The sign check methods (
IsZero,IsPositive,IsNegative) andDisplay()are correctly implemented with proper validation. TheDisplay()method appropriately delegates formatting to the currency's formatter.
174-237: LGTM!The remaining methods are correctly implemented:
Compare()properly handles currency mismatch by returning the amount value (as documented)AsMajorUnits()correctly converts to floating-point representation- JSON methods appropriately delegate to the JSON parser
ensureMoneyProvided()provides robust validation for nil receiver and nil currency
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
exchange/exchange_test.go(15 hunks)money/aggregator_test.go(2 hunks)money/db_test.go(13 hunks)money/exchange_test.go(14 hunks)money/factory_test.go(2 hunks)money/json_test.go(21 hunks)money/manager_test.go(25 hunks)money/money_test.go(37 hunks)money/test_helpers_test.go(0 hunks)support/tests.go(1 hunks)
💤 Files with no reviewable changes (1)
- money/test_helpers_test.go
🧰 Additional context used
🧬 Code graph analysis (6)
money/db_test.go (6)
money/manager.go (1)
NewManager(21-27)support/tests.go (1)
TestRequire(8-17)calculator/calculator.go (1)
Amount(6-6)money/type.go (1)
Amount(9-9)currency/currency.go (1)
Currency(12-27)currency/codes.go (1)
SGD(131-131)
money/manager_test.go (3)
currency/codes.go (1)
SGD(131-131)support/tests.go (2)
TestRequire(8-17)TestRequireNoErr(19-25)exception/exception.go (5)
ErrEmptyAmountString(31-31)ErrInvalidAmountMultiple(32-32)ErrInvalidAmountFraction(33-33)ErrInvalidAmount(34-34)ErrParserNotProvided(44-44)
money/factory_test.go (3)
support/tests.go (1)
TestRequire(8-17)money/type.go (1)
Amount(9-9)currency/currency.go (1)
Currency(12-27)
money/exchange_test.go (4)
support/tests.go (5)
TestRequire(8-17)TestRequireNoErr(19-25)TestMustParseFloatMoney(49-60)TestExpectedConvertAmountMoney(62-79)TestAbs64Money(39-47)calculator/calculator.go (1)
Amount(6-6)exchange/exchange.go (1)
NewExchange(18-22)exception/exception.go (1)
ErrCurrencyConversionNotFound(12-12)
money/money_test.go (3)
currency/codes.go (1)
SGD(131-131)support/tests.go (2)
TestRequire(8-17)TestRequireNoErr(19-25)exception/exception.go (5)
ErrCurrencyMismatch(10-10)ErrInvalidJSONUnmarshal(17-17)ErrNoMoneyProvided(23-23)ErrInvalidExchangeRate(18-18)ErrNoJSONParserProvided(19-19)
exchange/exchange_test.go (3)
exception/exception.go (2)
ErrInvalidExchangeRate(18-18)ErrCurrencyConversionNotFound(12-12)exchange/exchange.go (2)
Exchange(12-15)NewExchange(18-22)support/tests.go (4)
TestRequireNoErr(19-25)TestMustParseFloatMoney(49-60)TestExpectedConvertAmountMoney(62-79)TestAbs64Money(39-47)
🔇 Additional comments (22)
support/tests.go (2)
8-17: LGTM!
TestRequireandTestRequireNoErrare well-implemented generic test helpers with correct use oft.Helper()for proper stack trace reporting.Also applies to: 19-25
62-79: LGTM!
TestExpectedConvertAmountMoneycorrectly performs currency conversion using arbitrary precision rational arithmetic, handling fractional digit scaling properly before rounding.money/factory_test.go (1)
7-7: LGTM!The migration to
support.TestRequireis consistent with the new test helper utilities and correctly unwraps the(T, error)return types fromm.Amount()andm.Currency().Also applies to: 202-202, 207-207
money/aggregator_test.go (2)
10-10: LGTM!The migration to
support.TestRequireNoErrfor validatingAddRatecalls is consistent with the new test helper utilities.Also applies to: 15-15
39-55: LGTM!The currency code change from USD to SGD aligns with the broader test data standardisation across the repository. The tests correctly verify nil aggregator behaviour regardless of which currency is used.
money/db_test.go (1)
7-7: LGTM!The migration to SGD as the default test currency and adoption of
support.TestRequire/support.TestRequireNoErris consistent across all test cases. The changes maintain the same test coverage and assertions.Also applies to: 11-11, 18-18, 34-36, 48-50, 62-62, 67-67, 82-82, 119-119, 126-127, 142-143, 154-155, 252-252, 258-258, 304-304
money/manager_test.go (6)
11-11: LGTM!Import of the support package is correctly added for the new test helper utilities.
59-113: LGTM!
TestManagerCreatecorrectly migrated to use SGD as the primary test currency andsupport.TestRequirefor assertions. The test maintains comprehensive coverage of currency creation scenarios.
116-149: LGTM!
TestManagerCreateFromFloatandTestManagerCreateFromStringtests are well-structured with comprehensive edge cases including trailing decimals, negative values, and various decimal place configurations. The migration to SGD and support helpers is consistent.Also applies to: 151-212
343-433: LGTM!Arithmetic operation tests (
TestManagerAdd,TestManagerSubtract) correctly validate same-currency operations with SGD and properly test the error path for different currencies (USD/EUR).
529-555: Verify rounding expectations for SGD.The test comment on Line 532 says "rounds SGD amount" and expects 1234 → 1200. Ensure this matches the actual rounding behaviour of the
Roundmethod for a 2-decimal-place currency. The expected values suggest rounding to the nearest 100 minor units.
703-722: LGTM!Error path tests for
CreateFromStringcorrectly verify specific error types usingerrors.Is(). The nil parser test properly validates theErrParserNotProvidederror.money/json_test.go (5)
12-12: LGTM!The import of the support package and migration to
support.TestRequireNoErrforSetMarshalvalidation is consistent with the new test utilities.Also applies to: 32-34
51-79: LGTM!
TestJSONSetUnmarshalcorrectly tests custom unmarshalling with SGD as the target currency. The assertions usingsupport.TestRequireproperly validate both amount and currency code.
145-161: LGTM!Thread safety tests correctly updated to use SGD currency in both marshal and unmarshal operations. The concurrent goroutine tests maintain proper race condition coverage.
383-399: LGTM!Good test coverage verifying that when all custom JSON parser functions are nil, the default currency resolves to SGD. This ensures backward compatibility with the default behaviour.
490-531: LGTM!Comprehensive coverage of JSON unmarshal edge cases including decimal rounding (positive and negative), overflow detection, unknown currency handling, and default currency fallback to SGD.
money/money_test.go (1)
12-12: LGTM! Excellent refactoring to standardise test currency and error handling.The migration from USD to SGD and the adoption of support package helpers (TestRequire, TestRequireNoErr) is consistent throughout. This refactoring improves test maintainability by centralising error-handling patterns and provides a uniform currency context across the test suite.
Also applies to: 18-952
money/exchange_test.go (2)
11-11: LGTM! Consistent adoption of support package helpers.The migration to support.TestRequire and support.TestRequireNoErr is consistent throughout the existing tests.
Also applies to: 34-590
636-752: Excellent additions! Real-world conversion scenarios strengthen test coverage.The new tests provide valuable coverage:
TestConverter_RealWorld_CommonCurrenciesexercises multiple currency pairs with fixed fixture rates and tolerance handling for floating-point precisionTestConverter_RealWorld_NoImplicitCrossRateverifies that cross-rate conversions (EUR→JPY) correctly fail when only USD→EUR and USD→JPY rates exist, confirming the library doesn't automatically chain conversionsBoth tests properly use parallel execution and the support helpers for deterministic decimal rate parsing.
exchange/exchange_test.go (2)
10-10: LGTM! Consistent refactoring with support helpers and SGD context.The updates to use support package helpers and migrate some test fixtures to SGD are consistent with the broader test suite changes.
Also applies to: 24-540
542-711: Outstanding test additions! Comprehensive real-world scenarios at the exchange layer.Three valuable new tests strengthen the exchange conversion logic:
TestExchange_ConvertAmount_RealWorldCommonCurrencies(lines 542-677): Covers 8 real-world scenarios including invoices, salaries, card purchases, and negative refunds. Properly accounts for floating-point precision with tolerance bounds (e.g.,wantTol: 1for JPY conversions at x.5 boundaries).
TestExchange_ConvertAmount_UsesInverseRatePath_RoundTripBounded(lines 679-698): Verifies that USD→EUR→USD round-trip conversions via inverse rates stay within 1 cent, confirming the inverse rate calculation maintains acceptable precision.
TestExchange_ConvertAmount_DoesNotAssumeChainedCrossRates(lines 700-711): Confirms the exchange does not automatically chain rates (EUR→JPY fails when only USD→EUR and USD→JPY rates exist), preventing unexpected transitive conversions.All tests properly leverage parallel execution and support.TestMustParseFloatMoney for deterministic fixture rates.
No description provided.