Effective Code Reviews
How to foster a culture of learning through code review
Don’t want to read? Listen to this dude’s video, it covers most of what I have to say:
Or, since you’re here already — read on.
💭 Which one are you?
You might be one of these people:
1️⃣ You’re sitting at your desk, your hair turning white while scrolling through your colleague Alex’s PR. You know, that Alex, the one who always shortens variable names to incomprehensible gibberish, writes zero tests, and copy-pastes 3 files with only a few lines changed instead of taking the time to write a scalable solution. Your fingers are itching to write a comment for each variable name using camelCase instead of snake_case, leave a scathing review, show everyone how it’s done!!!
2️⃣ You’ve taken the time with your PR — added a link to the ticket, put some nice labels, even found a fun GIF! Then Greg, your lead engineer, leaves a review asking why you didn’t “just do X”. You could kindly tell Greg that X was the first thing you tried and it didn’t work for a very good reason. But since Greg didn’t ask politely you’ll snap at him and leave snarky comments on his PRs from now on. F-ing Greg! Hey, he started it!!
3️⃣ Pull requests come and go. You just don’t care to participate. What’s all the fuss about anyway? The app seems to work. Code quality? That’s just, like, your opinion, man 🤷♂️
4️⃣ Your pull requests are a pleasure to review, you start discussions by asking polite questions out of genuine curiosity, you’re proactive about proposing alternatives, and open to changing your mind while also knowing how to stand your ground.
Recognize anyone you know? Maybe yourself?
✅ How to approach code reviews
You might think reviewing code is about catching bugs. Surely that is the case sometimes. Yet is that all it’s good for?
🤔 But code review keeps our code clean, and clean code is good!
If maintaining clean code was the entirety of software development I would wholeheartedly agree. Other important parts are learning and maintaining good social relationships within your team — does telling people to “do it this way” without first asking why they did it it that way really help with those?
🤷♂️ It’s just code, we shouldn’t be that sensitive!
If you don’t want the people you work with to be sensitive does that mean you want them to be…insensitive?
If you as a reviewer aren’t willing to have an open and reasonable discussion on the best way to do something, but say “just do it my way” then who’s really the one really being emotional about code?
😔 Okay I’m listening. What are you suggesting instead?
✅ Do ask questions and start discussions
Even the most experienced developers occasionally make mistakes or incorrect assumptions. They get tired, stressed out, or distracted.
So is leaving a comment like this really the best way to go about it?
Extract this JSON to a separate file.
Ask, don’t tell. Instead of assuming the author was being lazy or didn’t know what they were doing, try to find out what was really going on.
Caveat 1: adding a question mark or “maybe?” after your statement does not make it a real question.
Caveat 2: rephrasing your statement as “why didn’t you” or worse — “why didn’t you just” does make it a real question, but a negative one.
How about turning it into a positive question? “Have you considered”, “have you tried”, “how about” (heeey — I used that one here), “what would happen if”, “would this idea work?”
✅ Do proactively propose solutions
Look for related documentation, articles, video, code examples, codepens, performance tests, libraries, tools. When proposing a code change — open a real code editor and try to type it out yourself first — you might find it doesn’t work out as well as you thought it would.
✅ Respect that accepting your solution is the author’s decision
Leave people express their personal coding style now and then. Your project’s code can look like it was written by the same super neat person and that’s okay. But it’s also okay if there’s some personality in there as long as everyone can understand and work with it.
Alex used a for … of loop instead of .forEach? What would happen if you let that slide and focused on whether the code does what it’s supposed to?
Here’s how to address some situations.
1️⃣ When code works well but could use small improvements
❌ Rarely request code changes
Requesting code changes (e.g. on GitHub) should be used sparingly, only as a last resort to warn everyone against obviously buggy code.
While PR authors can choose to dismiss the blocking review and merge the changes anyway, reviewers might take that as an open act of disrespect.
The red color in GitHub’s UI indicating “something’s wrong” doesn’t help either:
Getting a red-colored message from a dispassionate machine like a compiler, linter, or bot is to be expected; seeing it next to the picture of a real person you’re working closely with is another thing. Sooner or later there’s bound to be some resentment.
✅ Do leave a comment and withhold your approval until later
It costs you nothing and is completely neutral.
If the author simply ignores your comment without even acknowledging it then you might have bigger problems in your team than just “code quality”. Work on those first.
Also, if other team members don’t participate in the discussion and happily approve the PR despite your cautious comment, were the changes you wanted really that important after all?
2️⃣ When you notice incorrect formatting
❌ Don’t be a human linter
Machines are perfectly suited for linting and formatting code — they’re infinitely faster, cheaper, and more reliable in enforcing conformity than humans.
And humans program machines, turning many hours of manual human work into mere seconds.
Which job do you want to do?
✅ Do propose a lint config fix (or learn to let go)
If prettier and ESLint are okay with the code is it really worth talking about formatting? Unless it’s to discuss whether to change the lint rules long-term, I think not. This is after all why prettier exists in the first place:
By far the biggest reason for adopting Prettier is to stop all the on-going debates over styles.
And if we do decide to change the lint rules, we can do that in a separate PR instead of holding off on an important feature because there‘s no newline before that return statement.
3️⃣ When a PR has the same issue in multiple places
❌ Don’t leave a comment for each instance
This clutters the discussion timeline and spams everyone’s inbox. What if you only noticed 7 out of the 10 places this issue was present in? Would you rather automate the solution and get 100% of what you wanted in this and all future PRs, or get 70% only in this one PR?
✅ Do only leave 1 comment
Link to the other affected areas of the code, or leave instructions for the author to verify everything works on their own. Discuss long-term solutions like improving the linter configuration, building utility functions that make good practices easier to follow, or enhancing the project documentation.
And a final Do:
✅ Do stay curious and open to changing your mind
If you always insist on your own solution with no room for discourse does that mean you‘ve made up your mind forever?
Are you even the same person you were last year, or have some of your opinions changed ever so slightly? Do you want to think the exact same way a year from now, or do you want to be better?
To even have the opportunity of becoming better some day, you have to first be open to the possibility that you don’t know everything at present.
Genuine curiosity can shatter the concrete shell that judgment forms around our minds, and then willingness to learn can sprout up through the cracks.
So keep those cracks open — stay curious.
