Clean code: common mistakes when writing and refactoring backend code Intermediate

This page collects code patterns devs really write, with the version to write instead and the concrete consequence. The runtime behavior in the examples was tested on Node 24.

Group	Mistake
Process	Refactoring mixed with bug fixes
Functions	Boolean flags, nested ifs, splitting by line count, magic numbers
Errors	Swallowed errors, fetch without an ok check
Data	Money as floats, mutated inputs, getters with side effects
Testability	new Date() in the core
Structure	Commented-out code, wrong DRY, god service

Refactoring and bug fixes in the same commit

One commit that changes both structure and behavior

✕ Bad commit history

a1b2c3d refactor checkout + fix rounding + fix payment error

✓ Good commit history

4e5f6a7 test: characterization for quoteTotalVnd (pin old behavior)
8b9c0d1 refactor: guard clause, extract subtotal/shipping
2e3f4a5 refactor: inject Clock, extract pure quoteTotalVnd
6b7c8d9 fix: don't save order when charge fails
0a1b2c3 fix: round VND, Asia/Ho_Chi_Minh time

Why

• When the golden master goes red, nobody knows whether it’s a refactoring mistake or an intentional change.
• When production has an incident, git revert on the combined commit also removes the bug fix.
• Split them: refactor: commits keep tests green without touching tests; fix: commits change tests deliberately.

Boolean parameters on public methods

Flag arguments make the call site unreadable

✕ Common

async checkout(userId: string, items: Item[], coupon: string | null, express: boolean, dryRun: boolean) {
  // ...
  if (!dryRun) { await this.charge(...); await this.save(...); }
}

// Call site, 3 months later:
await svc.checkout(id, items, null, true, false);

✓ Prefer

quote(input: QuoteInput): number { /* pure, no side effects */ }
async placeOrder(cmd: PlaceOrderCommand): Promise<PlaceOrderResult> { /* has side effects */ }

await svc.placeOrder({ userId, lines, shipping: 'express', idempotencyKey });

Why

• Nobody knows whether true, false is express or dryRun. Swapping the two still compiles, and charges real money while you think you’re only quoting.
• dryRun completely changes behavior (writes to the DB or not), so it deserves two methods. express becomes the union 'standard' | 'express' so adding same_day doesn’t spawn another boolean.

Deeply nested ifs

The main path buried at the third indentation level

✕ Common

if (items && items.length > 0) {
  const user = await findUser(userId);
  if (user) {
    if (user.status === 'active') {
      // ... 20 lines of main path ...
    } else {
      throw new Error('error');
    }
  } else {
    throw new Error('error');
  }
}
return 0;

✓ Prefer

if (cmd.lines.length === 0) return { ok: false, error: 'EMPTY_CART' };
const customer = await this.orders.findCustomer(cmd.userId);
if (!customer) return { ok: false, error: 'CUSTOMER_NOT_FOUND' };
if (customer.status !== 'active') return { ok: false, error: 'CUSTOMER_INACTIVE' };

// main path from here, against the left margin

Why

• The reviewer has to keep a stack of conditions in mind to know when line 15 runs. Adding one more branch makes it easy to misplace an else.
• The bad version also returns 0 for an empty cart and the same 'error' for two different reasons: the client can’t tell them apart.

Splitting functions by line count

Ten three-line functions, each just calling the next

✕ Common

function process(order: Order) { return step1(order); }
function step1(order: Order) { validate(order); return step2(order); }
function step2(order: Order) { const t = calc(order); return step3(order, t); }
function step3(order: Order, t: number) { return finalize(order, t); }

✓ Prefer

function quoteTotalVnd(input: QuoteInput, now: Date): number {
  if (input.lines.length === 0) return 0;
  const subtotal = subtotalVnd(input.lines);
  const discounted = applyCoupon(subtotal, input.couponCode, now);
  return discounted + shippingFeeVnd(discounted, input.shipping);
}

Why

• The reader has to jump across four files/functions to see the flow; the name step2 adds no information. This is what Ousterhout criticizes: shallow interfaces.
• Split when the new name says more than the function body. A sequential 30-line function is often easier to read.

Magic numbers without units

Is TIMEOUT = 30 seconds or milliseconds?

✕ Common

const TIMEOUT = 30; // the author thinks this is 30 seconds

const res = await fetch(PAYMENT_URL, { signal: AbortSignal.timeout(TIMEOUT) });
if (total > 500000) shippingFee = 0;

✓ Prefer

const PAYMENT_TIMEOUT_MS = 30_000;
const FREE_SHIPPING_THRESHOLD_VND = 500_000;

const res = await fetch(PAYMENT_URL, { signal: AbortSignal.timeout(PAYMENT_TIMEOUT_MS) });
if (total > FREE_SHIPPING_THRESHOLD_VND) shippingFee = 0;

Why

• AbortSignal.timeout() takes milliseconds. Tested against a server that responds after 200 ms: TIMEOUT = 30 throws TimeoutError, meaning almost every charge fails in production.
• 500000 scattered across files: marketing changes the free-shipping threshold, one spot is missed, and the cart and the invoice disagree.

catch, then log or return null

Swallowing infrastructure errors

✕ Common

try {
  await payments.charge({ userId, total });
} catch (e) {
  console.log(e);
}
await db.query('INSERT INTO orders (user_id, total) VALUES ($1, $2)', [userId, total]);

async function findCustomer(id: string) {
  try { return await repo.findById(id); } catch { return null; }
}

✓ Prefer

try {
  return await this.payments.charge({ amountVnd: totalVnd, idempotencyKey });
} catch (err) {
  throw new PaymentUnavailableError(
    `charge failed: user=${userId} amount=${totalVnd}VND key=${idempotencyKey}`,
    { cause: err },
  );
}

// findCustomer: let DB errors surface; null only means "no record"

Why

• Network loss to the payment gateway: the bad code still runs the INSERT, and the order is saved without payment.
• catch { return null }: a dropped DB connection becomes “customer doesn’t exist”. The caller returns 404, no alert fires, and wrong data spreads.
• { cause: err } keeps the original error (ECONNRESET) with its stack, and the message adds which order and how much money.

Believing fetch rejects on HTTP errors

Not checking response.ok

✕ Common

try {
  await fetch('https://pay.example.com/charges', { method: 'POST', body });
  await saveOrder(); // assumes reaching here means the charge succeeded
} catch (e) {
  throw new PaymentUnavailableError('charge failed', { cause: e });
}

✓ Prefer

const res = await fetch('https://pay.example.com/charges', { method: 'POST', body });
if (res.status === 402) return { status: 'declined' } as const;
if (!res.ok) {
  throw new PaymentUnavailableError(`charge failed: HTTP ${res.status} key=${idempotencyKey}`);
}
const charge = await res.json();

Why

• fetch() only rejects on network errors. Tested against a server returning 500: the promise resolves with status: 500, ok: false.
• The payment gateway returns 402/500 and the bad code still saves the order. Status classification (declined vs retryable) belongs in the adapter.

Multiplying money by floats without rounding

total * 0.9 on VND

✕ Common

if (coupon === 'VIP10') total = total * 0.9;
await db.query('INSERT INTO orders (total) VALUES ($1)', [total]);
// the golden master sees: 587293.2000000001

✓ Prefer

export const VIP10_PRICE_FACTOR = 0.9;

// VND has no fractional unit: round to an integer once, in one place, after computing
const discountedVnd = Math.round(subtotalVnd * VIP10_PRICE_FACTOR);

Why

• The golden master demo in this cluster gets 587293.2000000001 VND from the old code. The fraction flows straight into the DB, the invoice and the payment request. Depending on the column type and driver it’s stored as is, silently rounded, or fails the insert. None of these is a deliberate decision.
• Rounding is a behavior change: it goes in a separate fix: commit, not smuggled into a refactor commit.

new Date() and Math.random() in business logic

Clock and randomness hidden in the core

✕ Common

function applyCoupon(totalVnd: number, code?: string): number {
  if (code === 'VIP10' && new Date().getHours() < 12) return totalVnd * 0.9;
  return totalVnd;
}

function checkoutVariant(): 'A' | 'B' {
  return Math.random() < 0.1 ? 'B' : 'A';
}

✓ Prefer

const VN_HOUR = new Intl.DateTimeFormat('en-US', {
  timeZone: 'Asia/Ho_Chi_Minh', hour: 'numeric', hourCycle: 'h23',
});

function applyCoupon(totalVnd: number, code: string | undefined, now: Date): number {
  const hourVn = Number(VN_HOUR.format(now));
  if (code === 'VIP10' && hourVn < 12) return Math.round(totalVnd * 0.9);
  return totalVnd;
}

function checkoutVariant(userId: string, bucketOf: (id: string) => number): 'A' | 'B' {
  return bucketOf(userId) < 10 ? 'B' : 'A'; // bucket 0..99, injected for testing
}

Why

• getHours() uses the process’s timezone. Tested at 09:00 Vietnam time: TZ=UTC returns 2, TZ=Asia/Ho_Chi_Minh returns 9. A UTC container applies the coupon in a window shifted by 7 hours.
• Tests pass only in the morning and fail in the afternoon: flaky. Math.random() makes users switch variants on every reload, and you can’t write an assertion.
• Pass in now (or inject a Clock) and the bucket function: test thousands of cases without mocking globals.

Mutating input parameters

A computation function that also modifies the caller's array

✕ Common

function applyMemberDiscount(lines: CartLine[]): CartLine[] {
  for (const l of lines) l.priceVnd = l.priceVnd * 0.9;
  return lines.sort((a, b) => a.priceVnd - b.priceVnd);
}

applyMemberDiscount(cart.lines); // quote
applyMemberDiscount(cart.lines); // place order: discounted again

✓ Prefer

function applyMemberDiscount(lines: readonly CartLine[]): CartLine[] {
  return lines
    .map((l) => ({ ...l, priceVnd: Math.round(l.priceVnd * 0.9) }))
    .toSorted((a, b) => a.priceVnd - b.priceVnd);
}

Why

• Tested: calling the bad version twice on a 100,000 VND cart leaves 81,000; the customer gets 19% off instead of 10%. sort() also reorders the original array, so the UI shows items in the wrong order.
• readonly CartLine[] turns lines.sort() into a compile error; toSorted() (ES2023, in Node 20+) returns a new array.

Getters with side effects

Reading a property changes state

✕ Common

class Cart {
  couponUsed = false;
  get total(): number {
    if (this.coupon && !this.couponUsed) {
      this.couponUsed = true;
      return this.subtotal - 50_000;
    }
    return this.subtotal;
  }
}

logger.info({ total: cart.total }, 'checkout');
await payments.charge({ amountVnd: cart.total });

✓ Prefer

class Cart {
  get totalVnd(): number { // pure query, same result however many times you read it
    return this.coupon ? this.subtotalVnd - this.coupon.amountVnd : this.subtotalVnd;
  }
}

// the side effect is a named command, called exactly once after the charge succeeds
await this.coupons.markUsed(cart.coupon.code, orderId);

Why

• Tested: the log line prints 250000, the charge takes 300000. Adding a log line changes the amount charged.
• A getter reads like a field access; nobody expects a side effect. This is Fowler’s Separate Query from Modifier.

Leaving commented-out code and ownerless TODOs

Dead code in the file

✕ Common

await this.orders.save(order);
// await this.mailer.sendReceipt(user.email, total);
// TODO: fix this later
// loop over items adding to total
for (const l of lines) total += l.priceVnd * l.qty;

✓ Prefer

await this.orders.save(order);
// Receipts moved to the OrderPlaced consumer (ORD-412); don't send them here.
const subtotalVnd = sumLinesVnd(lines);

Why

• The commented-out mailer line makes the next person ask “turn it back on or not?”, and someone actually does: customers get two receipt emails. Git already remembers the old code.
• A TODO without a ticket never gets done. The “loop over items” comment narrates the code; delete it and name things better. Keep why comments.

Consolidating duplication on the second occurrence with flags

Wrong abstraction: a shared function full of booleans

✕ Common

function shippingFeeVnd(totalVnd: number, isB2B: boolean, isExpress: boolean, skipThreshold = false) {
  if (isB2B) return isExpress ? 80_000 : 0;
  if (!skipThreshold && totalVnd > 500_000) return 0;
  return isExpress ? 50_000 : 30_000;
}

shippingFeeVnd(t, false, true);
shippingFeeVnd(t, true, false, true); // each caller has a fixed combination

✓ Prefer

// Two rules that change for two different reasons: keep two functions
function retailShippingFeeVnd(totalVnd: number, method: ShippingMethod): number {
  if (totalVnd > FREE_SHIPPING_THRESHOLD_VND) return 0;
  return SHIPPING_FEE_VND[method];
}

function b2bShippingFeeVnd(method: ShippingMethod): number {
  return method === 'express' ? B2B_EXPRESS_FEE_VND : 0;
}

Why

• This is step 2 in Sandi Metz’s chain. Changing the B2B fee means reading the retail branch too, and editing one if can accidentally change the retail fee.
• The way out: inline it back into each caller and delete unused parts. Wait for a third case before looking for an abstraction.

God service

One service that knows everything

✕ Common

@Injectable()
export class OrderService {
  constructor(
    private db: Pool, private redis: Redis, private mailer: Mailer, private sms: SmsClient,
    private payments: PaymentClient, private inventory: InventoryClient, private coupons: CouponRepo,
    private analytics: Analytics, private pdf: PdfRenderer, private s3: S3Client,
  ) {}
  checkout() {} refund() {} cancel() {} exportInvoicePdf() {} sendReminder() {} recalcLoyalty() {}
  // ... 2,000 lines
}

✓ Prefer

@Injectable()
export class PlaceOrderUseCase {
  constructor(
    @Inject(ORDER_REPOSITORY) private readonly orders: OrderRepository,
    @Inject(PAYMENT_PORT) private readonly payments: PaymentPort,
    @Inject(CLOCK) private readonly clock: Clock,
  ) {}
  async execute(cmd: PlaceOrderCommand): Promise<PlaceOrderResult> { /* ... */ }
}
// RefundOrderUseCase, InvoicePdfService... each class depends on exactly what it uses

Why

• Testing checkout requires 10 mocks, including an unrelated PdfRenderer. Everyone edits this file, so every PR conflicts.
• Changing the SmsClient signature forces a re-review of the whole payment flow. Split by use case so each class changes for one reason.

Next: go back to the Clean code overview to use the checklist when reviewing, or see the Code review cluster.