7 Things I Look For Before I Approve a Pull Request

Code review is one of those skills nobody really teaches you. You get thrown into it, you start by nitpicking variable names, and eventually — usually after shipping a bug that a five-second look would have caught — you develop an actual system.

This is the system I use. It's not exhaustive and it's not fancy. It's just seven questions I ask myself on every PR, roughly in this order, before I click approve. I've included real code examples for each one — a bad version, a better version, and what I'm actually looking for as a reviewer.

Let's get into it.

1. Does the code actually solve the problem?

This sounds obvious, but it's the check people skip most often, because it's tempting to jump straight into reading syntax instead of first asking "does this match the ticket?"

I read the PR description and the linked issue before I read a single line of code. Then I ask: if I ran this exact scenario, would it produce the right outcome? Not "does this look like reasonable code" — does it solve the actual problem.

Bad version — closes the ticket "filter out inactive users" but silently changes behavior elsewhere:

function getUsers(users) {
  return users.filter(u => u.active && u.role !== 'guest');
}

The ticket asked for inactive users to be filtered out. This PR also drops guests — a requirement nobody asked for, that might break another screen relying on guest visibility.

Improved version:

function getActiveUsers(users) {
  return users.filter(u => u.active);
}

Scoped exactly to what was requested. If guest filtering is genuinely needed, it belongs in its own PR with its own ticket and its own tests.

What to notice as a reviewer: scope creep disguised as a fix. Extra changes "while I was in there" are one of the most common sources of surprise regressions. If the PR does more than the ticket asked for, ask why, out loud, in the comments.

2. Is the code understandable?

Working code and readable code are not the same thing. I try to read each function the way I'd read it cold, six months from now, with zero memory of writing it.

Bad version:

def calc(d, t):
    r = []
    for x in d:
        if x[2] > t:
            r.append((x[0], x[1] * 1.1))
    return r

I have no idea what this does without tracing through it. What's d? What's index 2? Why 1.1?

Improved version:

def apply_discount_to_high_value_orders(orders, threshold):
    discounted_orders = []
    for order in orders:
        if order.total > threshold:
            discounted_orders.append((order.id, order.total * 1.1))
    return discounted_orders

Same logic, but now I can understand the function's purpose from its name and its variables, without running it in my head.

What to notice as a reviewer: if you have to open a second tab, trace three function calls, or ask the author "wait, what does this do?" in a comment, that's a readability problem — not a you problem. Flag it. Naming and structure are load-bearing, not cosmetic.

3. What happens when something goes wrong?

Most demos work. The question is what the code does when the network drops, the API returns a 500, or a file doesn't exist. I specifically look for what happens on the unhappy path, because that's the path nobody tests manually before opening the PR.

Bad version:

async function fetchUserProfile(userId) {
  const response = await fetch(`/api/users/${userId}`);
  const data = await response.json();
  return data;
}

No handling for a failed request, a non-200 response, or malformed JSON. One flaky network call and this throws an unhandled exception somewhere upstream, with a stack trace that tells the user nothing useful.

Improved version:

async function fetchUserProfile(userId) {
  let response;
  try {
    response = await fetch(`/api/users/${userId}`);
  } catch (err) {
    throw new Error(`Network error fetching user ${userId}: ${err.message}`);
  }

  if (!response.ok) {
    throw new Error(`Failed to fetch user ${userId}: ${response.status}`);
  }

  return response.json();
}

Now failures are explicit, informative, and distinguishable — a caller can tell the difference between "the network died" and "the server said no."

What to notice as a reviewer: any await, file read, external call, or parse step with no corresponding failure path. Ask: "what does the user see if this line throws?" If the honest answer is "a blank screen" or "an unhandled promise rejection in the console," that's not ready to merge.

4. Are edge cases handled?

This is different from error handling. Edge cases are inputs that are technically valid but easy to forget: empty lists, zero, negative numbers, duplicate entries, the very first or very last item, extremely long strings.

Bad version:

function getAverageScore(scores) {
  const total = scores.reduce((sum, s) => sum + s, 0);
  return total / scores.length;
}

Pass in an empty array and you get NaN, silently, with no error and no signal that anything went wrong.

Improved version:

function getAverageScore(scores) {
  if (scores.length === 0) {
    return null; // no scores yet — caller decides how to display this
  }
  const total = scores.reduce((sum, s) => sum + s, 0);
  return total / scores.length;
}

Now the empty case is a deliberate decision instead of an accident, and it's documented right there in the code.

What to notice as a reviewer: try to mentally run the function on an empty input, a single-item input, and a huge input. If the author clearly only ever tested with "3-5 typical items," that's usually where the bugs are hiding. Ask specifically: "what happens with zero items?" — it's a cheap question that catches a lot.

5. Is the code secure?

You don't need to be a security engineer to catch the common stuff. Most security bugs in day-to-day PRs aren't exotic — they're unsanitized input, secrets in code, or overly trusting data from outside the system.

Bad version:

app.get('/search', (req, res) => {
  const query = `SELECT * FROM products WHERE name = '${req.query.name}'`;
  db.execute(query, (err, results) => {
    res.json(results);
  });
});

Classic SQL injection — anything the user types goes straight into the query string.

Improved version:

app.get('/search', (req, res) => {
  const query = 'SELECT * FROM products WHERE name = ?';
  db.execute(query, [req.query.name], (err, results) => {
    if (err) return res.status(500).json({ error: 'Search failed' });
    res.json(results);
  });
});

Parameterized queries mean user input is data, never executable SQL, no matter what someone types into the search box.

What to notice as a reviewer: any spot where user input touches a query, a shell command, a file path, or gets rendered directly into HTML. Also scan for hardcoded API keys, tokens, or passwords — even in a comment, even "temporarily." And check whether data from external APIs is validated before being trusted, the same way you'd validate user input.

6. Are the tests meaningful?

A PR with 100% coverage can still have useless tests. I look at whether the tests actually verify behavior, or whether they just exercise the code without asserting anything meaningful.

Bad version:

test('calculates discount', () => {
  const result = applyDiscount(100, 0.1);
  expect(result).toBeDefined();
});

This test passes no matter what applyDiscount returns, as long as it returns something. It would still pass if the function returned -999.

Improved version:

test('applies a 10% discount correctly', () => {
  expect(applyDiscount(100, 0.1)).toBe(90);
});

test('returns original price when discount is zero', () => {
  expect(applyDiscount(100, 0)).toBe(100);
});

test('throws on a negative discount rate', () => {
  expect(() => applyDiscount(100, -0.1)).toThrow();
});

Each test verifies a specific, checkable outcome — including an edge case and an invalid input.

What to notice as a reviewer: look for assertions like toBeDefined(), toBeTruthy(), or no assertion at all — these are common signs of a test written to satisfy a coverage number rather than to catch a regression. Also check whether the tests cover the failure paths and edge cases from sections 3 and 4 above, not just the happy path. A test suite that only tests success is only half a test suite.

7. Will another developer understand this six months from now?

This is the catch-all question, and I ask it last because by this point I've usually already spotted the answer. It's about intent, not syntax: will someone (possibly you) be able to figure out why this code exists, not just what it does.

Bad version:

// fix
if (user.type === 3) {
  discount = 0.15;
}

What is type 3? Why 0.15? Six months from now, this is an archaeology project.

Improved version:

const USER_TYPE_ENTERPRISE = 3;
const ENTERPRISE_DISCOUNT_RATE = 0.15;

// Enterprise customers get a 15% discount per the Q3 pricing agreement (JIRA-4821)
if (user.type === USER_TYPE_ENTERPRISE) {
  discount = ENTERPRISE_DISCOUNT_RATE;
}

Named constants explain what, the comment explains why, and the ticket reference gives anyone a way to dig deeper if they need to.

What to notice as a reviewer: magic numbers, cryptic comments like // fix or // hack, and any logic that encodes a business rule without saying where that rule came from. If you, the reviewer, have to ask "why does this exist?" in the PR comments, future-you (or future-someone) will have to ask the same question with nobody around to answer it.

The Pull Request Checklist

Here's the condensed version I actually use, PR by PR:

• [ ] Solves the problem — matches the ticket, no unrelated scope creep
• [ ] Readable — I can follow the logic without re-reading it three times
• [ ] Handles failure — network errors, bad responses, and exceptions are caught and explicit
• [ ] Handles edge cases — empty input, zero, single item, duplicates, extremes
• [ ] Secure by default — no raw string interpolation into queries/commands, no hardcoded secrets, external input is validated
• [ ] Tests actually assert something — happy path, at least one edge case, at least one failure case
• [ ] Understandable in six months — no unexplained magic numbers, comments explain why not what

None of these checks take more than a minute or two individually. Together, they take maybe ten to fifteen minutes on a normal-sized PR — and they catch the vast majority of the bugs and headaches that would otherwise surface in production, at 2am, with much less context than you have right now.

Review is cheap. Production incidents aren't. That trade-off is really the whole point of this checklist.