The tips in this post are focused on informal code reviews; the kind most folks are used to doing on sites like Github or Gitlab and so forth. Some may also work with formal code reviews. Discretion is required.
Specifications Matter
Djikstra had made some famous quotes about testing and proving the correctness of programs. Among them was an anecdote I’ll paraphrase here regarding a colleague who proposed a change to the code: Djikstra would find an error in the code, the colleague would make the necessary changes and resubmit his proposal to Djikstra.
The whole process was time consuming and so Djikstra put the burden of proving that the change should be merged on the author. That made Djikstra’s job much easier. Instead of inspecting the code line by line he had only to review the proof: are the arguments sound and draw the right conclusions?
In software development teams today there aren’t many folks writing formal proofs of correctness with every change they submit for code review. It would be interesting if they were! However it’s often seen as impractical given the other constraints we all have to deal with.
The point here though is that, where possible, encourage your team to submit some kind of “proof” with their changes. This could be an informal set of unit tests. It could be a well-constructed type that ensures a valid program is correct by construction. Whatever the proof is the job of the reviewer is to make sure that it expresses the specification clearly and implements the required functionality.
It can greatly speed up your code reviews by moving the burden of proving that the change should be merged onto the author.
Focus on the Code
When giving feedback resist the temptation to address or interrogate the author. Avoid asking, “Why did you do this?” Or, “You should change this.” Avoid statements like, “I think…” and “I feel…” In fact, avoid using personal pronouns like “I” and “You.”
Instead, talk about the code itself! For example, if we are reviewing this code:
int num_occurrences(int* nums, int target) {
...
}We should avoid asking the author,
Why did you not qualify nums with const?You’re putting a burden on the author to justify the proposed change and it could be seen as a passive-aggressive way to judge their competence. That’s not the goal of code review!
Instead try,
This function should qualify the nums pointer as const so that...It is quick, simple and direct. It gives an explanation so that if
the author didn’t know about why we use const, they will now! And
they didn’t need to be called out for not knowing. If they had simply
made a mistake of omission, no worries! If they had intentionally
left if out then they can respond with their reasoning assured that
the conversation isn’t about their competence, it’s about the code.
You’re reviewing the code, not the people.
Display Intent
Ensure the author is aware of expectations you have regarding their responses. It is not always obvious which suggestions are required to be implemented and which are optional. Make it clear by establishing a convention for indicating expectations.
Some teams use prefixes to their comments like,
nit: If the terms of this expression are assigned meaningful names
it will be easier to read and understand.The “nit:” part indicates the reviewer’s expectation that implementing their suggestion is at the discretion of the author and not required for merging the code.
Other teams use emoji or labels on the comments. When you display your intentions up front it makes responding to feedback easier.
Build Trust
On a team with low churn that has worked together for a long time code reviews should be quick and simple. The goal is to ensure the code changes meet the criteria for merging. Aim to build trust between yourself and everyone on your team. When trust is high, code reviews should be fast and light.
Hopefully some of these tips will help with building trust on your team! Happy reviewing!