Add solution for Challenge 13 by imankhodadi#1032
Add solution for Challenge 13 by imankhodadi#1032imankhodadi wants to merge 1 commit intoRezaSi:mainfrom
Conversation
WalkthroughThis PR introduces a complete SQLite-backed product management module in Go with a public API for CRUD operations, batch updates, and dynamic filtering. The implementation includes a Product struct, ProductStore type with core methods, database initialization, transaction-based batch operations, and a demonstration main function. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 (3)
challenge-13/submissions/imankhodadi/solution-template.go (3)
229-241: Redundant explicittx.Rollback()calls.The deferred rollback (lines 215-218) already handles rollback when
committedis false. The explicittx.Rollback()calls here are redundant and will cause double-rollback attempts. Compare withBatchUpdateInventorywhich relies solely on the deferred rollback.🔎 Proposed fix - remove explicit rollbacks
result, err := stmt.Exec(price, id) if err != nil { - tx.Rollback() return fmt.Errorf("failed to update price for product %d: %w", id, err) } rowsAffected, err := result.RowsAffected() if err != nil { - tx.Rollback() return fmt.Errorf("failed to get rows affected for product %d: %w", id, err) } if rowsAffected == 0 { - tx.Rollback() return fmt.Errorf("product with ID %d not found", id) }
270-275: Prefer named fields in struct literals for clarity and safety.Positional struct literals are fragile—if
Productfields are reordered, this code will silently break or produce incorrect data. Using named fields improves readability and prevents such issues.🔎 Proposed fix
- err = ps.CreateProduct(&Product{0, "Cacao", 10.5, 3, "junk"}) + err = ps.CreateProduct(&Product{Name: "Cacao", Price: 10.5, Quantity: 3, Category: "junk"}) if err != nil { fmt.Println("Failed to create product:", err) return } - err = ps.CreateProduct(&Product{0, "milk", 20.5, 2, "dairy"}) + err = ps.CreateProduct(&Product{Name: "milk", Price: 20.5, Quantity: 2, Category: "dairy"})
134-167: Consider API consistency: standalone functions vs. ProductStore methods.
ListProducts2andBulkUpdatePrices2are standalone functions taking*sql.DB, while similar functionality (ListProducts,BatchUpdateInventory) are methods onProductStore. For a cohesive API, consider making these methods onProductStoreas well.Also applies to: 209-248
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
challenge-13/submissions/imankhodadi/solution-template.go
🧰 Additional context used
🧬 Code graph analysis (1)
challenge-13/submissions/imankhodadi/solution-template.go (2)
challenge-13/submissions/Gandook/solution-template.go (1)
ID(144-144)challenge-13/submissions/Ali-Fartoot/solution-template.go (1)
DB(29-29)
🔇 Additional comments (4)
challenge-13/submissions/imankhodadi/solution-template.go (4)
11-43: LGTM on struct definitions and initialization.The
Productstruct,ProductStore,NewProductStore, andInitDBare well-implemented. Good practice closing the database on table creation failure (line 39).
45-103: CRUD operations are well-implemented.Good practices observed:
- Parameterized queries prevent SQL injection
sql.ErrNoRowsis properly handled inGetProductRowsAffectedvalidation ensures operations affect expected rows
105-132: LGTM!Proper use of
defer rows.Close()androws.Err()check after iteration. The category filter uses parameterized queries correctly.
169-207: Well-structured transactional batch update.Good implementation with:
committedflag pattern for safe deferred rollback- Prepared statement reuse for performance
- Per-ID validation of affected rows
| if name, ok := filters["name"]; ok { | ||
| conditions = append(conditions, "name LIKE ?") | ||
| args = append(args, "%"+name.(string)+"%") |
There was a problem hiding this comment.
Unsafe type assertion can cause runtime panic.
The type assertion name.(string) will panic if the value is not a string. Use a type assertion with the "comma ok" idiom to handle this safely.
🔎 Proposed fix
if name, ok := filters["name"]; ok {
+ nameStr, isString := name.(string)
+ if !isString {
+ return nil, fmt.Errorf("filter 'name' must be a string")
+ }
conditions = append(conditions, "name LIKE ?")
- args = append(args, "%"+name.(string)+"%")
+ args = append(args, "%"+nameStr+"%")
}🤖 Prompt for AI Agents
In challenge-13/submissions/imankhodadi/solution-template.go around lines 139 to
141, the direct type assertion name.(string) can panic if filters["name"] is not
a string; change to a safe assertion using the "comma ok" idiom (e.g., val, ok
:= name.(string)) and only append the condition and args when ok is true, or
otherwise handle non-string values (skip, convert, or return an error) to avoid
runtime panics.
| products = append(products, product) | ||
| } | ||
| return products, nil |
There was a problem hiding this comment.
Missing rows.Err() check after iteration.
After iterating through rows with rows.Next(), you should check rows.Err() to catch any errors that occurred during iteration. Compare with ListProducts (line 128) which does this correctly.
🔎 Proposed fix
products = append(products, product)
}
+ if err := rows.Err(); err != nil {
+ return nil, fmt.Errorf("error iterating products: %w", err)
+ }
return products, 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.
| products = append(products, product) | |
| } | |
| return products, nil | |
| products = append(products, product) | |
| } | |
| if err := rows.Err(); err != nil { | |
| return nil, fmt.Errorf("error iterating products: %w", err) | |
| } | |
| return products, nil |
🤖 Prompt for AI Agents
In challenge-13/submissions/imankhodadi/solution-template.go around lines 164 to
166, the rows iteration return lacks a rows.Err() check; after the for
rows.Next() loop add a check like if err := rows.Err(); err != nil { return nil,
err } and return products only if that check passes so any iteration errors are
propagated.
1 participant
Challenge 13 Solution
Submitted by: @imankhodadi
Challenge: Challenge 13
Description
This PR contains my solution for Challenge 13.
Changes
challenge-13/submissions/imankhodadi/solution-template.goTesting
Thank you for reviewing my submission! 🚀