🚫 Avoid these mistakes reviewing code (Part 1)
🚫 Avoid these mistakes reviewing code (Part 1)
5 common mistakes when reviewing code and tips to solve.
Hello everyone, Fran here👋
This week we reached the 500 subs mark! As an MVP this newsletter is validated. More posts to come!
There are two groups of people. Those who enjoy learning in Code Reviews (CRs) and those who perceive them as a painful, hostile environment. I think the latest is a bigger group.
“It’s a feature, not a bug”
We tie our perceived self-worth to our work. We don’t want to know the mistakes we have made. We even rationalize bugs as features.
Let’s explore common mistakes I have seen and better ways of handling those situations.
(This post considers CRs as an asynchronous process. The author sends a CR and receives comments from reviewers. This is iterated until reviewers approve and the change is merged)
🤷♂️ Mistake 1: Writing ambiguous comments
Reviewer: “Can we use this library? (link)”
Author: Yes.
Reviewer: Why didn’t we use it?
Author: We only use a small part of the library but it increases our js bundle size.
Reviewer: Isn’t it better than handling the edge cases?
Author: We have testing for that.
Reviewer: But you have a bug in line 43, I saw it yesterday when I posted the first comment
Author: You could have started with that…
❌ The error
The author overthinks what this question means. "Is it a genuine question or a rhetorical question to make me think?". "Is it a blocker or not for the approval?"
A rule of thumb is to avoid ending the comment on a question mark. It’s good to frame as questions and not commands, but don’t end there. Narrow down to where you see the problem and provide the necessary details without overwhelming.
Most times we don’t know what we want and just end the comment with "Thoughts?" to push responsibility to the author. It requires effort upfront to structure our thinking but it saves time in the the long run. The author is responsible for the code under review, but not for clearing your mind
Prevent the author from being blocked and prevent a ping-pong thread of comments, none of them adding any value.
As engineers, we are obsessed with minimizing system network roundtrips that take milliseconds but ignore human communication roundtrips that take hours.
✅ The solution
Use semantic comments. Put a label in each comment indicating what you mean. It can be as simple as "Nit", "FYI", or "Blocker".
Even better, use a convention others can reference, like conventional comments.
Don’t try to get the entire team to follow your new rules. Start doing it yourself and you’ll notice people asking.
A coworker of mine started using them just because he saw the tags in bold at the beginning of all my comments (e.g: [Suggestion - nonblocking])
🤏 Mistake 2: Writing extremely concise comments
Reviewer: Refactor this
❌ The error
The author is overthinking: “How do I know if the next changes will solve the comment or not?”
We think others can read our minds. They can’t. Document in writing your reasons, arguments, and what you expect.
✅ The solution
Write generous comments. Include code snippets and references.
From: "Can we simplify this?"
The author has to investigate some way to do it, without ever knowing if it will be enough for the reviewer. There isn’t a success criteria.
To: Can we simplify this using the spread operator of JS? <Code Snippet> <Link>
The author can see a picture of what success looks like and can go to the right source to learn about it.
Don’t put lengthy examples to demonstrate your opinionated approach is better. That’s just being pushy and can be perceived badly.
🗂️ Mistake 3: Making comments about everything
A new email arrived: “John Foo wrote 37 comments in your Code Review”
❌ The error
You read top-down in the order the CR UI presents the files. You ask questions that are answered by reading a bit further. You comment once per occurrence of some issue. And even with that, you miss some. This is overwhelming for a reviewer, even if you think you are doing them a favor.
For someone junior or new to your team, it can trigger their impostor syndrome and feel incompetent. For other personalities, this transforms into hostility. Even for someone working with you for a long time, it’s a pain to go one by one through the comments and answer the same to all
Avoid commenting on low-level details that become not applicable the moment the author addresses a high-level comment you made.
Why write 10 comments on the variables and methods of a class when you have a high-level comment to remove the class? You are likely showing up and feeding your ego. In reality, you are wasting your time writing them and the author’s time processing them.
✅ The solution
Approach review by stages:
0️⃣ Start by collapsing all changes and building a map of how the modified files are interconnected.
1️⃣ Does the change make sense? If it doesn’t, provide feedback quickly and offer an alternative. The author is not idle waiting for your comments but working on the next CR. Changing the approach means a bigger rework the later it’s done.
2️⃣ If high-level makes sense, go to the core files. Ignore the minor files and tests (you may want to check tests of core files to understand them better). Again, if you have big comments there, provide fast feedback without going into low-level details.
3️⃣ If no major comments that change the direction of the CR, go to the low-level details and tests. Cover them all. Don’t assume a method does what its name or docs in code say.
Also, flag once and indicate the issue is repeated through the CR. Flagging everything means the author just shuts down their brain and goes only to the places you pointed the issue, missing the ones where there was no comment.
👨🦳 Mistake 4: Taking forever to provide feedback
A new email arrived: John Foo sent a new Code Review. Stats: 43 files modified, 1328 lines
❌ The error
You may think sending a big CR is a mistake of the author. But what if I told you John Foo had his last CR under review for 5 business days, but it had only 2-3 revisions for 5 files and 200 lines?
The big CR is the result of all the work he was doing while waiting for someone to review
When reviews take time, as developers we protest about the strictness of the process and send large CRs. We think: “If I’m going to wait for 5 days, it's better that I send all changes at once”. This also discourages sending any code for housekeeping like cleaning up some code.
✅ The solution
Find review blocks during your day. If you are on a focused task, don’t stop by any means, it wouldn’t benefit the team. But we all have some breaks in our days: After coming from having lunch or after a meeting. Use these context switches already present in your day to dedicate time to review.
I’d also encourage you to track your personal SLA. Don’t stall your input for more than a business day. And provide a meaningful review, don’t write ambiguous comments just to justify yourself that you met your personal SLA.
🤬 Mistake 5: Using intense wording
Reviewer: Why are you creating this badly designed data structure when it’s obvious there are better ones in the standard library?
❌ The error
We don’t interpret comments as a gift but as criticism of our person.
In this post, I have used words such as mistake, error, and solution. This post is in the open, there are many recipients of the communication. You’ll read and take whatever learning you think applies to you.
In a CR, there is a single recipient: The author. It's easy to interpret a comment as an attack on the person and not on the code. I wouldn't frame things as mistakes but sugarcoat them more.
✅ The solution
Multiple little changes make the difference:
You → We.
Subject of the sentence: Author -> Code
Focusing on the problem -> Focusing on the solution. Write proactive comments, starting them with “What about...”, “Let’s...”
Blame on the author -> Blame on you. Explain you are a dumb person having a hard time understanding. Request simplifying to make it easier for other dumb developers. Explanations in CR comments are lost once you are in the code editor.
Reduce intensity: “Should”, “must”, “need”, “have to” → “Can”, “may”, “I think we should”
Use questions to reduce intensity. Be careful with (Mistake #1). Avoid bitter, sarcastic questions.
Notice where the author paid attention to detail and praise it. Humans learn more from positive reinforcement than negative. Be careful with offering bitter praise just to prove your point about a comment made previously
Everything applied at once makes a difference:
From: “You shouldn’t write this badly designed data structure and should use an existing one”
To: “Can we reuse this existing data structure (link)? I think it will be easier for future developers to understand and we don’t have to worry about all the testing and edge cases.”
🎯 Take action
These are just 5 out of many tips I could write. I’ll write another round of tips for sure.
But the tips are useless if you don’t apply them. You may think you are already doing things great. In that case, you would have stopped reading by the third mistake. If you are still here, pick one and give it a chance in your next CR:
Try not to end any comment with a question mark
Label your comments following some convention to express your intent
Schedule in your calendar some review blocks after meetings or every 2-3 hours. If there’s nothing to review, you take your time back for something else.
👏 Weekly Applause
A lot of nice people are supporting the newsletter! I want to thank:
Jordan Cutler recommended the newsletter in last week’s post of
and substack's recommendations. Jordan writes a great newsletter for career growth for software engineers.- recommended the newsletter in substack’s recommendations. Leonardo writes , a newsletter with real industry case studies and condensed lessons.
NK recommended the newsletter in a LinkedIn post. NK writes
, a very easy-to-read newsletter with real case studies of complex real-world architecturesAlex recommended the newsletter in a LinkedIn post. Alex writes Hungry Minds, a newsletter curating AI news from the past week.
All the people who have taken the time to chat with me on LinkedIn. It’s a big focus for me to add any real value and not just treat writing as a numbers game.
See you next week!
Very good tips and actionable advice. Thank you for sharing!
Fantastic article and examples, Fran! Loved the read and appreciate the mention!