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
51 changes: 51 additions & 0 deletions NOTES.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
# Notes — shipping `PUT /users/:id`

## The plan

Two files, one small change each: a `updateUser` helper in `db/store.js`, and a
`PUT /:id` route in `routes/users.js`. I read `tests/update-user.test.js` first
and let it define the contract — 200 with the updated body, 404 for an unknown
id, 400 for a missing field — then planned the edges the tests don't cover.

## Choices

**Full replace, not a partial update.** The provided test sends `{ name: "Only
a name" }` and expects a 400, so `PUT` here means "replace both fields." That
matches `PUT` semantics anyway; a merge-style partial update would be a separate
`PATCH` endpoint. Both `name` and `email` must be non-empty strings, and the
email is shape-checked against a deliberately loose pattern — enough to catch an
obvious typo without pretending to implement RFC 5322.

**Validate the body before looking the user up.** A malformed request is a client
error whether or not the id happens to exist, so the 400 fires first. The
provided tests pass under either ordering (the 404 case sends a valid body, the
400 case targets an id that exists), so this was a judgement call, not a
constraint.

**Validation in the route, persistence in the store.** `updateUser` returns
`undefined` when the id is missing rather than throwing or sending a response,
so the "not found" decision stays in the route layer — exactly how `GET /:id`
already works. It reuses `getUserById` instead of a second `users.find(...)`,
and mutates the record in place so `GET /users` reflects the change immediately.

**A known inconsistency, left alone on purpose.** `PUT /users/abc` returns 400
("id must be an integer"), but `GET /users/abc` returns 404 today — `Number("abc")`
is `NaN`, so the lookup just misses. Making the two agree means touching `GET`,
which is outside this change; flagging it here beats silently widening the diff.

## Model and commits

Written with Claude Opus 5 in Claude Code, planned in plan mode before any edit.
Three commits, each independently reviewable: the store helper, then the route,
then this write-up.

## What the review caught

Two things worth fixing. The first draft leaned on the same falsy check as
`POST /users` (`!name || !email`), which lets `{ name: " " }` through as a valid
name — hence `isNonEmptyString` and trimming the values before they reach the
store. The second was the invalid-id gap above: I noticed it only when writing
the smoke tests by hand, and chose to document rather than quietly change `GET`.

Verified with `npm test` (9/9), `npm run lint` clean, and a manual `curl` pass
over all five response paths plus `GET /users` to confirm the update persisted.
14 changes: 13 additions & 1 deletion db/store.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,4 +24,16 @@ 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 };
35 changes: 35 additions & 0 deletions routes/users.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,14 @@ const store = require("../db/store");

const router = express.Router();

// Deliberately loose: enough to catch obvious typos without pretending to
// implement full RFC 5322 validation.
const EMAIL_PATTERN = /^[^\s@]+@[^\s@]+\.[^\s@]+$/;

function isNonEmptyString(value) {
return typeof value === "string" && value.trim() !== "";
}

// GET /users — list every user
router.get("/", (req, res) => {
res.json(store.getAllUsers());
Expand Down Expand Up @@ -32,4 +40,31 @@ router.post("/", (req, res) => {
res.status(201).json(user);
});

// PUT /users/:id — replace a user's details; name and email are both required
router.put("/:id", (req, res) => {
const id = Number(req.params.id);

if (!Number.isInteger(id)) {
return res.status(400).json({ error: "id must be an integer" });
}

const { name, email } = req.body;

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

if (!EMAIL_PATTERN.test(email.trim())) {
return res.status(400).json({ error: "email must be a valid email address" });
}

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

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

res.json(user);
});

module.exports = router;