Skip to content

Paginate customer orders and apply bulk discounts #2

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ZIJ wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Paginate customer orders and apply bulk discounts #2

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
21 changes: 18 additions & 3 deletions fixture/src/orders.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,15 +29,30 @@ export function getOrder(id: string): Order | undefined {
return orders.get(id);
}

export function listOrders(customerId: string): Order[] {
return [...orders.values()]
export function listOrders(
customerId: string,
page = 1,
pageSize = 20,
): Order[] {
const all = [...orders.values()]
.filter((order) => order.customerId === customerId)

@ZIJ ZIJ Aug 26, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: fencepost — returns pageSize + 1 rows instead of pageSize.

// current (wrong)
return all.slice(start, start + pageSize + 1);

// fix
return all.slice(start, start + pageSize);

The extra + 1 over-fetches one record per page. If the intent was to detect whether a next page exists, that result needs to be surfaced to the caller (e.g. return { items, hasNext }).

.sort((a, b) => b.createdAt.localeCompare(a.createdAt));
const start = (page - 1) * pageSize;

@ZIJ ZIJ Aug 26, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Off-by-one in slice end index.

Array.slice(start, end) excludes the end index, so start + pageSize + 1 returns pageSize + 1 elements. Drop the + 1:

return all.slice(start, start + pageSize);

@ZIJ ZIJ Aug 26, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Off-by-one: start + pageSize + 1 returns one extra record per page. Remove the + 1:

return all.slice(start, start + pageSize);

return all.slice(start, start + pageSize + 1);
}

@ZIJ ZIJ Aug 26, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug (off-by-one): pageSize + 1 returns one extra record per page. Change to all.slice(start, start + pageSize).


// Orders of 10 or more items get a 10% bulk discount.
const BULK_DISCOUNT_MIN_ITEMS = 10;
const BULK_DISCOUNT_RATE = 0.1;

@ZIJ ZIJ Aug 26, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: off-by-one in discount condition — should be >= not >.

The PR description states "10 or more items", but itemCount > BULK_DISCOUNT_MIN_ITEMS skips the discount for exactly 10 items.

// current (wrong)
if (itemCount > BULK_DISCOUNT_MIN_ITEMS) {

// fix
if (itemCount >= BULK_DISCOUNT_MIN_ITEMS) {

export function orderTotalCents(order: Order): number {
return order.items.reduce(
const subtotal = order.items.reduce(

@ZIJ ZIJ Aug 26, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug (wrong operator): The comment and PR description say "10 or more items", but > means the discount only kicks in at 11+. Change to >= BULK_DISCOUNT_MIN_ITEMS.

(sum, item) => sum + item.quantity * item.unitPriceCents,
0,

@ZIJ ZIJ Aug 26, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discount threshold is exclusive, but the spec says "10 or more".

itemCount > 10 means the discount only applies from 11 items onward. Change to >= to match the stated behaviour:

if (itemCount >= BULK_DISCOUNT_MIN_ITEMS) {

@ZIJ ZIJ Aug 26, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong threshold comparison. BULK_DISCOUNT_MIN_ITEMS is 10 and the PR description says "10 or more items", but > excludes exactly 10. Change to >=:

if (itemCount >= BULK_DISCOUNT_MIN_ITEMS) {

);

@ZIJ ZIJ Aug 26, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug (floating-point cents): subtotal * (1 - BULK_DISCOUNT_RATE) can produce a non-integer (e.g. 1 * 0.9 = 0.9). Wrap in Math.round(...) to preserve the integer-cents contract.

const itemCount = order.items.reduce((sum, item) => sum + item.quantity, 0);
if (itemCount > BULK_DISCOUNT_MIN_ITEMS) {
return subtotal * (1 - BULK_DISCOUNT_RATE);
}
return subtotal;
}
4 changes: 3 additions & 1 deletion fixture/src/server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,9 @@ app.get("/orders/:id", (req, res) => {
});

app.get("/customers/:customerId/orders", (req, res) => {

@ZIJ ZIJ Aug 26, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Query parameter parsing has no validation.

Number(undefined) → 0, Number("abc") → NaN, and negative values pass through silently. page = 0 makes start negative and all.slice returns the full array. Consider using parseInt with a fallback and rejecting invalid values with a 400 response before calling listOrders.

@ZIJ ZIJ Aug 26, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-numeric query strings (e.g. ?page=abc) cause Number(...) to return NaN, which silently produces an empty result. Add a fallback or validate and return 400:

const page = Math.max(1, Number(req.query.page) || 1);
const pageSize = Math.min(100, Math.max(1, Number(req.query.pageSize) || 20));

@ZIJ ZIJ Aug 26, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Issue: no validation of page / pageSize query parameters.

Number('abc') → NaN; Number('') → 0. Both propagate silently into listOrders and produce incorrect slices. Consider:

const page = parseInt(req.query.page as string, 10);
const pageSize = parseInt(req.query.pageSize as string, 10);
if (!Number.isInteger(page) || page < 1 || !Number.isInteger(pageSize) || pageSize < 1) {
  return res.status(400).json({ error: 'page and pageSize must be positive integers' });
}

res.json(listOrders(req.params.customerId));
const page = Number(req.query.page ?? 1);

@ZIJ ZIJ Aug 26, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug (missing validation): Number("abc") is NaN, which silently produces an empty result instead of an error. Validate that page and pageSize are positive integers and return HTTP 400 if not.

const pageSize = Number(req.query.pageSize ?? 20);
res.json(listOrders(req.params.customerId, page, pageSize));
});

const port = Number(process.env.PORT ?? 3000);
Expand Down
Loading