Add solution for Challenge 13 by kiramux #713
Conversation
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 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-13/submissions/kiramux/solution-template.go (2)
30-35: Consider validating the connection after opening.Adding a
db.Ping()call after opening the database would validate that the connection is actually working. This is especially useful for catching issues early.func InitDB(dbPath string) (*sql.DB, error) { // Open a SQLite database connection db, err := sql.Open("sqlite3", dbPath) if err != nil { return nil, err } + +// Validate the connection +if err = db.Ping(); err != nil { +db.Close() +return nil, err +} // Create the products table if it doesn't exist
119-138: Simplify the conditional logic.The
else if category == ""at line 129 is redundant since it's the opposite of the condition at line 121. This can be simplified to justelse. Additionally, the entire structure can be refactored for better readability.Apply this diff to simplify:
var rows *sql.Rows var err error if category != "" { rows, err = ps.db.Query( "SELECT id, name, price, quantity, category FROM products WHERE category = ?", category, ) -if err != nil { -return nil, err -} -} else if category == "" { +} else { // If category is empty, return all products rows, err = ps.db.Query( "SELECT id, name, price, quantity, category FROM products", -category, ) -if err != nil { -return nil, err -} } +if err != nil { +return nil, err +} defer rows.Close()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
challenge-13/submissions/kiramux/solution-template.go(1 hunks)
🔇 Additional comments (8)
challenge-13/submissions/kiramux/solution-template.go (8)
1-8: LGTM!The package declaration and imports follow Go best practices. The blank import for the SQLite driver is the correct pattern.
10-22: LGTM!The struct definitions are clean and well-documented. Field types are appropriate for the domain model.
24-27: LGTM!The constructor is straightforward and follows Go conventions.
46-63: LGTM!The implementation correctly uses parameterized queries and properly assigns the database-generated ID back to the product.
65-80: LGTM!Excellent error handling with explicit not-found error messages. The use of parameterized queries is appropriate.
139-154: LGTM!Excellent row iteration pattern with proper deferred cleanup and
rows.Err()check. This is the correct way to handle SQL rows in Go.
157-195: LGTM!Excellent implementation of batch updates with proper transaction handling. The use of prepared statements improves performance, and the validation of
RowsAffectedensures all products exist before committing. This is a great example of transaction-safe batch operations.
197-199: LGTM!The empty main function is appropriate for a challenge submission where testing is performed separately.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
challenge-13/submissions/kiramux/solution-template.go (4)
30-45: Resource leak has been fixed.The database connection is now properly closed on error (line 41), addressing the previous concern about resource leaks.
83-100: The RowsAffected check is still missing.The comment at line 95 states "Return an error if the product doesn't exist," but the implementation doesn't verify whether any rows were actually updated. This means updating a non-existent product will silently succeed, which is inconsistent with the BatchUpdateInventory pattern and could lead to bugs.
Apply this diff to properly check for non-existent products:
func (ps *ProductStore) UpdateProduct(product *Product) error { // Update the product in the database -_, err := ps.db.Exec( +result, err := ps.db.Exec( "UPDATE products SET name = ?, price = ?, quantity = ?, category = ? WHERE id = ?", product.Name, product.Price, product.Quantity, product.Category, product.ID, ) // Return an error if the product doesn't exist if err != nil { return err } + +rowsAffected, err := result.RowsAffected() +if err != nil { +return err +} +if rowsAffected == 0 { +return fmt.Errorf("product with ID %d not found", product.ID) +} + return nil }
102-114: The RowsAffected check is still missing.Similar to UpdateProduct, the comment at line 109 states "Return an error if the product doesn't exist," but the implementation doesn't verify whether any rows were actually deleted. This means deleting a non-existent product will silently succeed.
Apply this diff to properly check for non-existent products:
func (ps *ProductStore) DeleteProduct(id int64) error { // Delete the product from the database -_, err := ps.db.Exec( +result, err := ps.db.Exec( "DELETE FROM products WHERE id = ?", id, ) // Return an error if the product doesn't exist if err != nil { return err } + +rowsAffected, err := result.RowsAffected() +if err != nil { +return err +} +if rowsAffected == 0 { +return fmt.Errorf("product with ID %d not found", id) +} + return nil }
132-135: Remove unused query parameter.Line 134 passes
categoryas a parameter to the query, but the SQL statement doesn't contain any placeholder (?). This is incorrect and could cause issues depending on the database driver behavior.Apply this diff to fix the issue:
} else if category == "" { // If category is empty, return all products rows, err = ps.db.Query( "SELECT id, name, price, quantity, category FROM products", -category, ) if err != nil { return nil, err }
🧹 Nitpick comments (1)
challenge-13/submissions/kiramux/solution-template.go (1)
122-139: Simplify redundant condition.The condition
else if category == ""at line 130 is redundant because it's already covered by theelsecase. This can be simplified for better readability.Apply this diff to simplify:
if category != "" { rows, err = ps.db.Query( "SELECT id, name, price, quantity, category FROM products WHERE category = ?", category, ) if err != nil { return nil, err } -} else if category == "" { +} else { // If category is empty, return all products rows, err = ps.db.Query( "SELECT id, name, price, quantity, category FROM products", category, ) if err != nil { return nil, err } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
challenge-13/submissions/kiramux/solution-template.go(1 hunks)
🔇 Additional comments (6)
challenge-13/submissions/kiramux/solution-template.go (6)
1-27: LGTM!The package structure, imports, type definitions, and constructor are well-implemented. The Product and ProductStore types correctly model the domain, and the constructor follows Go conventions.
47-64: LGTM!The CreateProduct implementation correctly inserts the product and captures the generated ID, mutating the input Product struct appropriately.
66-81: LGTM!The GetProduct implementation properly handles the not-found case by checking for sql.ErrNoRows and returning a descriptive error.
140-156: LGTM!The row iteration logic is correct, with proper deferred cleanup, error handling during iteration, and a final rows.Err() check.
158-196: LGTM!The BatchUpdateInventory implementation correctly uses transactions, prepared statements, and properly checks RowsAffected to ensure all products exist before committing. This is a robust implementation with proper error handling and rollback logic.
198-200: LGTM!The empty main function is appropriate for a challenge submission where testing is optional.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
2 participants
Challenge 13 Solution
Submitted by: @kiramux
Challenge: Challenge 13
Description
This PR contains my solution for Challenge 13.
Changes
challenge-13/submissions/kiramux/solution-template.goTesting
Thank you for reviewing my submission! 🚀