Add solution for Challenge 7 by shansing#1435
Conversation
WalkthroughAdds a new Go file implementing a concurrency-safe BankAccount type with ID, Owner, Balance, MinBalance, a MaxTransactionAmount constant, custom error types, a NewBankAccount constructor, and Deposit/Withdraw/Transfer methods using mutexes and deterministic lock ordering and validations. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Source as BankAccount (Source)
participant Target as BankAccount (Target)
Client->>Source: Transfer(amount, target)
Source->>Source: validate amount & limits
alt Lock ordering by ID
Source->>Source: Lock mu (lower ID first)
Source->>Target: Lock target.mu (higher ID second)
end
Source->>Source: compute new source balance
Source->>Target: compute new target balance
Source->>Source: validate balances (min, max)
alt success
Source->>Source: apply source balance
Source->>Target: apply target balance
Source->>Source: unlock mu
Source->>Target: unlock target.mu
Source-->>Client: return nil
else failure
Source->>Source: unlock mu
Source->>Target: unlock target.mu
Source-->>Client: return error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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
🧹 Nitpick comments (2)
challenge-7/submissions/shansing/solution-template.go (2)
127-143: Remove duplicate unreachable validations inWithdrawandTransfer.
amountis already validated before locking. Re-checking it later adds dead code and maintenance noise.♻️ Proposed cleanup
func (a *BankAccount) Withdraw(amount float64) error { @@ if err := checkBalance(newBalance, a.MinBalance); err != nil { return err } - if amount > MaxTransactionAmount { - return &ExceedsLimitError{} - } a.Balance = newBalance return nil } @@ a.Balance = aBalance target.Balance = targetBalance - if amount > MaxTransactionAmount { - return &ExceedsLimitError{} - } - if amount < 0 { - return &NegativeAmountError{} - } return nil }Also applies to: 180-185
18-19: Clean up commented-out scaffolding before merge.Commented code and duplicated placeholder text (Line 107) make the solution harder to read and review.
Also applies to: 73-75, 95-102, 107-107
Refactor locking mechanism for account transactions.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
challenge-7/submissions/shansing/solution-template.go (1)
86-92: VariableBankAccountshadows the type name.Using the same identifier for the variable and the type can cause confusion. Consider renaming to
accountorbankAccount.♻️ Suggested rename
- BankAccount := &BankAccount{ + account := &BankAccount{ ID: id, Owner: owner, Balance: balance, MinBalance: minBalance, } - return BankAccount, nil + return account, nil
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
challenge-7/submissions/shansing/solution-template.go (3)
161-172:⚠️ Potential issue | 🔴 CriticalGuard
targetbefore dereferencing to prevent panic.Line 161 reads
target.IDbefore verifyingtarget != nil;Transfer(..., nil)will panic.🐛 Proposed fix
func (a *BankAccount) Transfer(amount float64, target *BankAccount) error { // Implement transfer functionality with proper error handling + if target == nil { + return &AccountError{} + } if amount > MaxTransactionAmount { return &ExceedsLimitError{} }
191-200:⚠️ Potential issue | 🟠 Major
checkBalanceis mixing balance validity with transaction-limit policy.
balance > MaxTransactionAmountat Line 198 makes account state invalid based on total balance, not transaction amount. This incorrectly affects constructor, deposit, and transfer target validations.✅ Proposed fix
func checkBalance(balance float64, minBalance float64) error { if balance < 0 { return &NegativeAmountError{} } if balance < minBalance { return &InsufficientFundsError{} } - if balance > MaxTransactionAmount { - return &ExceedsLimitError{} - } return nil }
141-147:⚠️ Potential issue | 🟡 MinorRemove duplicated checks/assignments (dead code).
There are duplicated
checkBalance/return and duplicated balance assignments. These are no-op repeats and increase bug surface.🧹 Proposed cleanup
func (a *BankAccount) Withdraw(amount float64) error { @@ newBalance := a.Balance - amount if err := checkBalance(newBalance, a.MinBalance); err != nil { return err } - if err := checkBalance(newBalance, a.MinBalance); err != nil { - return err - } a.Balance = newBalance return nil - a.Balance = newBalance - return nil } @@ a.Balance = aBalance target.Balance = targetBalance - a.Balance = aBalance - target.Balance = targetBalance return nil }Also applies to: 182-185
There was a problem hiding this comment.
♻️ Duplicate comments (1)
challenge-7/submissions/shansing/solution-template.go (1)
148-168:⚠️ Potential issue | 🔴 CriticalNil pointer dereference:
targetaccessed before validation.Line 156 accesses
target.IDwithout first checking iftargetis nil. This will cause a panic ifTransferis called with a nil target.🐛 Proposed fix
func (a *BankAccount) Transfer(amount float64, target *BankAccount) error { // Implement transfer functionality with proper error handling + if target == nil { + return &AccountError{} + } if amount > MaxTransactionAmount { return &ExceedsLimitError{} }
🧹 Nitpick comments (2)
challenge-7/submissions/shansing/solution-template.go (2)
86-92: Variable name shadows type name.The local variable
BankAccountshadows the struct typeBankAccount. This compiles but can cause confusion and makes the code harder to read.♻️ Suggested fix
- BankAccount := &BankAccount{ + account := &BankAccount{ ID: id, Owner: owner, Balance: balance, MinBalance: minBalance, } - return BankAccount, nil + return account, nil
95-102: Consider removing commented-out code.The commented
getAccountByIdfunction appears to be unused remnant code. If not needed for reference, removing it improves readability.
1 participant
Challenge 7 Solution
Submitted by: @shansing
Challenge: Challenge 7
Description
This PR contains my solution for Challenge 7.
Changes
challenge-7/submissions/shansing/solution-template.goTesting
Thank you for reviewing my submission! 🚀