How to Code Review
Written by Fuxing and Wei Yuan.
Originally published on Cake Group Engineering Blog.
What is a Code Review?
A code review is essentially peer developers assessing source code and scrutinising it for anything that could be wrong. What is ‘wrong’ is up to the reviewer — this can be anything from logic errors, lack of tests, code smells, even something as trivial as spelling errors. Code review can also be a place for knowledge sharing/exchange where peer developers can learn from each other and gain a better understanding of the source code. This helps us to avoid siloing knowledge - a typical scenario in many places that naturally toxifies the engineering culture over time. Ultimately, the goal of a review is to ensure that the code being reviewed is in a ‘production ready’ state, to the best of all reviewers’ knowledge.
Review Guidelines
The following section(s) will try to outline certain guidelines when it comes to reviewing. Keep in mind that every team has distinct styles when it comes to doing reviews. Take these guidelines as a baseline and adapt them to what suits your team best.
Do take ownership of a review approval
By far the most important guideline. If there’s only one thing you take away from reading this, then let this be it.
Take ownership of your review approval
What this means is that when you, as a reviewer, press that approve button, you are stamping your seal of approval on that PR. To the best of your knowledge, that PR looks good to you. However, don’t let the phrase ‘to the best of your knowledge’ be a crutch. The onus is on the reviewer to be aware of what is being reviewed. The reviewer should at the very least be aware of the subject-matter and why the change is needed. This is so that the review is meaningful, with a good amount of thought being put into it. Approve something knowing that if someone asks why you approved a particular review, you can answer why confidently.
With that being said…
Don’t approve for the sake of approving
Don’t approve a PR just because you want things to move along or to make things easy for yourself as a reviewer. Take some time to think about the code that is being written and the approach to a particular problem. An approval should take into account these thoughts, instead of something like “oh, the syntax looks fine to me, that’s an approval!”
Do clarify any doubts you have
This might not need mentioning, but clarify any doubts that you might have about the code you are reviewing. Ask if something seems out of place, or you think is implemented weirdly. The worst thing you could do is make assumptions and end up having to fix problems in production instead of having caught the problem at review. By asking questions, not only are you getting more familiar with the code, but the author is also experiencing having to explain to someone else what they have done. In doing so, more often than not, the developer might identify a scenario they haven’t thought about
Do set aside some time every day for reviews
Remember that you’re not the only person developing in the team! Set aside some time each day to see the PRs opened by other team members and give their PRs a review. Not only will this help to move things along, you’ll also be more aware of what is being pushed into the repository. Developers that are newer to the repository can also take more time of their day to do reviews to strengthen their understanding of the existing code.
Do make meaningful review comments and suggestions
Don’t be the guy that makes such a comment
I wrote the code and this looks wrong
Instead, explain your rationale and give a suggestion if appropriate.
This code seems to be wrong because of _, wouldn’t _ make more sense?
Do make use of the suggestion feature in Github
The suggestion feature in Github is a great way to suggest to the developer to fix spelling or grammar mistakes. They won’t even need to checkout the branch, they can make commit those suggestions from the Github UI itself.
Don’t spend too much time arguing discussing what is the ‘right’ way
Don’t drag a discussion on for too long in the comments section of a PR. If necessary, take the discussion offline, or come to a compromise. This is not to say that people shouldn’t chime in with their opinions, but do not discuss something in the PR thread when it might make more sense to have a more appropriate platform for discussion.
Don’t scope creep
There could be certain issues that you catch when doing a review. That’s great! Open a separate issue or PR for it. Do not bloat a PR beyond its original scope, unless it makes a lot of sense for it to be scoped within that PR.
Don’t approve a PR if something is obviously failing
Probably doesn’t need to be said, but if some stages of the CI are failing, please don’t approve a PR. Even if you’re already ok with the code and approach, request changes and let the developer know that there seems to be failing CI stages. There’s no point in approving something which you’ll most likely need to re-approve once a fix for the failing CI is implemented.
Author Guidelines
The following section(s) will try to outline certain guidelines when it comes opening a PR for code review.
Do describe a PR well
If the PR contains complex changes, make use of the PR description to outline your approach, if there are any caveats, etc. If there’s an open issue which the PR addresses, link it!
Do break up your PRs into smaller and manageable chunks
Authors should try to split up their PRs into smaller PRs wherever possible. This can be by cherry-picking certain commits, or working in an incremental way by opening PRs that other PRs will depend on. A good rule of thumb is that if you do not want to review your own PR, others probably wouldn’t want to review it too.
Do be proactive about getting your PR merged
If a PR needs to get merged urgently, or if there hasn’t been any reviews for a while and the PR is stagnating, do reach out and message your team the link to the PR to get reviews. Long-lived branches are horrible to deal with, so let’s avoid that!