Add solution for Challenge 7 by onomica#1884
Conversation
WalkthroughAdds a thread-safe Go ChangesBank account operations
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1c4c2429-a0f9-4f29-8a76-579631472695
📒 Files selected for processing (1)
challenge-7/submissions/onomica/solution-template.go
| func (a *BankAccount) Transfer(amount float64, target *BankAccount) error { | ||
| if a == target { | ||
| return nil | ||
| } | ||
|
|
||
| if err := validateAmount(amount); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| first, second := a, target | ||
| if first.ID > second.ID { | ||
| first, second = second, first | ||
| } | ||
|
|
||
| first.mu.Lock() | ||
| defer first.mu.Unlock() | ||
| second.mu.Lock() | ||
| defer second.mu.Unlock() | ||
|
|
||
| if a.Balance-amount < a.MinBalance { | ||
| return &InsufficientFundsError{ | ||
| Balance: a.Balance, | ||
| Amount: amount, | ||
| } | ||
| } | ||
|
|
||
| a.Balance -= amount | ||
| target.Balance += amount | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for Transfer-related tests in the challenge-7 test file
rg -n -C3 'TestTransfer' challenge-7/solution-template_test.goRepository: RezaSi/go-interview-practice
Length of output: 302
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the Transfer test and nearby helpers with line numbers
sed -n '240,360p' challenge-7/solution-template_test.go | cat -nRepository: RezaSi/go-interview-practice
Length of output: 4295
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for any other transfer-focused tests or self-transfer coverage
rg -n -C2 'self-transfer|Transfer\(|Concurrent.*Transfer|TestTransfer' challenge-7Repository: RezaSi/go-interview-practice
Length of output: 17520
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the Transfer implementation and the test file around any concurrency helpers
sed -n '1,220p' challenge-7/submissions/onomica/solution-template.go | cat -nRepository: RezaSi/go-interview-practice
Length of output: 4110
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for any dedicated self-transfer or concurrent transfer test cases
rg -n -C3 'self.?transfer|concurrent transfer|Transfer test|TestTransfer|go func\(\).*Transfer|\.Transfer\(' challenge-7/solution-template_test.go challenge-7Repository: RezaSi/go-interview-practice
Length of output: 2771
Add self-transfer and concurrency cases to TestTransfer. The current test table covers invalid amounts and insufficient funds, but not the a == target early return or concurrent transfers that exercise the lock ordering.
| if a == target { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Self-transfer skips amount validation.
The a == target early return at Line 115 happens before validateAmount at Line 119, so Transfer(-100, account) or Transfer(MaxTransactionAmount+1, account) returns nil instead of the expected NegativeAmountError/ExceedsLimitError. Move validation before the self-transfer check to keep the error contract consistent with Deposit and Withdraw.
🔧 Proposed fix
func (a *BankAccount) Transfer(amount float64, target *BankAccount) error {
+ if err := validateAmount(amount); err != nil {
+ return err
+ }
+
if a == target {
return nil
}
- if err := validateAmount(amount); err != nil {
- return err
- }
-
first, second := a, target📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if a == target { | |
| return nil | |
| } | |
| func (a *BankAccount) Transfer(amount float64, target *BankAccount) error { | |
| if err := validateAmount(amount); err != nil { | |
| return err | |
| } | |
| if a == target { | |
| return nil | |
| } | |
| first, second := a, target |
|
🎉 Auto-merged! This PR was automatically merged after 2 days with all checks passing. Thank you for your contribution, @onomica! 📊 Scoreboards and badges will be updated shortly. |
1 participant
Challenge 7 Solution
Submitted by: @onomica
Challenge: Challenge 7
Description
This PR contains my solution for Challenge 7.
Changes
challenge-7/submissions/onomica/solution-template.goTesting
Thank you for reviewing my submission! 🚀