Clean code: lỗi hay gặp khi viết và refactor code backend Trung cấp

Trang này gom các mẫu code dev hay viết thật, kèm bản nên viết và hậu quả cụ thể. Hành vi runtime trong các ví dụ đã chạy thử trên Node 24.

Nhóm	Lỗi
Quy trình	Refactor lẫn sửa bug
Hàm	Flag boolean, if lồng, tách theo số dòng, magic number
Lỗi	Nuốt lỗi, fetch không check ok
Dữ liệu	Tiền số thực, mutate input, getter có side effect
Testability	new Date() trong core
Cấu trúc	Code bị comment, DRY sai, god service

Refactor và sửa bug chung một commit

Một commit vừa đổi cấu trúc vừa đổi hành vi

✕ Lịch sử commit tồi

a1b2c3d refactor checkout + fix rounding + fix payment error

✓ Lịch sử commit tốt

4e5f6a7 test: characterization cho quoteTotalVnd (ghim hành vi cũ)
8b9c0d1 refactor: guard clause, extract subtotal/shipping
2e3f4a5 refactor: inject Clock, tách quoteTotalVnd thuần
6b7c8d9 fix: không lưu order khi charge lỗi
0a1b2c3 fix: làm tròn VND, giờ Asia/Ho_Chi_Minh

Vì sao

• Golden master đỏ thì không ai biết là lỗi refactor hay thay đổi cố ý.
• Production có sự cố, git revert commit gộp gỡ luôn bản sửa bug.
• Tách ra: commit refactor: giữ test xanh không sửa test, commit fix: đổi test có chủ đích.

Tham số boolean ở public method

Flag argument làm call site không đọc được

✕ Hay gặp

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

// Call site, 3 tháng sau:
await svc.checkout(id, items, null, true, false);

✓ Nên viết

quote(input: QuoteInput): number { /* thuần, không side effect */ }
async placeOrder(cmd: PlaceOrderCommand): Promise<PlaceOrderResult> { /* có side effect */ }

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

Vì sao

• true, false không ai biết là express hay dryRun. Đảo hai giá trị vẫn compile, và tính tiền thật trong khi tưởng chỉ báo giá.
• dryRun đổi hẳn hành vi (có/không ghi DB), nên xứng đáng là hai method. express thành union 'standard' | 'express' để thêm same_day không đẻ thêm boolean.

if lồng nhiều tầng

Đường chính chìm ở tầng thụt thứ ba

✕ Hay gặp

if (items && items.length > 0) {
  const user = await findUser(userId);
  if (user) {
    if (user.status === 'active') {
      // ... 20 dòng đường chính ...
    } else {
      throw new Error('error');
    }
  } else {
    throw new Error('error');
  }
}
return 0;

✓ Nên viết

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' };

// đường chính từ đây, sát lề trái

Vì sao

• Reviewer phải nhớ ngăn xếp điều kiện mới biết dòng 15 chạy khi nào. Thêm một nhánh là dễ đặt nhầm else.
• Bản bad còn trả 0 cho giỏ rỗng và cùng 'error' cho hai lý do khác nhau: client không phân biệt được.

Tách hàm theo số dòng

Mười hàm ba dòng, mỗi hàm chỉ gọi hàm kế tiếp

✕ Hay gặp

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); }

✓ Nên viết

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);
}

Vì sao

• Người đọc phải nhảy qua bốn file/hàm mới thấy luồng; tên step2 không thêm thông tin nào. Đây là điểm Ousterhout phê phán: interface nông.
• Tách khi tên mới nói được nhiều hơn thân hàm. Một hàm 30 dòng tuần tự thường dễ đọc hơn.

Magic number không có đơn vị

TIMEOUT = 30 là giây hay mili giây?

✕ Hay gặp

const TIMEOUT = 30; // người viết nghĩ là 30 giây

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

✓ Nên viết

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;

Vì sao

• AbortSignal.timeout() nhận mili giây. Chạy thử với server trả lời sau 200 ms: TIMEOUT = 30 ném TimeoutError, tức gần như mọi lệnh charge đều fail ở production.
• 500000 rải ở nhiều file: marketing đổi ngưỡng freeship, sửa sót một chỗ là giỏ hàng và hoá đơn lệch nhau.

catch rồi log hoặc return null

Nuốt lỗi hạ tầng

✕ Hay gặp

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; }
}

✓ Nên viết

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: để lỗi DB nổi lên; null chỉ nghĩa là "không có bản ghi"

Vì sao

• Mất mạng tới cổng thanh toán: code bad vẫn INSERT, order được lưu mà chưa thu tiền.
• catch { return null }: DB rớt kết nối biến thành “khách không tồn tại”. Caller trả 404, không có alert, dữ liệu sai lan đi.
• { cause: err } giữ lỗi gốc (ECONNRESET) kèm stack, message thêm đơn nào, bao nhiêu tiền.

Tin rằng fetch reject khi HTTP lỗi

Không kiểm tra response.ok

✕ Hay gặp

try {
  await fetch('https://pay.example.com/charges', { method: 'POST', body });
  await saveOrder(); // nghĩ rằng tới đây là đã charge thành công
} catch (e) {
  throw new PaymentUnavailableError('charge failed', { cause: e });
}

✓ Nên viết

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();

Vì sao

• fetch() chỉ reject khi lỗi mạng. Chạy thử với server trả 500: promise resolve với status: 500, ok: false.
• Cổng thanh toán trả 402/500 mà code bad vẫn lưu order. Phân loại status (declined vs retryable) nên nằm trong adapter.

Nhân tiền với số thực, không làm tròn

total * 0.9 trên VND

✕ Hay gặp

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

✓ Nên viết

export const VIP10_PRICE_FACTOR = 0.9;

// VND không có đơn vị lẻ: tính xong làm tròn về số nguyên một lần, ở một chỗ
const discountedVnd = Math.round(subtotalVnd * VIP10_PRICE_FACTOR);

Vì sao

• Demo golden master trong cụm này ra 587293.2000000001 VND từ code cũ. Số lẻ đi thẳng vào DB, hoá đơn và request thanh toán; tuỳ kiểu cột và driver mà nó bị lưu nguyên, làm tròn ngầm hoặc lỗi insert. Không cái nào là quyết định có chủ đích.
• Làm tròn là đổi hành vi: đi trong commit fix: riêng, không lén vào commit refactor.

new Date() và Math.random() trong logic nghiệp vụ

Đồng hồ và random giấu trong core

✕ Hay gặp

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';
}

✓ Nên viết

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, inject để test
}

Vì sao

• getHours() dùng timezone của process. Chạy thử 09:00 giờ VN: TZ=UTC trả 2, TZ=Asia/Ho_Chi_Minh trả 9. Container UTC áp coupon sai khung giờ 7 tiếng.
• Test chỉ pass buổi sáng, đỏ buổi chiều: flaky. Math.random() khiến user đổi variant mỗi lần reload và không viết được assert.
• Truyền now (hoặc inject Clock) và hàm bucket vào: test hàng nghìn case không mock global.

Mutate tham số đầu vào

Hàm tính toán sửa luôn mảng của caller

✕ Hay gặp

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); // báo giá
applyMemberDiscount(cart.lines); // đặt đơn: giảm thêm lần nữa

✓ Nên viết

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);
}

Vì sao

• Chạy thử: gọi bản bad hai lần trên giỏ 100.000 VND còn 81.000, khách được giảm 19% thay vì 10%. sort() cũng đổi thứ tự mảng gốc, UI hiển thị lộn.
• readonly CartLine[] làm lines.sort() báo lỗi compile; toSorted() (ES2023, có trong Node 20+) trả mảng mới.

Getter có side effect

Đọc thuộc tính mà đổi trạng thái

✕ Hay gặp

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 });

✓ Nên viết

class Cart {
  get totalVnd(): number { // query thuần, đọc bao nhiêu lần cũng như nhau
    return this.coupon ? this.subtotalVnd - this.coupon.amountVnd : this.subtotalVnd;
  }
}

// side effect là một lệnh có tên, gọi đúng một lần sau khi charge thành công
await this.coupons.markUsed(cart.coupon.code, orderId);

Vì sao

• Chạy thử: dòng log in 250000, lệnh charge lấy 300000. Thêm một dòng log làm đổi số tiền thu.
• Getter đọc như phép gán, không ai nghĩ nó có side effect. Đây là Separate Query from Modifier của Fowler.

Để lại code bị comment và TODO vô chủ

Code chết trong file

✕ Hay gặp

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

✓ Nên viết

await this.orders.save(order);
// Gửi receipt chuyển sang consumer của OrderPlaced (ORD-412), không gửi ở đây.
const subtotalVnd = sumLinesVnd(lines);

Vì sao

• Dòng mailer bị comment khiến người sau hỏi “bật lại hay không?”, và có người bật lại thật: khách nhận hai email receipt. Git đã nhớ code cũ.
• TODO không ticket không bao giờ được làm. Comment “loop qua items” kể lại code, xoá và đặt tên tốt hơn; comment vì sao thì giữ.

Gom trùng lặp ở lần thứ hai bằng flag

Abstraction sai: hàm chung đầy boolean

✕ Hay gặp

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); // mỗi caller một tổ hợp cố định

✓ Nên viết

// Hai quy tắc đổi vì hai lý do khác nhau: giữ hai hàm
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;
}

Vì sao

• Đây là bước 2 trong chuỗi của Sandi Metz. Đổi phí B2B phải đọc cả nhánh retail, sửa một if là lỡ đổi luôn phí retail.
• Lối ra: inline lại vào từng caller, xoá phần không dùng. Chờ trường hợp thứ ba mới tìm abstraction.

God service

Một service biết mọi thứ

✕ Hay gặp

@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 dòng
}

✓ Nên viết

@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... mỗi class phụ thuộc đúng thứ nó dùng

Vì sao

• Test checkout phải dựng 10 mock, kể cả PdfRenderer không liên quan. Ai cũng sửa file này nên PR nào cũng conflict.
• Đổi chữ ký SmsClient buộc review lại cả luồng thanh toán. Tách theo use case để mỗi class đổi vì một lý do.

Tiếp theo: quay lại Tổng quan Clean code để dùng checklist khi review, hoặc xem cụm Code review.