The Secret To Better Code Reviews
Code review is an essential task for software engineers today, and you’re probably doing it wrong. Well, at least some of the time. Although giving and receiving feedback on pull requests (PRs) is an essential part of life as a developer, we sometimes get bothered by the comments we see. Over time I have realized that when feedback frustrates me, it usually comes phrased as a question.
Using GitHub’s API, I pulled down all of the PR comments for a few of our projects to see if the real world matched my intuitions. I noticed that questions were very common in reviews. In general, there were almost one third as many question marks as there were comments total.
After reading through the comments and looking for trends, I became convinced that questions should be a last resort in PR feedback. I’m going to walk through the four main categories of questions that I found and how we can do better, but first, let’s consider what makes feedback good in the first place.
What makes for good feedback
Good PR feedback should hit four key points. It should be:
Clear
Clear feedback is easy to process and understand: there is no guessing at what the commenter liked or disliked about your code.
Actionable
Feedback is even stronger when it is actionable and there is a clear next step to resolve the issues. This may include code snippets or suggestions but doesn’t need to.
Kind
It’s important to be direct, but don’t forget your manners. Be kind and don’t patronize.
Timely
Given that the cost of context switching to old work increases as time goes on, it is best to get feedback resolved quickly and in as few iterations as possible.
Questions consistently fail to meet these goals.
Common question pitfalls
Did you mean to write this bug?
Examples:
- Why are we setting up the label’s constraints for every `prepareForReuse`
- Why only return the child view controller if it’s a UINavigationController? I’d think you’d want to do it always.
- Why set the attributed text and not just the `font` and `textColor`. I believe by setting the attributed text, you lose all ability to automatically resize font.
- Is there no constant for this name?
What went wrong:
This is the most common and frustrating sort of question to see. Based on the context the reviewer has reason to believe this code is not optimal. By asking the author why they wrote non-optimal code they are asking them to reflect on their failures, instead of pointing the way towards success. “Why did I write that bug?” Hmm…now I get to spiral out thinking about my litany of personal inadequacies.
You don’t want to make people feel bad with your feedback, you want them to fix the bug. Feedback like this isn’t kind, it’s patronizing. The more obvious the bug is the more patronizing you come across giving feedback like this.
Furthermore, since you already know the bug and what you want them to do about it this feedback is not as clear or actionable as it needs to be. Point out the bug and offer a fix or explanation if appropriate.
Improved Examples:
- We should not set up view constraints in `prepareForReuse` because it will be called on every re-use and we only need to set them up once.
- I think we always want to return the child view controller here, not just for UINavigationControllers
- By setting the attributed text, you lose all ability to automatically resize font. I think we should stick to using `font` and `textColor` instead.
- Add a constant for this name.
Being unnecessarily polite.
Examples:
- Can you add some documentation why this is being set to `nil`?
- Can you add a comment for future maintainers the need for this property and why the initial value is 5?
- Not a huge deal and i know this was there prior to you digging in but could you make this `elseif os(tvOS)` while you’re in there?
What went wrong:
These are all yes or no questions that would be inappropriate to answer with a no. In everyday conversation this sort of phrasing is commonplace and innocuous, but it should be avoided in PR comments. For one thing, it takes a lot longer to write and to process feedback like this. It’s simpler to just write “add docs” or “use elseif instead of else”. This feedback is clearer, simpler, and — in a good way — more impersonal.
Asking if someone can do something for you makes it sound as if the code changes are a personal favor to you. The whole team is trying to make the best product possible. Framing it otherwise is setting expectations that responding to feedback is a chore or a burden.
Improved Examples:
- Add some documentation on why this is being set to `nil`.
- Add a comment for future maintainers the need for this property and why the initial value is 5.
- I know this was there prior to you digging in, but make this `elseif os(tvOS)` while you’re in there.
I have an opinion, but what if I’m wrong?
Example:
- Why is the param called `isDisplayedWithHeader`? Is there something shorter we could try?
What went wrong:
In this case, the person asking the question suspects the code is wrong. They think the variable name should be shorter but have decided to couch their feedback within a question in order to hedge their bets in case they are wrong. It turns out the code is fine; the identifier matches the best practices of style for the language.
If you affirmatively state your beliefs and are proven wrong, that feels worse than asking a question and getting an answer, but if you have opinions about a change, you should bear the burden of having your opinions contradicted. This may make people who are newer or more junior in a project somewhat less comfortable but remember: being wrong is an essential part of learning new things.
On the opposite end, more senior developers should avoid acting like know it alls. State your opinions clearly, but use sentence starters like: “I think” to avoid making it seem like your opinion is the way, the truth, and the light. Instead of asking if there is something shorter available — of course there are shorter words — suggest something.
Although this case had a clear explanation, often opinions about code are really subjective and not all of your suggestions are going to be taken. Still, it’s better to state your preferences clearly then to make people guess at them.
Improved example:
- “I think `isDisplayedWithHeader` is too long and would prefer something shorter like: `displayWithHeader`”
Questions that are not even questions.
Examples:
- This seems like some weird code?
- I don’t think this is the best way to implement this.
- I’m not sure if I like this code.
None of these are questions (not even the one with the question mark) but I’m lumping them in here because they follow the trap of not being clear or actionable and it was sadly common to see people put question marks on statements.
These non-questions inspire all sorts of real, but confusing, questions. Are you supposed to change the weird code? What’s weird about it? How can I hope to figure out what you want me to do if you aren’t even sure yourself? What’s wrong with this implementation and what way would be better?
Feedback like this is unhelpful. It is not clear what you want to change and you come off as aloof and inscrutable.
Improved:
These comments are hopeless. Come back with clearer feedback.
We can do better
So there you have it: four categories of feedback that are super common and super frustrating. As someone who has been guilty of all of these in the past, the good news is that it is not hard to solve this problem. The next time you find yourself asking a question in a PR comment, consider if it could be a declarative statement instead. I have found that once I reflect on that, it helps me refine my own ideas on the code and I give clearer more actionable feedback.
Postscript for the unpersuaded
There are two common responses people have when I stand on my soapbox and evangelize to them about this topic. First, people suggest that asking questions is a good way of making more junior engineers think through problems and reach conclusions on their own — “It’s the Socratic method”. The communication lag on PR comments is just too long for a Socratic style discussion which involves a series of questions and answers. Move it to chat or a face-to-face conversation, either of which is a better venue to iterate through a set of probing questions.
Also, there is an implication that telling people how to do something prevents them from learning. This is not true. The first step to almost any good lesson is to model how to solve a problem.*
The other suggestion is that people are asking lots of legitimate clarifying questions. If code is not understandable and you have made a good faith effort to understand it, asking a question is appropriate. Just make sure that you are not asking a question when you really ought to be stating an opinion about the code. Clarifying questions should only be needed in rare circumstances.
*Also as a former Philosophy major, it’s my duty to remind you that Socrates made so many people angry with his communication style that he was sentenced to death. Not exactly the role model for healthy communication in an office.
