Add solution for gin challenge-1-basic-routing by yogtanko#1744
Add solution for gin challenge-1-basic-routing by yogtanko#1744yogtanko wants to merge 4 commits into
Conversation
WalkthroughAdds a single-file Gin REST API exposing BasicRouting(), defines exported User and Response types, initializes RWMutex-protected in-memory users with nextID, wires GET/POST/PUT/DELETE + /users/search routes, and implements handlers plus find/validate helpers. ChangesIn-Memory User CRUD API
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 6
🧹 Nitpick comments (2)
packages/gin/challenge-1-basic-routing/submissions/yogtanko/solution.go (2)
121-130: ⚡ Quick winPrefer Gin's
ShouldBindJSONfor idiomatic request parsing.Using
json.NewDecoderworks but Gin providesc.ShouldBindJSON(&user)which is more idiomatic, provides better error messages, and integrates seamlessly with Gin's validation framework.♻️ Proposed refactor
- user := User{} - decoder := json.NewDecoder(c.Request.Body) - err := decoder.Decode(&user) + var user User + err := c.ShouldBindJSON(&user) if err != nil { - c.JSON(500, Response{ + c.JSON(400, Response{ Success: false, Error: err.Error(), })
166-175: ⚡ Quick winPrefer
ShouldBindJSONand return 400 for malformed JSON.Similar to
createUser, this handler should usec.ShouldBindJSON(&user)for idiomatic Gin code and return 400 (not 500) for JSON decode errors.♻️ Proposed refactor
- user := &User{} - decoder := json.NewDecoder(c.Request.Body) - err = decoder.Decode(user) + var user User + err = c.ShouldBindJSON(&user) if err != nil { - c.JSON(500, Response{ + c.JSON(400, Response{ Success: false, Error: err.Error(), }) return } - if err = validateUser(*user); err != nil { + if err = validateUser(user); err != nil {Note: This also requires updating line 194 to use
userinstead of*user.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5b7f82a9-a8fc-43b3-8af7-8fc123817008
📒 Files selected for processing (1)
packages/gin/challenge-1-basic-routing/submissions/yogtanko/solution.go
| if len(users) > 0 { | ||
| c.JSON(200, Response{ | ||
| Success: true, | ||
| Data: users, | ||
| }) | ||
| } else { | ||
| c.JSON(404, Response{ | ||
| Success: false, | ||
| Error: "User not found", | ||
| }) | ||
| return | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Return 200 with empty array instead of 404 for no users.
When the user list is empty, returning 404 with "User not found" is misleading. An empty collection is a valid state and should return 200 with an empty array [], not a 404 error. This aligns with REST API best practices where 404 indicates a missing resource, not an empty collection.
♻️ Proposed fix
- if len(users) > 0 {
- c.JSON(200, Response{
- Success: true,
- Data: users,
- })
- } else {
- c.JSON(404, Response{
- Success: false,
- Error: "User not found",
- })
- return
- }
+ c.JSON(200, Response{
+ Success: true,
+ Data: users,
+ })📝 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 len(users) > 0 { | |
| c.JSON(200, Response{ | |
| Success: true, | |
| Data: users, | |
| }) | |
| } else { | |
| c.JSON(404, Response{ | |
| Success: false, | |
| Error: "User not found", | |
| }) | |
| return | |
| } | |
| c.JSON(200, Response{ | |
| Success: true, | |
| Data: users, | |
| }) |
✅ Addressed in commit 9441dda
There was a problem hiding this comment.
unit test used 404 instead 200
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the test file for this challenge
fd -e go 'solution_test' packages/gin/challenge-1-basic-routing/ --exec cat {}Length of output: 373
🏁 Script executed:
#!/bin/bash
# Find all Go files in the challenge directory
fd -e go . packages/gin/challenge-1-basic-routing/ --exec echo {}
cat packages/gin/challenge-1-basic-routing/solution_test.go 2>/dev/null || echo "not found at that path"
find packages/gin/challenge-1-basic-routing/ -name "*.go" | xargs ls -la 2>/dev/nullLength of output: 8798
🏁 Script executed:
#!/bin/bash
cat packages/gin/challenge-1-basic-routing/solution-template_test.goLength of output: 6911
@yogtanko, I checked the test file (solution-template_test.go) and the actual test for GET /users is:
func TestGetAllUsers(t *testing.T) {
router := setupRouter()
// ...
assert.Equal(t, 200, w.Code) // expects 200, not 404The test always pre-populates users with 3 entries via setupRouter(), so it expects 200 and never tests the empty-list scenario. There is no test case that exercises the 404 branch in getAllUsers at all — meaning the 404 code path is effectively dead code during testing.
My original suggestion still holds: returning 404 for an empty collection is non-standard REST behaviour. Since no test requires it, you can safely simplify to always return 200:
func getAllUsers(c *gin.Context) {
mu.RLock()
defer mu.RUnlock()
c.JSON(200, Response{
Success: true,
Data: users,
})
}This removes the misleading 404 path without breaking any tests.
✏️ Learnings added
Learnt from: yogtanko
Repo: RezaSi/go-interview-practice PR: 1744
File: packages/gin/challenge-1-basic-routing/submissions/yogtanko/solution.go:69-80
Timestamp: 2026-05-30T08:42:48.267Z
Learning: In the gin challenge-1-basic-routing challenge (`packages/gin/challenge-1-basic-routing/solution-template_test.go`), the `TestGetAllUsers` test always pre-populates 3 users via `setupRouter()` and expects HTTP 200. There is no test case for an empty users list in `getAllUsers`. Returning 404 for an empty `users` slice is non-standard REST behavior and is never required by the challenge tests.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: PopovMarko
Repo: RezaSi/go-interview-practice PR: 1396
File: challenge-18/submissions/PopovMarko/solution-template.go:36-44
Timestamp: 2026-02-20T12:38:34.852Z
Learning: Do not modify predefined function signatures in submission solutions. In this repository, challenges specify exact function signatures that tests rely on. Keep parameter types and return types intact; only implement the required logic within the given function bodies.
Learnt from: CV-Elevation
Repo: RezaSi/go-interview-practice PR: 1559
File: packages/gin/challenge-1-basic-routing/submissions/CV-Elevation/solution.go:236-245
Timestamp: 2026-04-07T14:19:58.995Z
Learning: In Go repositories using modules with `go 1.22` or later in `go.mod`, the iteration variable in `for range` loops has per-iteration scope. Therefore, taking `&loopVar` inside a `range` loop should not be flagged as the classic shared-loop-variable bug for Go 1.22+. The review should focus instead on correctness: `&loopVar` points to a per-iteration copy of the value, not the underlying slice/map element. If the caller intends to mutate the original slice element through the pointer, prefer taking the address of the original element (e.g., `&slice[i]`) rather than `&loopVar`.
| if err != nil { | ||
| c.JSON(500, Response{ | ||
| Success: false, | ||
| Error: err.Error(), | ||
| }) | ||
| return |
There was a problem hiding this comment.
Use 400 Bad Request for client errors, not 500 Internal Server Error.
Lines 159 and 177 return HTTP 500 for invalid client input (malformed ID and validation failures). These are client errors and should return 400 Bad Request. Reserve 500 for unexpected server failures.
🛠️ Proposed fix
id, err := strconv.Atoi(paramId)
if err != nil {
- c.JSON(500, Response{
+ c.JSON(400, Response{
Success: false,
Error: err.Error(),
}) if err = validateUser(*user); err != nil {
- c.JSON(500, Response{
+ c.JSON(400, Response{
Success: false,
Error: err.Error(),
})Also applies to: 176-181
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/gin/challenge-1-basic-routing/submissions/yogtanko/solution.go (1)
200-205:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse 400 Bad Request for invalid ID format.
Parsing failure of the ID parameter indicates a client error (malformed input), not a missing resource. This should return
http.StatusBadRequest(400) instead of 404.🛠️ Proposed fix
id, err := strconv.Atoi(paramId) if err != nil { - c.JSON(http.StatusNotFound, Response{ + c.JSON(http.StatusBadRequest, Response{ Success: false, Error: err.Error(), })
♻️ Duplicate comments (2)
packages/gin/challenge-1-basic-routing/submissions/yogtanko/solution.go (2)
152-156:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse 400 Bad Request for invalid ID format.
Parsing failure of the ID parameter indicates a client error (malformed input), not an internal server failure. This should return
http.StatusBadRequest(400) instead of 500.🛠️ Proposed fix
id, err := strconv.Atoi(paramId) if err != nil { - c.JSON(500, Response{ + c.JSON(http.StatusBadRequest, Response{ Success: false, Error: err.Error(), })
168-173:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse 400 Bad Request for validation failures.
Validation errors indicate the client sent invalid data (missing or malformed fields), not an internal server failure. This should return
http.StatusBadRequest(400) instead of 500.🛠️ Proposed fix
if err = validateUser(user); err != nil { - c.JSON(http.StatusInternalServerError, Response{ + c.JSON(http.StatusBadRequest, Response{ Success: false, Error: err.Error(), })
🧹 Nitpick comments (1)
packages/gin/challenge-1-basic-routing/submissions/yogtanko/solution.go (1)
69-80: ⚡ Quick winSimplify by removing the unnecessary else branch.
The else branch returns
200 OKwithSuccess: falseand an error message when the list is empty, which is semantically confusing. Since an empty collection is a valid state and the test suite pre-populates users (per the retrieved learning), you can safely remove the conditional and always return200 OKwith the users slice (empty or not).♻️ Simplified implementation
func getAllUsers(c *gin.Context) { mu.RLock() defer mu.RUnlock() - if len(users) > 0 { - c.JSON(http.StatusOK, Response{ - Success: true, - Data: users, - }) - } else { - c.JSON(http.StatusOK, Response{ - Success: false, - Error: "User not found", - }) - return - } + c.JSON(http.StatusOK, Response{ + Success: true, + Data: users, + }) }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 36d68a9a-7d3e-4bf6-8bc5-f7b38009917c
📒 Files selected for processing (1)
packages/gin/challenge-1-basic-routing/submissions/yogtanko/solution.go
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/gin/challenge-1-basic-routing/submissions/yogtanko/solution.go (2)
75-78:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReturn an empty list payload for empty collections, not an error-shaped body.
For
GET /userswith HTTP 200, returningSuccess: false+"User not found"makes a valid empty collection look like a failure. PreferSuccess: truewithData: []User{}.Proposed fix
- } else { - c.JSON(http.StatusOK, Response{ - Success: false, - Error: "User not found", - }) - return - } + } else { + c.JSON(http.StatusOK, Response{ + Success: true, + Data: []User{}, + }) + return + }Based on learnings: for this challenge,
GET /usersshould remain200, and empty-list handling should be non-breaking.
199-203:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse 400 Bad Request for malformed
:idin delete handler.
strconv.Atoifailure is a client input error, not a missing resource. Returning 404 here misclassifies the error.Proposed fix
if err != nil { - c.JSON(http.StatusNotFound, Response{ + c.JSON(http.StatusBadRequest, Response{ Success: false, - Error: err.Error(), + Error: "Invalid ID", }) return }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1b30b9c5-387a-4ef1-9919-f4e2d32efa6e
📒 Files selected for processing (1)
packages/gin/challenge-1-basic-routing/submissions/yogtanko/solution.go
1 participant
gin challenge-1-basic-routing Solution
Submitted by: @yogtanko
Package: gin
Challenge: challenge-1-basic-routing
Description
This PR contains my solution for gin challenge-1-basic-routing.
Changes
packages/gin/challenge-1-basic-routing/submissions/yogtanko/solution.goTesting
Thank you for reviewing my submission! 🚀