I originally posted this post on my blog a long time ago in a galaxy far, far away.
"Ask questions" is common advice for better code reviews.
At some point, we followed that advice and started using what I call "leading" or "pushy" questions. Questions that only hint a request for a code change.
After working on a remote software team for a couple years, I stopped using "pushy" questions on code reviews. Here's why it's a bad idea.
"Pushy" Questions Are Time-Consuming
Let's imagine we've written a method and forgot to check for nulls. Something like this,
public void DoSomething(OneParam oneParam, AnotherParam anotherParam)
{
var someResult = AMethodThatUsesOneParam(oneParam.SomeProperty);
// ...
// Beep, beep, boop...🤖
}
If we follow the advice to ask "pushy" questions, we might leave and receive comments like "What if oneParam is null?" or "Could oneParam or anotherParam be null?"
The problem with those types of comments is we can't tell if they're genuine questions or actionable items. Is the reviewer asking a clarification question or "pushing" us in a different direction? We can't tell.
Behind those comments, there's a hidden change request. How is the code author supposed to know the reviewer is asking for a change?
While working on a remote team, it happened more than once that I had to reach out to reviewers via email or chat to ask them to clarify their intentions behind those comments. But some reviewers were in different time zones or even on the other side of the world. All interactions took about ~24 hours between my initial message and their response.
It was frustrating and time-consuming. Arrrggg!
When it was my turn to be a code reviewer, I chose a different approach: I stopped asking those questions.
Use Unambiguous and Intentional Comments
Instead of asking "pushy" questions, let's leave actionable and unambiguous comments that distinguish between questions, to-dos, and nice-to-haves.
Let's go back to the previous example and leave an unambiguous comment. Like this one: "Is it possible that oneParam could be null? If that's the case, please let's add the appropriate null checks. Something like if (oneParam == null) throw ..."
With that comment, it's clear we're suggesting a change.
To better show the intention behind our comments, we can use Conventional Comments.
With that convention, we add keywords like "question," "suggestion" or "nitpick" to clarify the purpose of our comments.
I used it for months in one of my client's projects and other reviewers started to use it too.
For example, we can turn our previous "pushy" comment into these two depending on our intention:
- A clarification question: "question: Is it possible that oneParam could be null?"
- A change request: "suggestion (blocking): Let's add the appropriate null checks if oneParam could be null."
Now it's clear we're referring to two different actions.
Voilà! That's why I don't like "pushy" questions in code reviews. Let's always prefer clear and direct comments without forgetting good manners of course. And let's remember we review code from people with different experience levels and even non-native speakers of our language.
After this experience, my rule of thumb for better code reviews is to write unambiguous comments and always include a suggestion with each comment.
Whether you're starting out or already on the software engineering journey, join my free 7-day email course to refactor your coding career now—I distill 10+ years of career lessons into 7 short emails.
Happy coding!
Top comments (10)
Good article! I just hope we’re not adding null checks everywhere (hence the bilion dollar mistake: medium.com/@ugochukwuchizaramoku/t...)
:D That was the simplest example I could think of to prove the point...Sure, there are better alternatives these days to null-checks everywhere, at least in C# Thanks for your comment
This seems like great remote communication - always try to anticipate the first couple of questions and provide your context up front.
That said, the only reason I can think of to ask the question but not provide your suggestion is to draw out a power struggle. (Why ask a question with such an obvious suggestion hidden behind it if not to force the person to come back to you and ask questions in return or reassure you that all is well?) Is the point of reviewing code to get many eyes on the code to help the PR creator make sure they've thought of everything or is it to make them please you? (That's rhetorical; we know the answer is the former.) It seems less like a "clarify whether you have a question or you have a suggestion" issue and more like a "create fewer rounds of back and forth" issue. Only leaving a question always forces a longer PR turnaround time.
100% agree
Great share @canro91! Very helpful best practice for code reviews. I agree how it is important to be clear if a comment is about a question or an actual suggestion. As another commenter mentioned- it can reduce unnecessary back and forth.
Excellent article! 😊
Lesson learned after waiting for ~24h of back and forth with reviewers and reviewees in the other side of the world
Good point. By making the (conditional) change request explicit, we reduce the review ping-pong. We should always think about how to communicate to make the code reviews as quick as possible.
Exactly! Especially when working on international teams with people all over the world with different seniority levels and language proficiency
100%. Thank you for explaining this! Communication is key..
Sure, for software projects and I've heard the same thing for marriages and relationships too :D