Add solution for gin challenge-1-basic-routing by alle2k#1486
Add solution for gin challenge-1-basic-routing by alle2k#1486alle2k wants to merge 1 commit intoRezaSi:mainfrom
Conversation
WalkthroughA submission file adds a standalone Gin-based REST API in Go with in-memory user storage. Implements CRUD endpoints for user management (create, read, update, delete), a name-based search endpoint, user and response data models, input validation with email format checking, and error handling for invalid requests. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 616f61fa-71f5-431e-b319-f745534e73ee
📒 Files selected for processing (1)
packages/gin/challenge-1-basic-routing/submissions/alle2k/solution.go
| u.ID = nextID | ||
| nextID++ | ||
| users = append(users, u) | ||
| c.JSON(201, Response{true, u, "success", "", 200}) |
There was a problem hiding this comment.
Keep Response.Code aligned with the 201 status.
Line 134 returns 201 Created over HTTP but serializes Code: 200 in the JSON body. Clients reading the envelope will see conflicting statuses.
🛠️ Suggested fix
- c.JSON(201, Response{true, u, "success", "", 200})
+ c.JSON(201, Response{true, u, "success", "", 201})📝 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.
| c.JSON(201, Response{true, u, "success", "", 200}) | |
| c.JSON(201, Response{true, u, "success", "", 201}) |
| u := User{} | ||
| if err := c.ShouldBind(&u); err != nil { | ||
| c.JSON(400, Response{ | ||
| Code: 400, | ||
| Error: err.Error(), | ||
| }) | ||
| return | ||
| } | ||
| // Find and update user | ||
| delegate, idx := findUserByID(id) | ||
| if delegate == nil { | ||
| c.JSON(404, Response{Code: 404}) | ||
| return | ||
| } | ||
| u.ID = id | ||
| users[idx] = u |
There was a problem hiding this comment.
Validate updates before overwriting the stored user.
PUT /users/:id currently accepts {} or an invalid email and persists it directly into users[idx]. createUser already rejects those payloads, so update should apply the same validateUser check before mutation.
🛠️ Suggested fix
u := User{}
if err := c.ShouldBind(&u); err != nil {
c.JSON(400, Response{
Code: 400,
Error: err.Error(),
})
return
}
+ if err := validateUser(u); err != nil {
+ c.JSON(400, Response{
+ Success: false,
+ Code: 400,
+ Error: err.Error(),
+ })
+ return
+ }
// Find and update user
delegate, idx := findUserByID(id)📝 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.
| u := User{} | |
| if err := c.ShouldBind(&u); err != nil { | |
| c.JSON(400, Response{ | |
| Code: 400, | |
| Error: err.Error(), | |
| }) | |
| return | |
| } | |
| // Find and update user | |
| delegate, idx := findUserByID(id) | |
| if delegate == nil { | |
| c.JSON(404, Response{Code: 404}) | |
| return | |
| } | |
| u.ID = id | |
| users[idx] = u | |
| u := User{} | |
| if err := c.ShouldBind(&u); err != nil { | |
| c.JSON(400, Response{ | |
| Code: 400, | |
| Error: err.Error(), | |
| }) | |
| return | |
| } | |
| if err := validateUser(u); err != nil { | |
| c.JSON(400, Response{ | |
| Success: false, | |
| Code: 400, | |
| Error: err.Error(), | |
| }) | |
| return | |
| } | |
| // Find and update user | |
| delegate, idx := findUserByID(id) | |
| if delegate == nil { | |
| c.JSON(404, Response{Code: 404}) | |
| return | |
| } | |
| u.ID = id | |
| users[idx] = u |
| users = append(users[:idx], users[idx+1:]...) | ||
| // Return success message | ||
| c.JSON(200, Response{true, users, "success", "", 200}) |
There was a problem hiding this comment.
Return the delete acknowledgement, not the full collection.
Line 191 says this handler should return a success message, but Line 192 sends the remaining users slice. That changes the DELETE response shape and can miss the template's expected contract.
🛠️ Suggested fix
- c.JSON(200, Response{true, users, "success", "", 200})
+ c.JSON(200, Response{
+ Success: true,
+ Message: "success",
+ Code: 200,
+ })Use the exact delete message the challenge expects if it needs to be more specific than "success".
| for i, user := range users { | ||
| if user.ID == id { | ||
| u = &user | ||
| idx = i |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Go, when iterating over a slice with for _, user := range users, does taking &user point to the slice element or to the per-iteration copy?
💡 Result:
user is the iteration variable, which gets assigned a copy of users[i] each iteration—so &user points to that variable (the copy), not to the slice element. In other words, &user != &users[i] in general. [1]
Version detail:
- Go ≤ 1.21: the same iteration variable is reused each time, so
&useris the same address every iteration (a common bug when collecting pointers). - Go ≥ 1.22: loop variables are created anew each iteration, so
&useris a different address per iteration, but it still points to the per-iteration copy, not the slice element. [2]
To take the address of the actual slice element, use the index:
for i := range users {
p := &users[i]
_ = p
}Sources: Go spec on range assigning iteration values [1]; Go 1.22 change to per-iteration loop variables [2].
[1] (go.dev)
[2] (tip.golang.org)
Citations:
- 1: https://go.dev/doc/go1.17_spec?utm_source=openai
- 2: https://tip.golang.org/doc/go1.22?utm_source=openai
🏁 Script executed:
cd packages/gin/challenge-1-basic-routing/submissions/alle2k && \
wc -l solution.go && \
sed -n '220,250p' solution.goRepository: RezaSi/go-interview-practice
Length of output: 803
🏁 Script executed:
cd packages/gin/challenge-1-basic-routing/submissions/alle2k && \
grep -n "findUserByID\|u\." solution.go | head -30Repository: RezaSi/go-interview-practice
Length of output: 250
🏁 Script executed:
cd packages/gin/challenge-1-basic-routing/submissions/alle2k && \
sed -n '85,135p' solution.go && \
echo "---" && \
sed -n '155,175p' solution.goRepository: RezaSi/go-interview-practice
Length of output: 1507
🏁 Script executed:
cd packages/gin/challenge-1-basic-routing/submissions/alle2k && \
sed -n '175,210p' solution.goRepository: RezaSi/go-interview-practice
Length of output: 896
Return &users[i] instead of &user.
Line 229 takes the address of the range copy, not the slice element. While the current code avoids issues by using idx directly for mutations, the pointer is detached from the slice backing store and could cause subtle bugs if the code is refactored later.
🛠️ Suggested fix
- for i, user := range users {
- if user.ID == id {
- u = &user
+ for i := range users {
+ if users[i].ID == id {
+ u = &users[i]
idx = i
break
}
}📝 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.
| for i, user := range users { | |
| if user.ID == id { | |
| u = &user | |
| idx = i | |
| for i := range users { | |
| if users[i].ID == id { | |
| u = &users[i] | |
| idx = i | |
| break | |
| } | |
| } |
1 participant
No description provided.