The Standard of Code Review at Sixfold — Effective Communication of Feedback

10 min readSep 1, 2021

Note: This is a modified version of an internal document we’ve had in place at Sixfold since April 2020.

In our previous post on code reviews, we discussed aspects of code that help ensure its health. In this post, we’ll go further into the process of code review, focusing on the communication aspects and how to keep things effective.

Terms & Background

Pull Request (PR) — Method for submitting changes to a project. In our context, method for submitting changes to our production environment.
Author — Person(s) that are driving the PR, aiming to make changes to the production environment.
Reviewer — Person(s) that are conducting the code review of the PR.

Guidelines for communicating in code reviews

Equally important to focusing on the correct aspects of the changes suggested in a PR, is how we communicate our questions or feedback about them. The key properties here are professionalism and kindness.

The following four suggestions are outlined in Google’s engineering practices for the reviewer:

Be kind
Explain your reasoning
Balance giving directions with just pointing out problems and letting the author decide
Encourage the author to simplify code or add comments instead of just explaining the complexity to the reviewer

In addition to these suggestions, we also define our expectations for the two roles of the code review:

Come prepared as the author
Be present as the reviewer
Be committed to seeing the code review to a timely finish

Be kind

Code review should never involve making comments about personalities, it should always revolve around the code at hand. Both the reviewer and the author should remain professional and level-headed at all times.

One of the most practical suggestions to achieve this is to avoid questions that start with “Why did you…” or comments that start with “I don’t…”. This often leads to more neutral wording, as well as focusing more on a constructive suggestion, rather than just overwhelming the other party with questions/negativity.

Some examples of how wording can make feedback sound kinder and more neutral towards the people involved.

There are many more examples of different wording and the effects this has, on Thoughtbot’s Code Review guide. Here are some that are most applicable to our workflow:

Ask good questions, don’t make demands
Good questions avoid judgments or assumptions about the author’s perspective
Avoid selective ownership of code (eg. “Your code has a bug here” type of comment)
Avoid terms that could be construed as referring to personal traits — code can not be “dumb” or “stupid”
Don’t use hyperbole (eg. “This never happens” or “There is no point”)
Don’t use sarcasm, it does not communicate well over text

Most importantly, don’t forget that there is a person on the receiving end of your comments. Keep the communications light and friendly, encourage them with positive feedback as well (never hesitate to point out good things about the code changes).

Explain your reasoning

In addition to the author justifying the changes they are proposing, the reviewer is also expected to justify any criticism or requests they have about the PR.

At no point during the code review, should the author find themselves not understanding why the reviewer is commenting on a piece of code.

Code should be clear and lack complexity, but it’s equally important that the feedback on the code is clear and includes reasoning. This can usually take the form of a question that is accompanied by a scenario, which raised the question. Or a proposal for changing something with a reason why said change should be preferred.

It is important to realise that reasoning should be based on facts, not hearsay or gut-feeling. It can be based on prior experiences either party has had, but even then, it should still rely on facts and not on memories.

It is sometimes the case that the reviewer wants to write a comment, that they feel should be stated, however, they do not expect there to be a followup by the author. Such a comment could be about a small optimisation, or changing the code-style to be more elegant. It is expected that in such situations the reviewer makes this intent clear, for example by prefixing the comment with “Nitpick” or “:neckbeard:”.

Once a comment is posted, it is expected of the author to address it with equal care, not dismissing comments as not worthy of time or as something that is out of the discussion. This also applies to the comments discussed in the previous paragraph, for which it is still encouraged that the author offer at least some response to, for example by using Github’s reactions.

Examples, where A is author and R is reviewer:

Now imagine the same exchange, if the reviewer did not justify their comment:

Even if the author fundamentally understands the benefits, the comment does not allow to apply that knowledge without making assumptions about what was meant by the reviewer. And as is often the case, assumptions can be incorrect when it comes to communications between people.

Similarly, imagine if the author does not provide justification for their choice:

Again, by not involving concrete facts, the author makes it difficult for the reviewer to apply knowledge about the context without making assumptions about the decisions the author took.

This example also illustrates, why it is never a good idea to state a concrete decision without proof, it leaves the other party in the dark.

In case the reviewer rejects the PR due to blocker(s), it should be made clear what the main blocking reasons are, giving the author a concrete set of action items to followup with.

Without this, the author is encouraged to ask what the main reasons were, or if the number of comments is sufficiently low (less than five), treat them all as equally blocking.

Balance between directions and guidance

In order for the code review process to also satisfy its secondary goal, which is to help grow the experience of our engineers, it is important to strike a balance between suggesting concrete changes vs raising questions and letting the author decide for themselves.

This can often be caused by the reviewer feeling like it is faster to just fix the thing themselves than it is to communicate it to the author. While understandable in general, we should prefer giving guidance over giving concrete advice to be applied mechanically by the author.

In more practical terms, given our chosen tool for code reviews is Github, it seems like it is reasonable to give concrete directions via the multi-line code suggestion tool.

This has the benefit of often minimising the scope of the suggested change, whilst providing the reviewer with the ability to communicate their suggestion faster to the author than they could by just using words.

It also has the benefit of leaving it up to the author to decide how and if they apply the feedback, allowing for a discussion before doing so.

Similarly, we have to be careful about just guiding and not giving concrete directions at all. This can be the case, when the reviewer has spotted an edge-case or a scenario in which the functionality of the code is not what the author expects. In such situations, the reviewer should have the means of explaining what parts of the code need to be changed to either eliminate the flaw, or what sort of test to write to make the situation appear.

Simplify or refactor the code over just explaining it in code review

If a piece of code is complex and is then rightly pointed out by the reviewer as being exactly that, the outcome should not be an explanation of the complexity in the code review process, but rather this should be taken as a hint by the author to refactor the code.

It is expected that reviewer reminds the author of this as well as the author to understand that comments and discussion on PRs will never replace code that is understandable by just looking at it.

Instead of just comments on the PR, it is expected of the author and reviewer to work together to make the complexity either:

Not be there at all (preferred)
Be documented in the code itself

The former option is the preferred approach, as the latter often takes the form of comments in code that ultimately have the risk of getting outdated.

Coming prepared as the author

The code review process starts from a PR being opened, thus it is expected of the author to come well-prepared. This should include several things:

Validating whether the scope of the PR is as minimal as it viably could be
Referencing the reasoning for the change and giving as much context as possible for anyone doing the review
Choosing an appropriate number/set of reviewers
Framing the expectations for the reviewer, both in terms of any deadlines, as well as what they would want most feedback on

Perhaps one of the most practical suggestions to the author is to keep the PR as small as possible. This can often be accomplished by separating several logical steps into separate PRs, each of which are easier and less time consuming to review. For example, by first submitting a PR that refactors some existing code in preparation for new functionality being added in a followup PR.

In addition to this, the author should take responsibility in making sure that the reviewer is aware of their request for review. This can be done in several ways, however, our experience shows that just opening a PR is not enough and at least a mention of it via some other communication channel is expected. A practical method is to use our existing communication tools, such as Slack, pinging the reviewer about the pending review.

Being present as the reviewer

While it is tempting to offer suggestions in a “fly-by” fashion (“I heard you were discussing this topic, perhaps you could…”), it is expected of the reviewer to not get involved in a code review unless they are willing to work with it until some final result — usually the point when PR gets merged.

This is most important when it comes to more abstract suggestions (for example about algorithms or changes in programming patterns), which can distract the author and cause confusion in the code review. If reviewer feels the need to make such suggestions, they are expected to help the author understand these, by committing more of their time to the code review, than just a few minutes to make the initial comment.

It is not expected for the reviewer to always be available at the request of the author, but in such case, it is expected of the reviewer to suggest a replacement for themselves. There should not exist a situation where an author is left looking for reviewers as seemingly no-one is able or willing to review the code.

Another practical method for being present as the reviewer is to mark comments/questions that have been addressed as resolved, signalling to the author that the reviewer accepts the changes and is willing to move forward.

Committing to finishing the code review

Both the author as well as the reviewer are expected to keep an active interest in the PR and the review.

It is not acceptable to leave a PR dangling for long periods of time — if no agreement can be reached as to whether the change is acceptable or not, then the change needs to either be withdrawn or worked upon.

Practical suggestions for reaching an agreement on the change:

As a reviewer, don’t reject changes you can live with just because they are not yet perfect. Remember, the goal is to make iterative progress towards healthier code.
As an author, evaluate whether it is worth postponing refactoring if the argument around it is costing a lot of time. Remember, if a part of code is raising a lot of discussions, it will likely also waste a lot of time in the future in similar discussions.
As an author, feel free to use alternative methods of communication, if the code review seems to drag on for extended periods of time, suggest a meeting with the reviewer to discuss the changes in person/over a call.

It is up to both parties to make sure that their comments and actions have a followup, however, it is ultimately in the interest of the author to reach a conclusion, so majority of the responsibility of finishing the code review lies with them.

It is encouraged the parties remind each other when a discussion has gone silent for extended periods of time (days). It is seen as a positive, when either of the parties reaches out to the other to restart the discussion — even if the conclusion is that they are unable to finish the code review, it should still lead to an action plan, for example to finding an alternative reviewer/author or to withdrawing the PR for the time being.

There are several practical suggestions for re-starting a discussion, for example if the author feels like they have addressed all open comments, they can mark the comments as resolved and re-request a review from the reviewer (once again, making sure the reviewer has received this information by reaching out via a second mean)

Similarly, it is understandable if due to some reason the author needs to make changes to the code after the reviewer has already accepted the changes. In such a situation, it is encouraged that the author evaluate whether the change was significant enough to warrant a dismissal/re-request of the review. And as with any other action, it is expected of the author to communicate their decision to the reviewer in case they do want another review.

Summary

In the two posts we’ve discussed our standard for code reviews, we’ve gone over the various aspects that help us keep, and improve, the health of the code, as well as making sure these aspects are enforced via positive and efficient communications.

We’ve seen these principles work well, mostly due to them being generic enough and not just a “process” that is applied. We’ve noticed that some of these principles have also carried over to our other communications, such as our grooming or preparation processes for features.

For machines the structure of the code does not really make much of a difference (at least in terms of understanding), for humans, the impact can be profound. We believe communicating about code is equally as important as the code itself. At the end of the day, it is humans that write the code, and it is also humans that have to read the code before applying new changes to it.