Code-Review Best Practices
Table of Contents
Code review is a crucial practice in software development. One can design and write great software, but we are humans after all. And all humans make mistakes, so another pair of eyes is always helpful.
The review process might seem straightforward, but there are useful tips to make it less painful is some cases.
The senior principle #
In general, reviewers should favor approving a CL once it is in a state where it definitely improves the overall code health of the system being worked on, even if the CL isn’t perfect.
That is the senior principle among all the code review guidelines.
There are limitations to this, of course. For example, if a change adds a feature that the reviewer doesn’t want in their system, then the reviewer can certainly deny approval even if the code is well-designed.
A key point here is that there is no such thing as “perfect” code – there is only better code. Reviewers should not require the author to polish every tiny piece before granting approval. Rather, the reviewer should balance out the need to make forward progress compared to the importance of the changes they are suggesting. Instead of seeking perfection, what a reviewer should seek is continuous improvement. A change that, as a whole, which improves the maintainability, readability, and understandability of the system shouldn’t be delayed for days or weeks because it isn’t “perfect.”
Reviewers should always feel free to leave comments expressing that something could be better, but if it’s not very important, prefix it with something like “Nit: “ to let the author know that it’s just a point of polish that they could choose to ignore.
Going baby-steps #
Multiple small incremental changes is better than one big change. There are a number of benefits of making many small incremental pull requests instead of few big ones:
- Reviewed more quickly. It’s easier for a reviewer to find five minutes several times to review small pull requests than to set aside a 30-minutes block to review one large pull request.
- Reviewed more thoroughly. With large changes, reviewers and authors tend to get frustrated by large volumes of detailed commentary shifting back and forth—sometimes to the point where important points get missed or dropped.
- Less likely to introduce bugs. Since you’re making fewer changes, it’s easier for you and your reviewer to reason effectively about the impact of the PR and see if a bug has been introduced.
- Less wasted work if they are rejected. If you write a huge PR and then your reviewer says that the overall direction is wrong, you’ve wasted a lot of work.
- Easier to merge. Working on a large PR takes a long time, so you will have lots of conflicts when you merge, and you will have to merge frequently.
- Easier to design well. It’s a lot easier to polish the design and code health of a small change than it is to refine all the details of a large change.
- Less blocking on reviews. Sending self-contained portions of your overall change allows you to continue coding while you wait for your current PR in review.
- Simpler to roll back. A large PR will more likely touch files that get updated between the initial PR submission and a rollback PR, complicating the rollback (the intermediate PRs will probably need to be rolled back too).
As an example, one of my colleagues had to spend a lot of time every day rebasing her working branch from “master” to keep it up to date with recent changes. As a result, the task could not be completed for a couple of weeks.
What you can do if your pull request became huge? There are multiple strategies:
- Create multiple another PRs, cherry-picking or manually including only limited number of changes.
- Close existing PR, and create new smaller PR
- Grab your peer and make a code-review in pair (similar to pair-programming). Explain your changes to your colleague(s), but give them some time to “digest” it. Pair code-review is very useful when there are a lot of simple changes, like renaming or code reorganization, when it’s easier to explain the idea instead of making other person to wade through the changes. Another usecase is complex algorithmic changes, where explanation is necessary.
The first two options are more common, because even with a peer review it is hard to handle a large piece of information.
Replace code-review with pair programming #
The reason of reviewing code is to get a second opinion and find possible issues with a new code. But also a learning. And all this can be achieved when practicing pair programming, as people can share ideas and immediately give and receive feedback. The feedback loop is very fast. And PR-review is not needed, since code was reviewed on writing.
Complete code review within one business day #
If you are not in the middle of a focused task, you should do a code review shortly after it comes in. One business day is the maximum time it should take to respond to a code review request (i.e., first thing the next morning). Following these guidelines means that a typical PR should get multiple rounds of review (if needed) within a single day. When code reviews are slow, several things happen:
- The velocity of the team as a whole is decreased. Yes, the individual who doesn’t respond quickly to the review gets other work done. However, new features and bug fixes for the rest of the team are delayed by days, weeks, or months as each PR waits for review and re-review.
- Developers start to protest the code review process. If a reviewer only responds every few days, but requests major changes to the PR each time, that can be frustrating and difficult for developers. Often, this is expressed as complaints about how “strict” the reviewer is being. If the reviewer requests the same substantial changes (changes which really do improve code health), but responds quickly every time the developer makes an update, the complaints tend to disappear. Most complaints about the code review process are actually resolved by making the process faster.
- Code health can be impacted. When reviews are slow, there is increased pressure to allow developers to submit PRs that are not as good as they could be. Slow reviews also discourage code cleanups, refactorings, and further improvements to existing PRs.
Keep repository clean #
Automatically delete stale branches after pull request merging.
Having stale branches in a repo is confusing. It is better to create a short-living branch for next
Squash commits to keep git history clean #
As you work on a feature branch, you often create small, self-contained commits. These small commits help describe the process of building a feature, but can clutter your Git history after the feature is finished. As you finish features, you can combine these commits and ensure a cleaner merge history in your Git repository by using the squash and merge strategy.
- Small commits are joined together, making it simpler to revert all parts of a change.
- When the single commit merges into the target branch, it retains the full commit history.
- Your base branch remains clean, and contains meaningful commit messages.
Commit author should merge his PR and squash commits in order to preserve commit author attribute in Git history.
If there are many unrelated commits in a pull request, then this pool request should be rejected and replaced with a series of small PRs (see “Going baby-steps”).