BT

InfoQ Homepage News Code Reviews in Practice

Code Reviews in Practice

Bookmarks

Code reviews are a great way to find bugs, get input from other team members, and share knowledge and ownership. For maximum benefit, integrate code reviews into your development process to ensure that no code reaches production without being reviewed. Reviews tend to uncover unresolved issues in the development process which you may need to address.

Alex Fernandez, a senior developer at Devo, spoke about code reviews at Codemotion Berlin 2018. InfoQ is covering this conference with Q&A, summaries, and articles.

The great majority of open source projects do review all external contributions, and many also review contributions by team members, said Fernandez. But at most companies nobody reads code before it reaches production, not even the author who was just writing it! If you are not scared by this fact, then I don’t know what will, he argued.

Fernandez stated that no code should ever reach production without at least being reviewed by another set of eyes besides the author’s; ideally you want to have two or three reviewers check everything that goes into production, except for trivial changes.

Benefits that code reviews bring are fewer bugs and increased team spirit, and they also support shared code ownership, knowledge sharing, and mentorship, said Fernandez.

Code reviews keep you honest; you won’t code the same way if you know that someone will review your code, argued Fernandez.

InfoQ interviewed Fernandez about code reviews in practice.

InfoQ: You mentioned in your talk that code review is one of the most unknown practices in software development. Why is that?

Alex Fernandez: Most developers are not aware of the crucial importance of code reviews, the reasons for doing them, the techniques for correctly performing them and the wide range of benefits they bring.

Some people do review some code sporadically; it is much harder to find organizations where reviews are deeply ingrained in the development process and they can therefore achieve their full potential. Many techniques can be found in the literature but are quite hard to find in the field. For instance, developers tend to think about reviews as pyramidal supervision: the technical lead reviews the code of their teams, some senior technical lead reviews theirs, and so on. Some projects work this way, most notably the Linux kernel. And yet reviews by junior developers bring their own benefits:

Code reviews are classless: being the most senior person on the team does not imply that your code does not need review. Even if, in the rare case, code is flawless, the review provides an opportunity for mentorship and collaboration, and, minimally, diversifies the understanding of code in the code base.

Another common misconception is that reviews are only useful for reducing bugs, and there is good hard data to prove it. But they also bring many other benefits as we will see later.

InfoQ: What makes code reviews so important?

Fernandez: Allow me to answer with another question: do you know what your organization is deploying into production? And can you vouch for it?

One of the big strengths of open source projects is precisely the frequency and intensity with which code is reviewed: in the aforementioned Linux kernel changes are reviewed at the design stage, then during early development, and finally by the maintainer of the subsystem and all other interested parties. This process ensures that all changes are fit for distribution.

In fact, the review process can be seen as the cornerstone for all other development practices. For instance, you cannot ensure that all new features have unit or integration tests unless the code is independently examined: some developers other than the original authors must manually verify that every new feature has its own test; likewise with coding standards or legibility. Sometimes automated tools can help; testing practices like TDD can be enforced if we require 100% code coverage in the tests. But in the end there is no substitute for manual inspection; someone must ensure that tests are relevant and that they really verify what they are supposed to test.

No sane news outlet would allow an opinion to reach the public without editorial review; and in fact the process of writing this Q&A itself strikes me as very similar to a code review. In development you may be author one time and editor the next, but the principles are the same.

InfoQ: How can we do code reviews effectively?

Fernandez: The first step is to start doing them! Any way you see fit, just try to review some proposed change. GitHub pull requests or Gitlab merge requests are great tools for commenting on code.

The next step is to do code reviews regularly across all teams. For maximum benefit you want to integrate code reviews into your development process; require approval by independent reviewers before any code is merged into the code base. At one of my previous jobs we instituted a "six eyes" policy inspired by the Apache consensus rules: changes are merged only when they have at least two +1’s and no -1’s. One-liners and patches could go through with one +1 and in urgent cases with no scrutiny at all. It worked great! All of us felt that we were part of the decision-making process and in time there were few obscure parts of the code that we were afraid to touch. Rules can be adapted to require more (or less) approvals while keeping the general idea; anyone can veto any changes, and code requires consensus to reach production.

Doing code walkthroughs is a great practice with a long history; you get the original author and the reviewers together and go through the changes. If the author does not learn anything just from explaining their code, you are doing it wrong!

You should always review early, and review often. I have seen too many projects fail because the authors were working on their own for many months, and they failed to deliver. Code reviews are a great way to get the input from other team members. From the point of view of the reviewer it is important to frame everything as a question, instead of giving orders. For instance, instead of saying "change this to that" you may ask, "why is this necessary?".

Doing good reviews is kind of an art, and as such there are many conflicting viewpoints. Too often pull requests become battlefields or ego minefields, and it takes real discipline to keep them useful. Beware: there are many more known pitfalls, such as choosing the wrong reviewers or sending pull requests too late in the development process. The sooner you start with it the better you will eventually get!

In my experience you have to be patient and let the culture of independent review seep into the organization until it is accepted naturally. You should take into account that reviews tend to uncover unresolved issues in the development process, so any problems found in the process are usually pre-existing conditions. For instance, if egos or hierarchy are a problem in pull requests, it is probably because there were conflicts in place that need to be addressed.

InfoQ: What benefits do code reviews bring?

Fernandez: First let me mention the holy grail of computing: less bugs. Now that this is out of the way, benefits spread much further:

  • Shared code ownership: team members learn how everything works, even if they did not write it. The bus factor is thus increased (and risk reduced).
  • Knowledge sharing: a direct consequence of the above, more people get to know the code which can only be good.
  • Shared blame: when a bug is deployed to production there is no longer a single culprit, since other people signed it off. Less blame means more confidence to do risky stuff.
  • Team spirit: helping one another makes the team feel they are achieving something together, because they are! In time you will learn to ask for help when needed, not just at the time of a pull request.
  • Mentorship: it is very easy to mentor juniors by reviewing their code, but also by making them review someone else’s code.
  • Maintaining high standards: when you know your code is going to be reviewed you are less likely to take shortcuts.

If these reasons are not good enough for you, think about the peace of mind that comes with knowing that one bad moment will not ruin your product, or your team.

Rate this Article

Adoption
Style

Hello stranger!

You need to Register an InfoQ account or or login to post comments. But there's so much more behind being registered.

Get the most out of the InfoQ experience.

Allowed html: a,b,br,blockquote,i,li,pre,u,ul,p

Community comments

Allowed html: a,b,br,blockquote,i,li,pre,u,ul,p

Allowed html: a,b,br,blockquote,i,li,pre,u,ul,p

BT

Is your profile up-to-date? Please take a moment to review and update.

Note: If updating/changing your email, a validation request will be sent

Company name:
Company role:
Company size:
Country/Zone:
State/Province/Region:
You will be sent an email to validate the new email address. This pop-up will close itself in a few moments.