Add solution for Challenge 7 by kiramux #733
Conversation
WalkthroughAdds a new Go implementation of a concurrency-safe BankAccount type with validations, custom error types, a MaxTransactionAmount constant, a constructor, and Deposit/Withdraw/Transfer methods using mutexes and deterministic ID-based lock ordering for transfers. Changes
Sequence Diagram(s)sequenceDiagram participant Client participant Account as BankAccount participant Mu as account.mu Client->>Account: Deposit(amount) activate Account Account->>Account: Validate amount (>=0 && <= MaxTransactionAmount) alt invalid Account-->>Client: error else valid Account->>Mu: Lock activate Mu Account->>Account: Balance += amount Mu-->>Account: Unlock deactivate Mu Account-->>Client: nil end deactivate Account sequenceDiagram participant Client participant Src as SourceAccount participant Tgt as TargetAccount participant MuSrc as src.mu participant MuTgt as tgt.mu Client->>Src: Transfer(amount, Tgt) activate Src Src->>Src: Validate amount, limits, target (non-nil, not self) alt invalid Src-->>Client: error else valid Note over Src,Tgt: Determine lock order by account ID (deterministic) Src->>MuSrc: Lock (first) activate MuSrc Tgt->>MuTgt: Lock (second) activate MuTgt Src->>Src: Balance -= amount Tgt->>Tgt: Balance += amount MuTgt-->>Tgt: Unlock deactivate MuTgt MuSrc-->>Src: Unlock deactivate MuSrc Src-->>Client: nil end deactivate Src Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Pre-merge checks✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 4
🧹 Nitpick comments (2)
challenge-7/submissions/kiramux/solution-template.go (2)
78-78: Replace Chinese comments with English.The inline comments use Chinese characters, which may reduce readability for international contributors.
Apply this diff:
-// 验证 accountID +// Validate account ID if id == "" { ... } -// 验证 owner +// Validate owner if owner == "" { ... } -// 验证 initialBalance +// Validate initial balance if initialBalance < 0 { ... } -// 验证 minBalance +// Validate minimum balance if minBalance < 0 { ... } -// 比较 initialBalance 和 minBalance +// Check that initial balance meets minimum requirement if initialBalance < minBalance { ... }Also applies to: 87-87, 96-96, 105-105, 114-114
205-205: Fix typo in comment."targer" should be "target".
Apply this diff:
-// check targer account is valid or not +// check target account is valid or not switch target {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
challenge-7/submissions/kiramux/solution-template.go(1 hunks)
🔇 Additional comments (1)
challenge-7/submissions/kiramux/solution-template.go (1)
236-247: LGTM!The balance transfer logic correctly validates the source account's minimum balance constraint and updates both account balances under proper locking.
1. fix error code mistakes of 'check minBalance' and 'Withdraw' 2. add defensive programming for duplicate IDs. maybe `a.ID == target.ID` but `a != target (duplicate IDs)`
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
challenge-7/submissions/kiramux/solution-template.go (1)
11-17: Exported Balance/MinBalance fields still create race conditions.This issue was previously flagged: exported
BalanceandMinBalancefields allow direct read/write access without mutex protection, causing potential data races with concurrentDeposit/Withdraw/Transferoperations.
🧹 Nitpick comments (2)
challenge-7/submissions/kiramux/solution-template.go (2)
78-114: Use English for code comments.The validation logic is solid, but comments on lines 78, 87, 96, 105, and 114 are in Chinese. For consistency and broader accessibility, please use English comments throughout the codebase.
Apply this diff to translate the comments:
-// 验证 accountID +// Validate account ID if id == "" { return nil, &AccountError{ Code: "INVALID_ACCOUNT_ID", Message: "account ID cannot be empty", AccountID: id, } } -// 验证 owner +// Validate owner if owner == "" { return nil, &AccountError{ Code: "INVALID_OWNER", Message: "owner name cannot be empty", AccountID: id, } } -// 验证 initialBalance +// Validate initial balance if initialBalance < 0 { return nil, &NegativeAmountError{ Code: "INVALID_INITIAL_BALANCE", Message: "initial balance cannot be negative", Amount: initialBalance, } } -// 验证 minBalance +// Validate minimum balance if minBalance < 0 { return nil, &NegativeAmountError{ Code: "INVALID_MIN_BALANCE", Message: "min balance cannot be negative", Amount: minBalance, } } -// 比较 initialBalance 和 minBalance +// Compare initial balance and minimum balance if initialBalance < minBalance {
207-212: Improve error message grammar.The error message "target account is not existed" should be "target account does not exist" or "target account is nil" for better English grammar.
Apply this diff:
case nil: return &AccountError{ Code: "INVALID_TARGET_ACCOUNT", -Message: "target account is not existed", +Message: "target account does not exist", AccountID: "", }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
challenge-7/submissions/kiramux/solution-template.go(1 hunks)
🔇 Additional comments (5)
challenge-7/submissions/kiramux/solution-template.go (5)
1-8: LGTM!The package declaration and imports are appropriate for the bank account implementation with concurrency support.
19-22: LGTM!The
MaxTransactionAmountconstant is well-defined and provides a clear transaction limit.
24-71: LGTM!The custom error types are well-designed with clear fields and informative
Error()methods that provide good context for debugging.
131-153: LGTM!The
Depositmethod correctly validates the amount and uses mutex protection for thread-safe balance updates.
221-241: LGTM!The deterministic lock ordering by account ID effectively prevents deadlocks, and the equal-ID case is now properly handled with a clear error message.
1. fix the typo in line 205:"targer" should be "target" 2. fix error code of 'withdraw' or 'transfer'. When withdraw or transfer, the error code "INVALID_TRANSFER_AMOUNT" is misleading here. This code suggests the amount parameter is invalid, but the actual issue is insufficient funds. Use "INSUFFICIENT_FUNDS" or "BELOW_MIN_BALANCE" for clarity.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
challenge-7/submissions/kiramux/solution-template.go (1)
11-17: Exported Balance fields still create race conditions.The
BalanceandMinBalancefields remain exported, allowing external code to read/write them without acquiring the mutex. This races with theDeposit/Withdraw/Transfermethods. This issue was flagged in a previous review.If the test harness requires these fields to be exported, document that direct field access is unsafe. Otherwise, make them private and provide thread-safe getter methods.
🧹 Nitpick comments (2)
challenge-7/submissions/kiramux/solution-template.go (2)
78-114: Consider using English for all comments.The inline comments are written in Chinese (e.g., "验证 accountID" means "validate accountID"). For consistency and broader accessibility, consider translating them to English.
Apply this diff if desired:
-// 验证 accountID +// Validate account ID if id == "" { return nil, &AccountError{ Code: "INVALID_ACCOUNT_ID", Message: "account ID cannot be empty", AccountID: id, } } -// 验证 owner +// Validate owner if owner == "" { return nil, &AccountError{ Code: "INVALID_OWNER", Message: "owner name cannot be empty", AccountID: id, } } -// 验证 initialBalance +// Validate initial balance if initialBalance < 0 { return nil, &NegativeAmountError{ Code: "INVALID_INITIAL_BALANCE", Message: "initial balance cannot be negative", Amount: initialBalance, } } -// 验证 minBalance +// Validate minimum balance if minBalance < 0 { return nil, &NegativeAmountError{ Code: "INVALID_MIN_BALANCE", Message: "min balance cannot be negative", Amount: minBalance, } } -// 比较 initialBalance 和 minBalance +// Compare initial balance and minimum balance if initialBalance < minBalance {
115-121: Consider using consistent error code naming.The error code
"INSUFFICIENT_FUND"(singular) is used here, while line 178 and 246 use"INSUFFICIENT_FUNDS"(plural). Consider using"INSUFFICIENT_FUNDS"consistently throughout for better maintainability.Apply this diff for consistency:
if initialBalance < minBalance { return nil, &InsufficientFundsError{ -Code: "INSUFFICIENT_FUND", +Code: "INSUFFICIENT_FUNDS", Message: fmt.Sprintf("the initialBalance: %.2f is less than minBalance: %.2f", initialBalance, minBalance), MinBalance: minBalance, } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
challenge-7/submissions/kiramux/solution-template.go(1 hunks)
🔇 Additional comments (8)
challenge-7/submissions/kiramux/solution-template.go (8)
1-8: LGTM!Clean package structure with appropriate imports for error handling and concurrency control.
20-22: LGTM!Transaction limit constant is well-defined and consistently applied across all monetary operations.
27-71: LGTM!Well-designed error types with descriptive messages and appropriate context. Each error provides sufficient information for debugging and user feedback.
133-153: LGTM!The deposit method correctly validates input and uses proper mutex synchronization for thread safety.
158-185: LGTM!The withdraw method properly validates inputs, enforces minimum balance constraints, and correctly synchronizes balance updates. All previous issues have been addressed.
190-219: LGTM!Comprehensive validation of transfer parameters including amount bounds, nil target, and self-transfer prevention. All previous issues have been corrected.
221-236: LGTM!Excellent deadlock prevention implementation using deterministic lock ordering. The equal-ID edge case is now properly handled as suggested in the previous review.
238-254: LGTM!Proper dual-lock acquisition with correct defer ordering, thorough balance validation, and atomic transfer execution. The implementation correctly prevents race conditions during transfers.
2 participants
Challenge 7 Solution
Submitted by: @kiramux
Challenge: Challenge 7
Description
This PR contains my solution for Challenge 7.
Changes
challenge-7/submissions/kiramux/solution-template.goTesting
Thank you for reviewing my submission! 🚀