Making the Most of a de Facto Industry Standard
The practice of code review has become so popular that everybody understands it intuitively: One or several people read and discuss all code before it’s merged with the common code base. The purpose of this activity is to improve the quality of code—an extra pair of eyes can catch more flaws.
Although, if the process is so good, why do many people still experience trouble with it? Why do developers still discuss ways to improve code review? Like with anything in the world of software development, no rules are carved in stone. Different people understand the purpose of code review differently—they follow different principles, and, as often happens, they can argue and get too personal.
There are, however, some common recommendations, which I cover in this post, that can turn code review into a pleasant—and productive—experience.
Good Pull Requests
Nowadays, the Gitflow workflow—your base and feature branches, pull or merge requests—is the standard. An effective code review starts with a good pull request, but what makes a pull request “good”?
Commit History
People don’t typically strive for a clean commit history. The fast development pace demanded of most programmers forces them to cut corners and save time on everything. However, saving time on preparing a pull request might easily end up increasing the time required for code review and testing.
So, what does it mean to have a clean commit history? The Single Responsibility Principle became legendary among programmers. In a nutshell, this means that any piece of code should be responsible for only one thing. But one of the not-so-obvious places where it should be applied is the commit. The commit should represent an atomic piece of functionality, though fully working, covered with unit tests, and well-documented. Just imagine that you need to move this commit to another, unrelated, branch: If you can do it with a single cherry-pick, you're good.
This approach has tons of advantages, especially in the long run. But in the context of code review, it helps reviewers understand the logic behind changes, reviewing them commit by commit.
Title and Description
A description is often not needed. But even more often, it is. You should always remember that a reviewer isn’t as familiar with the context of a task as you are, so a couple of sentences explaining exactly what the code does never hurts. This is especially true if the pull request is not small or the code changes are not trivial.
Additionally, it's always useful to provide an explanation in the description regarding your design decisions, any corners cut, and the justifications of any workarounds. You’ll be asked about these anyway (given the code reviewer cares), so you might as well be proactive.
Explanations
Apart from the description, it’s never a bad idea to leave your own comments right in the code, if the reasons for your decisions are not immediately obvious. Again, you’ll save time for both parties if you explain in advance why, for example, you picked such an odd name for a function.
Another proper place for your explanations is the pull request itself. When someone reviews code, they leave comments (all modern tools like GitLab and Bitbucket have this capability). If you know a place in your own code that a reviewer may find odd, don’t wait until you're asked; leave an explanatory comment yourself.
Effective Code Review
Next, let’s step into the other party’s shoes for a moment and see how to review other people’s code effectively and smoothly.
What to Review
It's rarely possible to test code just by reading it, so code review should focus on, well, the code itself. Some people say that it's too late to discuss big design decisions during a review —this should be done in the planning stage. However, lower-level architecture—e.g., how specific classes of a program’s components interact with each other — is a good topic, as well as almost everything else: code smells, applied design patterns, classes and function names, code performance, etc.
On the one hand, it's generally discouraged to discuss code style topics during code review. It's always better to make use of linters and formatters that ensure specific style guidelines are being followed. On the other hand, if the team has a strict code style and doesn't have an automated tool to ensure it, be prepared to answer questions about and remediate style flaws. Otherwise, poorly formatted code will leak into the common code base and stay there forever.
Iterative Approach
Bigger pull requests should be approached iteratively. Always begin with a high-level overview and, only if there are no problems, look at lower-level issues. I always start by looking at the overall design, wait until comments are resolved, then move on to implementation details (the SOLID principles, code performance, etc.), proceed to code style (naming, standard library best practices, etc.), and, finally, formatting and other minor things.
The reason for this is that while the code author resolves higher-level problems, the code underneath tends to change, with lower-level problems likely resolving themselves.
Questions
If you don't understand the code, don't stamp it with your good-enough-for-me approval because this defeats the entire purpose of code review. Ask questions, try to understand the intent of the changes. Only approve code if you think that the pull request fulfills its purpose.
If you don't have time to approach the pull request properly, ask someone else to review it. Don't try to create the illusion that everything is perfect. One of the key points of code review is to grow professionally and learn from each other.
Being Nice
This piece of advice needs no explanation. No one will listen to a cock sparrow, even if they're right. Your 20 years of experience is not proof of your infallibility.
Respect other people’s work. The author of the code you’re reviewing spent time on it and is most likely stressed out over having to present it to others.
This also works the other way around. The reviewer makes an effort to help improve your code, so you can’t ignore questions and comments. Even if you don't agree with a point, explain your reasoning. In tough situations, a third party can help find common ground. In simple matters, the author's opinion can be considered primary (at the very least because they’ve been working within the code’s context for a longer period of time).
Being Responsible
Code review is a sacred responsibility of every programmer, and it should be prioritized over other tasks. The pieces under review are always the closest to production, so they have even a greater priority than your current, perhaps very important (but, sadly, unfinished) task.
Alternatives to Code Review
Some people find it surprising, but code review is not the only way. There actually are teams that don't practice it at all. Some of them are walking on thin ice, of course, but others have found another, more suitable process for their needs.
Pair Programming
One of the alternative approaches is pair programming—a widely underestimated practice that at first sight seems to be a waste of time. But just think about it. By letting a couple of developers work on one task together, you save time on code review and the accompanying routine. You improve code quality and save time on bug fixes. Working on a task this way might actually take less time in the long run.
Ship/Show/Ask
Another option is the Ship/Show/Ask model, which involves separating work into three types. The first one is merged immediately (Ship); the second is merged also immediately but through a pull request (Show); and the last follows a traditional flow with a pull request and code review (Ask).
The programmer makes the decision themselves: Trivial code changes fall into the Ship category; more complicated or influential changes, but ones the code author is sure about, are merged through a pull request to let colleagues know about them via Show; and finally, the Ask category includes the changes that the author wants to discuss or is not so sure about.
Ship/Show/Ask is supposed to save time on reviewing insignificant changes and encourage faster continuous integration.
RefinementCodeReview
And finally, there’s RefinementCodeReview promoted by Martin Fowler, the famous author of Refactoring: Improving the Design of Existing Code. This approach can be thought of as a kind of continuous code review. Instead of opening a pull request, quickly fixing all presumed code flaws, merging the code, and then forgetting about it for good, you fix and improve code any time you see it. For instance, if you start working on a new feature or are simply investigating a code area and notice a flaw, you fix it and immediately merge it into the main branch.
This approach has the obvious advantage of quick and continuous code improvement. However, it also has its dangerous sides. For example, especially with large code bases, too many changes can easily turn them into a mess. But for small teams, this method might work fine.
Conclusion
As you can see, there are other options besides code review. However, the existence of such alternatives doesn’t mean that code review itself is an overestimated or generally bad practice. It only means that the practice of code review is not the only way. So, you don’t have to force yourself through it if you really dislike it.
If you do like code review, following the principles described above will make your experience much smoother. You can also check out Shipbook. It helps you improve the code quality of the software you’re developing, giving you the power to remotely gather, search, and analyze your user logs and exceptions in the cloud, on a per-user & session basis.