7 Quick Tips for a Healthy Code Review Culture
The bad
Code Reviews can turn ugly very quickly, it has all the right ingredients:
Criticism, harsh tone, defensive replies, heated arguments, all that make the entire process irritating with little benefit to the project — A gigantic passive-aggressive colosseum for the software gladiators 🛡️⚔️.
And frankly, it shouldn’t be surprising, we all know that giving feedback is hard and receiving criticism is even harder. Plus chances are, nobody ever taught you how to do it properly. It’s not something you learn in college. Heck, it’s not even part of on the job training, you just learn it as you go.
So why do we keep doing it?
Because the devastating truth is — we need it, desperately.
The good
There are many benefits to doing CRs, from bug-proofing to knowledge sharing, or even keeping consistent code-style across the project.
It creates collaboration within the team, improves the code quality
and sparks insightful conversations which eventually ensures the robustness and sustainability of the project.
So how do we extract the good, without getting mixed up with the bad?
The healthy
A healthy CR is aware of its fragility and is more of an art than science.
It should be non-threatening and created in an inclusive environment,
where people tend to listen and are more open to constructive feedback.
Over the years (and after making countless mistakes), I created myself a list of goto rules to keep in mind when reviewing PRs. I hope these could help you as well, enjoy!
The rules
1
The Detect Suggest rule
As much as it’s easy to point out mistakes then go on your way, it’s also extremely unhelpful, CRs are a team effort, not a competition.
When you detect a problem, take the extra time to think of an alternative solution to suggest and only then comment.
Can’t think of one on the spot? You can always initiate a brainstorming thread.
Do less: “This will increase the number of requests going to the server.”
Do more: “This will increase the number of requests, perhaps we could implement some rate limit here? WDYT?”
2
The Huey rule
Try to avoid using “You” when talking to the author, prefer “We” instead.
“You” makes the feedback more personal, while “We” is directed to the code/team itself.
Do less: “You shouldn’t extract the userId from the response.”
Do more: “We don’t have to extract the userId from the response, we can get it from the context”
3
The dot rule
Do you text your friends with perfect punctuation?
Yeah, if you’re mad at them.
Ending a sentence with a dot feels too formal, it sounds more like a statement than a suggestion and leaves less room for a discussion.
Good feedbacks end with a question mark.
Open-ended questions are non-threatening nor judging, they offer room for clarifications and finding more suitable solutions that are both agreeable by the reviewer and the author.
Not sure what to ask? you can always end with the good old WDYT?
Oh! and throw some emojis here and there, it’s free 😇
Do less: “Remove this var.”
Do more: “Can we remove this var? or there’s some constraint I’m missing?”
4
The Dora the explorer rule
The tone of your review is greatly affected by your mindset.
When you see yourself as a detective, you will find criminals.
When you see yourself as an explorer, you’ll learn new things.
CRs are a two-way learning street. So position yourself as the explorer,
worst case you’ll learn something new.
Do less: “Don’t think we need a new lib, probably mess up our test suites.”
Do more: “Nice lib! 🤩 didn’t know we could use that! does it behave well with our test suits?”
5
The nitpick rule
Adjust your feedback to the complexity of the PR.
Try to avoid nitpick comments on complicated PRs, so the bigger picture won’t be missed.
Also, consider automating these repetitive things with prettifiers and linters. Your time and focus are better used elsewhere.
Do less: “Remove blank line” / “Capitalize the first letter in docstring” / “Grammar, *than not then”
Do more: Proactively add tooling, introduce grammar check extensions, create git hooks to run prettifiers & linters on committed files, etc.
6
The kind rejection rule
Rejections are hard (I learned that in high school), but sometimes we find issues that must be addressed before the PR can be merged.
And when that time comes to request changes, remember communication is key, and go talk to the author in person.
Make sure you understand each other, they have the tools to fix the issues and offer your help if needed.
Doing so refrain from future grudge-holding, speed up the fixes, and maintain a positive learning environment.
Do less: Changes Requested: “This introduces performance issues.”
Do more: Changes Requested: “This totally makes sense! I do see some performance issues here I’m worried about, let’s take it offline see how we can mitigate and proceed”
7
The positive feedback rule
Since CR is often viewed as “Searching for problems in other people’s code”,
negative feedback catches most of the attention. So reviewers tend to forget they can also give positive feedback from time to time.
Now I understand it’s not easy to think of unicorns and sparkles when you’re deep in review mode. But if you try, you’ll realize it’s more powerful than negative feedback.
When we give positive feedback, we actually put the focus on things we want to reinforce and it creates a positive environment. So it’s a win-win!
Do more: “This is an elegant solution!” / “Nice catch!”
To summarize:
- Don’t point a problem without providing a solution.
- Prefer “we” over “you”.
- Ask open-ended questions.
- Look for new things to learn.
- Ignore nitpicks on complex PRs / add automation.
- Talk to the author when requesting changes.
- Give positive feedback from time to time.
That’s It, Folks! 👋
WDYT? Do you agree with these points? have some of your own?
Feel free to share them in the comments!
And if you enjoyed this post, help me spread the word by:
- 🌐 Sharing it with your team.
- 👁 Following me on Medium.
- 👏 Oh! And don’t forget those claps! Oh, how I love those 👏👏👏.
Sincerely yours,
- Adir
