Code Review Best Practices: From Both Sides of the Table

I once submitted a 2,000-line PR and got back one comment: "LGTM." Looked good to them. Merged it. Two weeks later we found three bugs that any careful review would have caught.

The reviewer wasn't lazy—they were overwhelmed. Nobody can meaningfully review 2,000 lines. Their brain just checked out.

Code review is a skill on both sides. As an author, you can make it easy or impossible for reviewers to help you. As a reviewer, you can teach or you can demoralize. Here's what I've learned.

For Authors: Make It Easy

Keep It Small

There's an old joke: "10 lines of code = 10 issues found. 500 lines of code = 'Looks good to me.'"

Big PRs get rubber-stamped because reviewers get fatigued. Aim for under 400 lines. Building a big feature? Split it: database changes first, then API, then UI. Each PR is reviewable.

Explain the Why

A PR description that just says "Fixes JIRA-123" is useless. The reviewer has to context-switch to another tool, read a ticket, piece together what you did.

Good description:

## What
Adds retry logic to payment API calls.

## Why
5% of requests were failing from network blips. This improves UX.

## How to test
1. Go to checkout
2. Disconnect wifi briefly while clicking Pay
3. Observe it retries before failing

Self-Review First

Before assigning reviewers, look at your own diff. You'll catch console.logs, commented code, and obvious issues. Sending a messy PR signals you don't care—which invites reviewers to also not care.

For Reviewers: Be Helpful, Not Harsh

Tone Matters

Text is easily misread. A short comment sounds aggressive.

• ❌ "You forgot to close the connection. Fix it."
• ✅ "The connection might stay open if this throws. Should we add a finally block?"

Questions open dialogue. Commands shut it down. And maybe the author knows something you don't.

Categorize Your Comments

Not all feedback is equal. Blocking bugs are different from style preferences. Make it clear:

// [Blocking]: This SQL is vulnerable to injection
// [Nit]: I'd name this 'fetchUserData' instead of 'getData'
// [Question]: Why not use the shared utility for this?

Now the author knows what must be fixed versus what's optional.

Automate the Boring Stuff

Humans shouldn't argue about semicolons and indentation. That's what linters and formatters are for. Set up ESLint/Prettier/Black in CI. Block merges if formatting fails. Now reviews focus on logic, not style.

Offer Solutions

Don't just say "this is wrong." Explain why and suggest an alternative:

// ❌ Unhelpful
"Don't use string literals here."

// ✅ Helpful
"Using 'admin' as a string makes refactoring hard later. 
Consider using the UserRole enum from constants.js."

When You Disagree

Disagreements are healthy. But if you're going back and forth more than three times in comments, stop typing. Get on a call. Five minutes of conversation resolves what two days of comment threads cannot.

And if you're the author disagreeing, don't just ignore the comment. Reply with your reasoning:

"I see your point about performance, but this only runs once per day. I'm optimizing for readability here."

Quick Checklists

Authors:

• Is the PR under 400 lines?
• Did I explain what and why?
• Did I self-review first?

Reviewers:

• Am I being kind?
• Did I separate blocking issues from nits?
• Did I say "nice work" on the good parts?

Code review is a team sport. Done well, it makes everyone better. Done poorly, it creates resentment and rubber-stamps bugs into production.