Add new constructor that receives already resolved currency to avoid …#161
Add new constructor that receives already resolved currency to avoid …#161radykal-com wants to merge 2 commits into
Conversation
Reviewer's GuideAdds a new constructor that accepts an already-resolved Currency to avoid repeated currency lookup and formatting overhead, along with a unit test verifying behavior is equivalent to the existing constructor. Class diagram for Money with new NewWithCurrency constructorclassDiagram
class Money {
-int64 amount
-Currency* currency
+New(amount int64, code string) Money*
+NewWithCurrency(amount int64, currency Currency*) Money*
+NewFromFloat(amount float64, code string) Money*
}
class Currency {
<<struct>>
}
Money --> Currency : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Consider adding a defensive check (or at least a panic) if
NewWithCurrencyis called with a nilcurrency, so misuse is caught early instead of silently creating aMoneywith a nil currency pointer.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider adding a defensive check (or at least a panic) if `NewWithCurrency` is called with a nil `currency`, so misuse is caught early instead of silently creating a `Money` with a nil currency pointer.
## Individual Comments
### Comment 1
<location path="money_test.go" line_range="51" />
<code_context>
}
}
+func TestNewWithCurrency(t *testing.T) {
+ m := New(1, EUR)
+
</code_context>
<issue_to_address>
**suggestion (testing):** Expand `TestNewWithCurrency` to assert that the same Currency instance is reused, not just value equality
Currently the test only checks value equality, which would still pass if `NewWithCurrency` re-resolved the currency instead of using the provided instance. Please also assert that `m.currency` and `om.currency` refer to the same `*Currency` (or equivalent invariant) so a future refactor that reintroduces a lookup will be caught by this test.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| } | ||
| } | ||
|
|
||
| func TestNewWithCurrency(t *testing.T) { |
There was a problem hiding this comment.
suggestion (testing): Expand TestNewWithCurrency to assert that the same Currency instance is reused, not just value equality
Currently the test only checks value equality, which would still pass if NewWithCurrency re-resolved the currency instead of using the provided instance. Please also assert that m.currency and om.currency refer to the same *Currency (or equivalent invariant) so a future refactor that reintroduces a lookup will be caught by this test.
Hi @Rhymond,
This PR adds a new constructor that already receives the resolved currency.
Right now, all constructors have to allocate a temporary currency to just lookup if it exists (it also requires to scan the currency code and convert to uppercase if required). It becomes even worse when using the
NewFromFloatthat forces amath.Pow10for each call.With this new constructor, callers can implement currencies cache mechanisms to reduce this overhead.
Summary by Sourcery
Add a constructor that creates Money instances from an already resolved currency to avoid repeated currency lookups.
New Features:
Tests: