Code review is one of the highest-leverage activities in software development. A good review catches bugs, spreads knowledge, and raises the bar for the entire team. A bad review wastes everyone’s time — or worse, rubber-stamps broken code into production.

This is a practical checklist you can use on every pull request, followed by guidance on how to give feedback that actually helps.

The Checklist

1. Does the Code Do What It Claims?

Before anything else, read the PR description. Then read the code. Do they match?

// PR says: "Add rate limiting to the API"
// But the code actually does:
app.use(cors()); // This is CORS, not rate limiting

Surprisingly common. Sometimes the PR description is a copy-paste from a previous PR. Sometimes the scope drifted during implementation. Always verify intent matches implementation.

2. Are Edge Cases Handled?

The happy path works. It always works. What about:

  • Empty inputs: What happens with an empty array, null, undefined, or empty string?
  • Boundary values: Zero, negative numbers, MAX_SAFE_INTEGER, dates in the past
  • Concurrent access: Two users editing the same record at the same time
  • Network failures: API timeout, DNS resolution failure, partial response
// Missing edge case — what if users is empty?
function getOldestUser(users: User[]): User {
  return users.sort((a, b) => a.age - b.age).pop()!; // 💥 undefined on empty array
}

// Better
function getOldestUser(users: User[]): User | undefined {
  if (users.length === 0) return undefined;
  return users.reduce((oldest, user) =>
    user.age > oldest.age ? user : oldest
  );
}

3. Are There Tests? Do They Test the Right Things?

Tests exist. But do they actually verify behavior, or just exercise code?

// Useless test — asserts nothing meaningful
it('should work', async () => {
  const result = await processOrder(mockOrder);
  expect(result).toBeDefined(); // So what? Even an error object is "defined"
});

// Useful test — asserts specific behavior
it('applies 15% discount for loyalty customers with 1000+ points', async () => {
  const order = createOrder({ total: 100 });
  const result = applyLoyaltyDiscount(order, 1500);
  expect(result.total).toBe(85);
  expect(result.discountApplied).toBe(0.15);
});

Check for:

  • Are failure cases tested, not just success cases?
  • Are boundary conditions covered?
  • Would the test fail if the code was wrong? (Delete a line and see if a test breaks)

4. Is Error Handling Adequate?

Swallowed errors are time bombs:

// Silent failure — nobody will know this broke
try {
  await sendNotification(user);
} catch (e) {
  // TODO: handle this later
}

// Better — log, metric, or rethrow
try {
  await sendNotification(user);
} catch (error) {
  logger.error('Failed to send notification', { userId: user.id, error });
  metrics.increment('notification.send.failure');
  // Don't rethrow if notification failure shouldn't block the main flow
}

In Python:

# Bad — catches everything, hides bugs
try:
    process_payment(order)
except Exception:
    pass

# Good — specific exception, meaningful handling
try:
    process_payment(order)
except PaymentDeclinedError as e:
    logger.warning("Payment declined for order %s: %s", order.id, e)
    notify_customer_payment_failed(order)
except ConnectionError as e:
    logger.error("Payment gateway unreachable: %s", e)
    raise RetryableError("Payment gateway unavailable") from e

5. Is There Duplication?

Look for copy-pasted code with minor variations. It’s a signal that an abstraction is missing:

// Repeated pattern — fetch, validate, transform, save
async function updateUserName(userId: string, name: string) {
  const user = await db.users.findById(userId);
  if (!user) throw new NotFoundError('User not found');
  if (!name || name.length < 2) throw new ValidationError('Invalid name');
  user.name = name;
  await db.users.save(user);
}

async function updateUserEmail(userId: string, email: string) {
  const user = await db.users.findById(userId);
  if (!user) throw new NotFoundError('User not found');
  if (!email || !email.includes('@')) throw new ValidationError('Invalid email');
  user.email = email;
  await db.users.save(user);
}

// Better — extract the pattern
async function updateUserField<K extends keyof User>(
  userId: string,
  field: K,
  value: User[K],
  validate: (value: User[K]) => void,
) {
  const user = await db.users.findById(userId);
  if (!user) throw new NotFoundError('User not found');
  validate(value);
  await db.users.save({ ...user, [field]: value });
}

6. Are Names Clear?

Can you understand what a variable, function, or class does from its name alone? If you have to read the implementation to understand the name, the name is wrong.

7. Is the Change Scope Appropriate?

A PR that says “fix login bug” but also refactors the database layer, updates dependencies, and reformats 40 files is a red flag. Each PR should do one thing. If it does more, ask the author to split it.

8. Security Concerns

Watch for:

  • SQL injection: String concatenation in queries instead of parameterized queries
  • Exposed secrets: API keys, passwords, tokens in code or config files
  • Missing auth checks: New endpoints without authentication or authorization
  • Unvalidated input: User input passed directly to shell commands, file paths, or database queries
// SQL injection vulnerability
const query = `SELECT * FROM users WHERE email = '${email}'`;

// Safe — parameterized
const query = 'SELECT * FROM users WHERE email = $1';
await db.query(query, [email]);

9. Performance Red Flags

  • N+1 queries (fetching related records in a loop)
  • Unbounded queries (no LIMIT clause)
  • Large objects held in memory unnecessarily
  • Synchronous I/O blocking the event loop
// N+1 problem — one query per user
const users = await db.users.findAll();
for (const user of users) {
  user.orders = await db.orders.findByUserId(user.id); // 💥 N queries
}

// Fix — batch fetch
const users = await db.users.findAll();
const userIds = users.map(u => u.id);
const allOrders = await db.orders.findByUserIds(userIds); // 1 query

10. Documentation and Types

  • Are public APIs documented?
  • Are TypeScript types specific (not any everywhere)?
  • Would a new team member understand the code without asking the author?

How to Give Feedback

Be Specific

❌ "This code is confusing"
✅ "The variable `d` on line 42 could be renamed to `deliveryDate` for clarity"

Distinguish Severity

Use prefixes to signal importance:

  • nit: — Nitpick. Trivially small. Take it or leave it.
  • suggestion: — I think this is better but won’t block on it.
  • question: — I don’t understand this. Can you explain?
  • blocker: — This must be fixed before merging. Bug, security issue, or incorrect behavior.

Explain Why

❌ "Use `const` here"
✅ "Use `const` here — this variable is never reassigned, and `const` makes that intent explicit to future readers"

Ask, Don’t Tell

❌ "Move this to a separate function"
✅ "What do you think about extracting this block into a `validateOrderItems()` function?
    It would make the main function easier to scan."

Praise Good Work

Reviews aren’t only for criticism. If you see something clever, well-tested, or elegantly simple — say so. Positive reinforcement shapes culture more than criticism does.

"Nice use of discriminated unions here — really clean way to handle the different response types 👍"

Common Pitfalls

  1. Rubber-stamping: Approving without reading. Just don’t.
  2. Bikeshedding: Spending 30 minutes debating variable names while ignoring a missing null check.
  3. Tone: Written text lacks tone of voice. “Why did you do this?” reads as hostile. “Curious — what was the motivation for this approach?” reads as collaborative.
  4. Review size: If a PR is over 400 lines, ask the author to split it. Nobody can effectively review 2000 lines of changes.
  5. Blocking on style: If you have a formatter (Prettier, Black), let it handle style. Don’t argue about semicolons in reviews.

Make It a Habit

Print this checklist. Bookmark it. Use it on your next review. After a few weeks, it becomes second nature — and your team’s code quality will noticeably improve.