|
| 1 | +# Notes on the "update a user" change |
| 2 | + |
| 3 | +## The plan I approved |
| 4 | + |
| 5 | +Add `PUT /users/:id` to `routes/users.js` plus a matching `updateUser` helper in |
| 6 | +`db/store.js`, so routes never touch the `users` array directly. The route validates |
| 7 | +that `name` and `email` are both present (400 if not), converts the `:id` param to a |
| 8 | +number before looking the user up, returns 404 when no user has that id, and responds |
| 9 | +with the updated user on success. `server.js` needed no changes because the users |
| 10 | +router is already mounted there. |
| 11 | + |
| 12 | +I made one edit to the plan before approving it: I moved validation ahead of the |
| 13 | +lookup. The 400 test hits `/users/1`, which is a real seeded user, so validating first |
| 14 | +makes the 400 come from the validation check itself rather than from a lookup that |
| 15 | +happened to miss. |
| 16 | + |
| 17 | +## Model choice |
| 18 | + |
| 19 | +Sonnet for the whole task. It is a small, well-scoped change in a tiny codebase with |
| 20 | +tests that spell out the exact contract, so the extra reasoning budget of a larger |
| 21 | +model would not have bought anything. I used plan mode first so the approach was |
| 22 | +reviewed before any code was written. |
| 23 | + |
| 24 | +## Commit split |
| 25 | + |
| 26 | +Three commits: (1) the `updateUser` store helper, (2) the `PUT` route that calls it, |
| 27 | +(3) this `NOTES.md`. Helper before route mirrors the dependency direction and keeps |
| 28 | +each diff to one file and one idea. Docs are separated from code so the graded change |
| 29 | +stays easy to read on its own. |
| 30 | + |
| 31 | +## What the review caught |
| 32 | + |
| 33 | +Running the endpoint against a live server confirmed the three test cases plus two |
| 34 | +edge cases the tests do not cover: a non-numeric id (`/users/abc`) returns 404 rather |
| 35 | +than crashing, because `Number("abc")` is `NaN` and never matches an id; and a request |
| 36 | +with no body at all returns 400, because `express.json()` yields `{}` and the |
| 37 | +validation check catches it. A follow-up `GET` confirmed the update actually persisted |
| 38 | +in the store. |
| 39 | + |
| 40 | +Known limitation left in on purpose: validation is a truthiness check identical to |
| 41 | +`POST /users`, so it does not reject a whitespace-only name or a non-string value. |
| 42 | +Making `PUT` stricter than `POST` on the same resource would be inconsistent; if that |
| 43 | +validation is wanted it should be added to both routes together. |
0 commit comments