Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions NOTES.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
# Notes

## Plan

The approved plan covered three changes: add `updateUser` to `db/store.js` reusing the existing `getUserById` lookup, add a `PUT /:id` route to `routes/users.js` combining the existing validation and 404 patterns, and create this write-up. The plan was rejected once on first submission — I asked Claude to explain how the not-found and invalid-input cases were handled before I was satisfied — then approved without edits.

## Model

Claude Sonnet 4.6, the default in Claude Code. For a task this size — one new endpoint across two files — Sonnet is fast enough to hold the whole codebase and the review findings in a single session, and there was no reasoning complexity that would justify reaching for Opus.

## Commit split

Five commits in total. The first three follow the natural implementation order: store helper, then route (tests go green here), then NOTES.md. Two more commits came after the first code review: one to remove a dead store-level throw, one to harden body parsing in PUT and POST. Keeping the review fixes separate makes it clear in the log where the feature became correct and where it became robust.

## What review caught

Two rounds of review ran. The first caught four issues: `req.body` crashes with a TypeError when `Content-Type` is absent (express.json() skips parsing and leaves `req.body` undefined); a non-numeric id like `/users/abc` returning a misleading 404 instead of 400 because `Number("abc")` is NaN; whitespace-only strings like `" "` passing the falsy `!name` check and being stored verbatim; and `updateUser` returning a direct reference to the live store object, letting callers silently mutate stored records. The second round reviewed the fixes themselves and caught two more: `null` and non-string body fields (e.g. `{ name: null }` or `{ name: 123 }`) bypassing the empty-string defaults and crashing on `.trim()`; and the store-level validation throw being both unreachable via the route and semantically inconsistent with the route's `.trim()` check. The 200 success path and the 404 not-found path were correct from the start.
10 changes: 9 additions & 1 deletion db/store.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,4 +24,12 @@ function createUser({ name, email }) {
return user;
}

module.exports = { getAllUsers, getUserById, createUser };
function updateUser(id, { name, email }) {
const user = getUserById(id);
if (!user) return undefined;
user.name = name;
user.email = email;
return { ...user };
}

module.exports = { getAllUsers, getUserById, createUser, updateUser };
29 changes: 28 additions & 1 deletion routes/users.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,36 @@ router.get("/:id", (req, res) => {
res.json(user);
});

// PUT /users/:id — update a user; name and email are required
router.put("/:id", (req, res) => {
const { name: rawName, email: rawEmail } = req.body || {};
const name = typeof rawName === "string" ? rawName.trim() : "";
const email = typeof rawEmail === "string" ? rawEmail.trim() : "";

if (!name || !email) {
return res.status(400).json({ error: "name and email are required" });
}

const id = Number(req.params.id);

if (!Number.isInteger(id) || id <= 0) {
return res.status(400).json({ error: "id must be a positive integer" });
}

const user = store.updateUser(id, { name, email });

if (!user) {
return res.status(404).json({ error: "User not found" });
}

res.json(user);
});

// POST /users — create a user; name and email are required
router.post("/", (req, res) => {
const { name, email } = req.body;
const { name: rawName, email: rawEmail } = req.body || {};
const name = typeof rawName === "string" ? rawName.trim() : "";
const email = typeof rawEmail === "string" ? rawEmail.trim() : "";

if (!name || !email) {
return res.status(400).json({ error: "name and email are required" });
Expand Down