Add solution for Challenge 7 by livingpool#1882
Conversation
WalkthroughAdded a complete Challenge 7 Go bank-account implementation with custom errors, constructor validation, transaction limits, mutex-protected deposits and withdrawals, and account transfers. 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: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: eead8216-ccb1-440c-a58c-89c15b2c0103
📒 Files selected for processing (1)
challenge-7/submissions/livingpool/solution-template.go
| func (e *InsufficientFundsError) Error() string { | ||
| // Implement error message | ||
| return "" | ||
| } | ||
|
|
||
| // NegativeAmountError occurs when an amount for deposit, withdrawal, or transfer is negative. | ||
| type NegativeAmountError struct { | ||
| // Implement this error type | ||
| } | ||
|
|
||
| func (e *NegativeAmountError) Error() string { | ||
| // Implement error message | ||
| return "" | ||
| } | ||
|
|
||
| // ExceedsLimitError occurs when a deposit or withdrawal amount exceeds the defined limit. | ||
| type ExceedsLimitError struct { | ||
| // Implement this error type | ||
| } | ||
|
|
||
| func (e *ExceedsLimitError) Error() string { | ||
| // Implement error message | ||
| return "" | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Empty Error() strings defeat the purpose of the error interface.
InsufficientFundsError, NegativeAmountError, and ExceedsLimitError all return "", so logging or wrapping these errors yields no diagnostic text. The tests only inspect the concrete type via %T, which masks this, but any real caller printing the error gets a blank message. Give each a meaningful message (following the AccountError pattern).
♻️ Example implementation
func (e *InsufficientFundsError) Error() string {
- // Implement error message
- return ""
+ return "insufficient funds: withdrawal would bring balance below minimum"
} func (e *NegativeAmountError) Error() string {
- // Implement error message
- return ""
+ return "amount must not be negative"
} func (e *ExceedsLimitError) Error() string {
- // Implement error message
- return ""
+ return fmt.Sprintf("amount exceeds transaction limit of %.2f", MaxTransactionAmount)
}📝 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.
| func (e *InsufficientFundsError) Error() string { | |
| // Implement error message | |
| return "" | |
| } | |
| // NegativeAmountError occurs when an amount for deposit, withdrawal, or transfer is negative. | |
| type NegativeAmountError struct { | |
| // Implement this error type | |
| } | |
| func (e *NegativeAmountError) Error() string { | |
| // Implement error message | |
| return "" | |
| } | |
| // ExceedsLimitError occurs when a deposit or withdrawal amount exceeds the defined limit. | |
| type ExceedsLimitError struct { | |
| // Implement this error type | |
| } | |
| func (e *ExceedsLimitError) Error() string { | |
| // Implement error message | |
| return "" | |
| } | |
| func (e *InsufficientFundsError) Error() string { | |
| return "insufficient funds: withdrawal would bring balance below minimum" | |
| } | |
| // NegativeAmountError occurs when an amount for deposit, withdrawal, or transfer is negative. | |
| type NegativeAmountError struct { | |
| // Implement this error type | |
| } | |
| func (e *NegativeAmountError) Error() string { | |
| return "amount must not be negative" | |
| } | |
| // ExceedsLimitError occurs when a deposit or withdrawal amount exceeds the defined limit. | |
| type ExceedsLimitError struct { | |
| // Implement this error type | |
| } | |
| func (e *ExceedsLimitError) Error() string { | |
| return fmt.Sprintf("amount exceeds transaction limit of %.2f", MaxTransactionAmount) | |
| } |
| if a.Balance-amount < a.MinBalance { | ||
| return &InsufficientFundsError{} | ||
| } | ||
| a.mu.Lock() | ||
| a.Balance -= amount | ||
| a.mu.Unlock() | ||
| return nil |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
TOCTOU race: balance check is outside the mutex.
Line 110 reads a.Balance without holding a.mu, then the update at 113–115 takes the lock. This is a data race against concurrent writers (flagged by go test -race), and the check-then-act gap allows two concurrent withdrawals to both pass the MinBalance guard and overdraw. Hold the lock across the check and the mutation.
🔒 Proposed fix
if amount > MaxTransactionAmount {
return &ExceedsLimitError{}
}
- if a.Balance-amount < a.MinBalance {
- return &InsufficientFundsError{}
- }
- a.mu.Lock()
- a.Balance -= amount
- a.mu.Unlock()
+ a.mu.Lock()
+ defer a.mu.Unlock()
+ if a.Balance-amount < a.MinBalance {
+ return &InsufficientFundsError{}
+ }
+ a.Balance -= amount
return nil📝 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.Balance-amount < a.MinBalance { | |
| return &InsufficientFundsError{} | |
| } | |
| a.mu.Lock() | |
| a.Balance -= amount | |
| a.mu.Unlock() | |
| return nil | |
| if amount > MaxTransactionAmount { | |
| return &ExceedsLimitError{} | |
| } | |
| a.mu.Lock() | |
| defer a.mu.Unlock() | |
| if a.Balance-amount < a.MinBalance { | |
| return &InsufficientFundsError{} | |
| } | |
| a.Balance -= amount | |
| return nil |
| func (a *BankAccount) Transfer(amount float64, target *BankAccount) error { | ||
| // Implement transfer functionality with proper error handling | ||
| if amount < 0 { | ||
| return &NegativeAmountError{} | ||
| } | ||
| if amount > MaxTransactionAmount { | ||
| return &ExceedsLimitError{} | ||
| } | ||
| if a.Balance-amount < a.MinBalance { | ||
| return &InsufficientFundsError{} | ||
| } | ||
| a.mu.Lock() | ||
| a.Balance -= amount | ||
| target.Balance += amount | ||
| a.mu.Unlock() | ||
| return nil |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '1,260p' challenge-7/submissions/livingpool/solution-template.goRepository: RezaSi/go-interview-practice
Length of output: 4049
🏁 Script executed:
rg -n --hidden --glob '!**/.git/**' -e 'Transfer' -e 'MinBalance' -e 'deadlock' -e 'concurr' challenge-7 .Repository: RezaSi/go-interview-practice
Length of output: 50385
🏁 Script executed:
sed -n '250,430p' challenge-7/solution-template_test.go && printf '\n--- README ---\n' && sed -n '1,120p' challenge-7/README.md && printf '\n--- learning ---\n' && sed -n '110,150p' challenge-7/learning.mdRepository: RezaSi/go-interview-practice
Length of output: 8909
Transfer should lock both accounts and guard nil targets. target can panic when nil, target.Balance is updated without target.mu, and the minimum-balance check runs outside the lock. Lock both accounts in a stable order before checking and updating.
|
🎉 Auto-merged! This PR was automatically merged after 2 days with all checks passing. Thank you for your contribution, @livingpool! 📊 Scoreboards and badges will be updated shortly. |
1 participant
Challenge 7 Solution
Submitted by: @livingpool
Challenge: Challenge 7
Description
This PR contains my solution for Challenge 7.
Changes
challenge-7/submissions/livingpool/solution-template.goTesting
Thank you for reviewing my submission! 🚀