Code reviews, the last resort after automated checks, and the best mechanism for learning for both parties: the person whose code is being reviewed and the reviewers. This post reveals the best practices for code reviews in the context of professional commercial software development. The practices listed here are based highly on my personal experiences, and thus may not be applicable to all software projects.
Treat code reviews as first-class citizens. Every change going to the main branch should go through a separate branch, and most importantly, it should be reviewed before merging. Take your time doing the review, it’s significantly cheaper to spot potential weak points at review compared to letting your end users spot them. If the changeset is not trivial, do a checkout for the branch you're reviewing and play with it to gain more insight.
Automate everything you can. People's time is expensive whereas running linters and automatic code formatting is cheap. It's expensive (and pointless) to spend time on arguing about code formatting in code reviews. Have one and only one enforced way (with CI checks) to do things. The more contributors, the more of the best practices should be enforced in order to ensure consistency across codebase. Ideally, there should not be any formatting related diffs in PRs.
Having a ton of best practices enforced should not be a blocker for contributors though. Thus, consider having a single tool which is able to run all enforced checks/tooling effortlessly also during local development. If you don't have one yet, have a look at pre-commit.
Require tests for all new functionality. Have some quality gate for test coverage. A simple approach is to require full coverage or to not allow lowering coverage. Have code coverage reports easily available.
If you are in a team developing Python, consider embedding at least a combination of black/yapf, flake8/pylint, isort, and mypy into your workflow and projects.
Don't rely on single reviewer. The more pairs of eyes, the more likely it's that potential weaknesses are spotted in the review stage. On the other hand, code reviews are not only for verifying that the changeset is legit. Reading code from others is one of the best ways to learn and stay up-to-date about what’s happening in the code base.
Considering the learning aspect, it might be beneficial if junior developers or new comers spend more time on reviewing than writing code themselves at the beginning of employment. Unfortunately I haven't witnessed such approach in working life but I have a strong feeling that it'd have a positive impact on the onboarding phase.
Keep the big picture in mind while reviewing. Often people (especially less-experienced reviewers) tend to focus only on the correctness of the changeset while reviewing. Keep the architecture and maintainability in mind as well. As a reviewer, ask yourself whether you’d be confident to touch the same code in six months from now before giving green light.
Be polite. This should be obvious. We are adults. Respect others.
Keep changesets small. It’s easy to lose focus at some point if the changeset you’re reviewing is overwhelming. In practice this means that people are more reluctant to even start a review if the diff can not be skimmed through in a minute or two. Additionally, semi hidden weak spots go more likely unnoticed in huge diffs. On the other hand, some types of changes require a big changeset. In such cases, consider splitting your review to multiple "sub-reviews" to fight against loosing focus. Drink coffee between sub-reviews.
Review ASAP. This aspect is rarely highlighted in talks or blog posts about the topic. I argue that considering the throughput of a team (or whole company) it’s crucial to review as soon as possible. The sooner it’s merged, the better (relates to keeping changesets small). Especially in situations in which the author of the changeset is likely to continue building something on top of the current changeset.
In general, try to prioritize code reviews over your own code. On the other hand, switching context from your own code to review is not easy. For some people, it’s a complete productivity turnoff. If this is the case, it’s ok, don’t force yourself. Review when you are comfortable switching the context.
In case there are multiple reviewers, watch out for overlapping work. Overlap can be easily avoided by assigning code review to yourself when you start reviewing (in case someone hasn’t already assigned it to themselves). Remember to remove assignment as soon as you’re done.
Enjoyable code reviews!
