DevOps Zone is brought to you in partnership with:

I am an experienced software development manager, project manager and CTO focused on hard problems in software development and maintenance, software quality and security. For the last 15 years I have been managing teams building electronic trading platforms for stock exchanges and investment banks around the world. My special interest is how small teams can be most effective at building real software: high-quality, secure systems at the extreme limits of reliability, performance, and adaptability. Jim is a DZone MVB and is not an employee of DZone and has posted 100 posts at DZone. You can read more from them at their website. View Full User Profile

Don't Waste Time on Code Reviews

08.22.2014
| 47094 views |
  • submit to reddit

Less than half of development teams do code reviews and the other half are probably not getting as much out of code reviews as they should.

Here’s how to not waste time on code reviews.

Keep it Simple

Many people still think of code reviews as expensive formal code inspection meetings, with lots of prep work required before a room full of reviewers can slowly walk through the code together around a table with the help of a moderator and a secretary. Lots of hassles and delays and paperwork.

But you don’t have to do code reviews this way – and you shouldn’t.

There are several recent studies which prove that setting up and holding formal code review meetings add to development delays and costs without adding value. While it can take weeks to schedule a code review meeting, only 4% of defects are found in the meeting itself – the rest are all found by reviewers looking through code on their own.

At shops like Microsoft and Google, developers don’t attend formal code review meetings. Instead, they take advantage of collaborative code review platforms like Gerrit, CodeFlow, Collaborator, or ReviewBoard or Crucible, or use e-mail to request reviews asynchronously and to exchange information with reviewers.

These light weight reviews (done properly) are just as effective at finding problems in code as inspections, but much less expensive and much easier to schedule and manage. Which means they are done more often.

And these reviews fit much better with iterative, incremental development, providing developers with faster feedback (within a few hours or at most a couple of days, instead of weeks for formal inspections).

Keep the number of reviewers small

Some people believe that if two heads are better than one, then three heads are even better, and four heads even more better and so on…

So why not invite everyone on the team into a code review?

Answer: because it is a tragic waste of time and money.

As with any practice, you will quickly reach a point of diminishing returns as you try to get more people to look at the same code.

On average, one reviewer will find roughly half of the defects on their own. In fact, in a study at Cisco, developers who double-checked their own work found half of the defects without the help of a reviewer at all!

A second reviewer will find ½ as many new problems as the first reviewer. Beyond this point, you are wasting time and money. One study showed no difference in the number of problems found by teams of 3, 4 or 5 individuals, while another showed that 2 reviewers actually did a better job than 4.

This is partly because of overlap and redundancy – more reviewers means more people looking for and finding the same problems (and more people coming up with false positive findings that the author has to sift through). And as Geoff Crain at Atlassian explains, there is a “social loafing” problem: complacency and a false sense of security set in as you add more reviewers. Because each reviewer knows that somebody else is looking at the same code, they are under less pressure to find problems.

This is why at shops like Google and Microsoft where reviews are done successfully, the median number of reviewers is 2 (although there are times when an author may ask for more input, especially when the reviewers don’t agree with each other).

But what’s even more important than getting the right number of reviewers is getting the right people to review your code.

Code Reviews shouldn’t be done by n00bs – but they should be done for n00bs

By reviewing other people’s code a developer will get exposed to more of the code base, and learn some new ideas and tricks. But you can’t rely on new team members to learn how the system works or to really understand the coding conventions and architecture just by reviewing other developers’ code. Asking a new team member to review other people’s code is a lousy way to train people, and a lousy way to do code reviews.

Research backs up what should be obvious: the effectiveness of code reviews depend heavily on the reviewer’s skill and familiarity with the problem domain and with the code. Like other areas in software development, the differences in revew effectiveness can be huge, as much as 10x between best and worst performers. A study on code reviews at Microsoft found that reviewers from outside of the team or who were new to the team and didn’t know the code or the problem area could only do a superficial job of finding formatting issues or simple logic bugs.

This means that your best developers, team leads and technical architects will spend a lot of time reviewing code – and they should. You need reviewers who are good at reading code and good at debugging, and who know the language, framework and problem area well. They will do a much better job of finding problems, and can provide much more valuable feedback, including suggestions on how to solve the problem in a simpler or more efficient way, or how to make better use of the language and frameworks. And they can do all of this much faster.

If you want new developers to learn about the code and coding conventions and architecture, it will be much more effective to pair new developers up with an experienced team member in pair programming or pair debugging.

If you want new, inexperienced developers to do reviews (or if you have no choice), lower your expectations. Get them to review straightforward code changes (which don’t require in depth reviews), or recognize that you will need to depend a lot more on static analysis tools and another reviewer to find real problems.

Substance over Style

Reviewing code against coding standards is a sad way for a developer to spend their valuable time. Fight the religious style wars early, get everyone to use the same coding style templates in their IDEs and use a tool like Checkstyle to ensure that code is formatted consistently. Free up reviewers to focus on the things that matter: helping developers write better code, code that works correctly and that is easy to maintain.

“I’ve seen quite a few code reviews where someone commented on formatting while missing the fact that there were security issues or data model issues.”
Senior developer at Microsoft, from a study on code review practices

Correctness – make sure that the code works, look for bugs that might be hard to find in testing:

  • Functional correctness: does the code do what it is supposed to do – the reviewer needs to know the problem area, requirements and usually something about this part of the code to be effective at finding functional correctness issues
  • Coding errors: low-level coding mistakes like using <= instead of <, off-by-one errors, using the wrong variable (like mixing up lessee and lessor), copy and paste errors, leaving debugging code in by accident
  • Design mistakes: errors of omission, incorrect assumptions, messing up architectural and design patterns like MVC, abuse of trust
  • Safety and defensiveness: data validation, threading and concurrency (time of check/time of use mistakes, deadlocks and race conditions), error handling and exception handling and other corner cases
  • Malicious code: back doors or trap doors, time bombs or logic bombs
  • Security: properly enforcing security and privacy controls (authentication, access control, auditing, encryption)

Maintainability:

  • Clarity: class and method and variable naming, comments, …
  • Consistency: using common routines or language/library features instead of rolling your own, following established conventions and patterns
  • Organization: poor structure, duplicate or unused/dead code
  • Approach: areas where the reviewer can see a simpler or cleaner or more efficient implementation

Where should reviewers spend most of their time?

Research shows that reviewers find far more maintainability issues than defects (a ratio of 75:25) and spend more time on code clarity and understandability problems than correctness issues. There are a few reasons for this.

Finding bugs in code is hard. Finding bugs in someone else’s code is even harder.

In many cases, reviewers don’t know enough to find material bugs or offer meaningful insight on how to solve problems. Or they don’t have time to do a good job. So they cherry pick easy code clarity issues like poor naming or formatting inconsistencies.

But even experienced and serious reviewers can get caught up in what at first seem to be minor issues about naming or formatting, because they need to understand the code before they can find bugs, and code that is unnecessarily hard to read gets in the way and distracts them from more important issues.

This is why programmers at Microsoft will sometimes ask for 2 different reviews: a superficial “code cleanup” review from one reviewer that looks at standards and code clarity and editing issues, followed by a more in depth review to check correctness after the code has been tidied up.

Use static analysis to make reviews more efficient

Take advantage of static analysis tools upfront to make reviews more efficient. There’s no excuse not to at least use free tools like Findbugs and PMD for Java to catch common coding bugs and inconsistencies, and sloppy or messy code and dead code before submitting the code to someone else for review.

This frees the reviewer up from having to look for micro-problems and bad practices, so they can look for higher-level mistakes instead. But remember that static analysis is only a tool to help with code reviews – not a substitute. Static analysis tools can’t find functional correctness problems or design inconsistencies or errors of omission, or help you to find a better or simpler way to solve a problem.

Where’s the risk?

We try to review all code changes. But you can get most of the benefits of code reviews by following the 80:20 rule: focus reviews on high risk code, and high risk changes.

High risk code:

  • Network-facing APIs
  • Plumbing (framework code, security libraries….)
  • Critical business logic and workflows
  • Command and control and root admin functions
  • Safety-critical or performance-critical (especially real-time) sections
  • Code that handles private or sensitive data
  • Old code, code that is complex, code that has been worked on by a lot of different people, code that has had a lot of bugs in the past – error prone code
High risk changes:
  • Code written by a developer who has just joined the team
  • Big changes
  • Large-scale refactoring (redesign disguised as refactoring)

Get the most out of code reviews

Code reviews add to the cost of development, and if you don’t do them right they can destroy productivity and alienate the team. But they are also an important way to find bugs and for developers to help each other to write better code. So do them right.

Don’t waste time on meetings and moderators and paper work. Do reviews early and often. Keep the feedback loops as tight as possible.

Ask everyone to take reviews seriously – developers and reviewers. No rubber stamping, or letting each other off of the hook.

Make reviews simple, but not sloppy. Ask the reviewers to focus on what really matters: correctness issues, and things that make the code harder to understand and harder to maintain. Don’t waste time arguing about formatting or style.

Make sure that you always review high risk code and high risk changes.

Get the best people available to do the job – when it comes to reviewers, quality is much more important than quantity. Remember that code reviews are only one part of a quality program. Instead of asking more people to review code, you will get more value by putting time into design reviews or writing better testing tools or better tests. A code review is a terrible thing to waste.

Published at DZone with permission of Jim Bird, author and DZone MVB. (source)

(Note: Opinions expressed in this article and its replies are the opinions of their respective authors and not those of DZone, Inc.)

Comments

Richard Langlois replied on Wed, 2014/08/27 - 7:21am

Fully agree with this article.

In my department at Nokia, we use PMD static analysis tool during our development process, so the code is already cleaned up of any static faults. So the reviewer (One developer is assigned to review the code change) can focus on higher level of the review. And this model works very well for us.

 

Richard Langlois

Principal Software Engineer

Nokia, Burlington, MA, USA.

Yaqoob Johnson replied on Wed, 2014/08/27 - 11:10am

Thanks for an excellent article, I recognise quite a few of my faults!

Previously I used PMD and FindBugs, now we've moved to SonarQube on a new colleague's advice. It rolls FindBugs, PMD and much more into one pretty comprehensive static analysis with plenty of scope for configuration and extension.


Yaqoob Johnson

Developer

Dario Macchi replied on Wed, 2014/08/27 - 11:37am

Fully agree with this article. I've done my MsC thesis about reviews and there are many review methods (with different formality levels) that are misused in industry. I'm also agree with the use of specific tools to help in the code review (and avoid the time wasted on style review).

What do you think about reviewing other software development artifacts?

Robin Varghese replied on Wed, 2014/08/27 - 12:48pm

 Really nice post. I remember having attend one of the above mentioned "group code reviews" and remember it as big waste of time. Single code reviews conducted by a senior person makes perfect sense. And I think IDEs like Eclipse provide an awesome set of tools to manage code formatting, bad code habits etc. Where Eclipse falls short there are really good plugins available too.

From personal experience - I have found that writing Junits is a good way to catch bugs in our code - but of course that step is before code review.

Robin Varghese replied on Wed, 2014/08/27 - 12:48pm

 Really nice post. I remember having attend one of the above mentioned "group code reviews" and it was a big waste of time.

Single code reviews conducted by a senior member makes perfect sense. And I think IDEs like Eclipse provide an awesome set of tools to manage code formatting, bad code habits etc. Where Eclipse falls short there are really good plugins available too.

From personal experience - I have found that writing Junits is a good way to catch bugs in our code - but of course that step is before code review.

Theodore Ravind... replied on Wed, 2014/08/27 - 3:41pm

Excellent article. Thanks for sharing. Agree with you completely! 

Randall Goya replied on Wed, 2014/08/27 - 7:28pm

My current team does not hold "code review" sessions. I usually end up spotting issues in my team's code, like failure to follow "best practices," by working on the code to debug or extend it after someone else has already written the basis.

A Digression About Coding Practices in Drupal

Much of my work is with the Drupal Content Management Framework, and over 10 years Drupal has developed some coding practices that are sui generis and not necessarily what someone who has worked on other kinds of PHP applications would know about.

Many developers are now familiar with applications that follow a MVC (Model-View-Controller) architecture. Any Node.js/Backbone application is based on MVC. Evolved from a CMS (Content Management System), Drupal is not a MVC framework as it uses more efficient methods to manage content in its database. Since its primary function is to display content, Drupal follows a PAC (Presentation-Abstraction-Control) model. A Drupal node  is the basic container for a piece of content, and the Drupal framework can perform some elegant operations on nodes.

Therein lies the danger for developers who are unfamiliar with Drupal, and who may write code that is not using Drupal-specific objects and functions. Drupal is moving towards a more OO architecture with Entities in Drupal 7 , and the Drupal core project is revising much of its fundamental structure to use Symfony2 in the upcoming Drupal 8. I have designed OO data structures in Drupal 7 with Entities for data, when Drupal is the application engine that consumes this data and then does something different with it than Drupal's primary function of displaying content on web pages.

Back to Code Review -- for Drupal

Drupal has a contributed module called Coder  - and it can run an automatic scan on your code to detect variances from "best practices" for Drupal conventions. 

As an end-to-end framework, Drupal provides its own objects, functions and "hooks" that may not be known to a PHP developer who is not familiar with Drupal. Often an inexperienced Drupal developer will write some PHP code that "works" but does not leverage the resources available in Drupal. Often the consequences are reduced performance, or code that is difficult to maintain and upgrade.

I usually run a "Coder review" on a Drupal project that was begun before I joined, to see if there are any glaring problems.

And I often run a review on my own code, just to make sure I have not been sloppy.

A great feature of Coder is the ability to create your own "rules"  for conventions that may be specific for your individual project, and the ability to choose to review only PHP or HTML code, and you can also limit the scan to specific parts of the Drupal application such as Themes or Modules. A common configuration is to limit the review to only custom modules and themes that you have written, to check your work.

Coder also includes a command line plugin Coder Sniffer , and if you are using the drush command line tool for Drupal, you can automatically re-format your code according to rules set in Coder. Like automatic translations, the result will be about as good as your algorithm.

Many people are surprised to find Coder warning or error results for contributed modules that have been a key part of any Drupal site for many years. Coder does not tell you the code doesn't work, it tells you when code does not fit a specified formatting rule. Sometimes there are functional reasons to NOT use the Drupal convention in a piece of code; often it does not really matter if the code "passes" Coder Review, and there are bigger priorities in your project than "fixing" code that works fine as it is. And since contributed modules are managed via Git version control, by "fixing" a contrib module you are forking it.

In the end, you need to make the judgment whether the Coder message has any bearing on the actual function and performance of your application. 

Shweta Aggarwal replied on Thu, 2014/08/28 - 11:07pm

Nice Article !! 

Not wasting time on code reviews ....is it also possible that we may skip code reviews altogether ,if we use a combination of static code analysis and functional verification tools.?

Like in our organization we  spend significant time reviewing design docs (both High/low level doc including use cases) and later on doing code reviews.

Using static code analysis tools like Sonar Qube to fix coding bugs and for  functional correctness if we map requirement correctly to use cases and use cases to wider set of Junits and ensuring all of them pass (which is also one of the best practices)  , would it be safe to skip code reviews assuming low level design document have been religiously followed.


Thanks!!




Philippe Lhoste replied on Fri, 2014/08/29 - 5:43am

Ah, that's funny. Perhaps because English is not my native language, I interpreted the title as "don't waste time in doing code reviews", and I went here thinking to find a provocative or controversial article... :-)

While actually the title is more like "don't waste time while doing code reviews", which is much more useful.

At work, we have Checkstyle and FindBugs in the Maven build, so we can see instantly when we did something basic wrong. One less problem. BTW, we don't use PMD. Is it really necessary when you have FindBugs? Are they really complementary?

And we have the rule to never commit a change without a code review, and we put the initials of the reviewer in the commit message.

Most of the time, reviews are just a seal of approval (we are a small team of senior devs), but we get enough useful / pertinent remarks to prove this practice is a good one.

The alternative is pair programming, which we practice from time to time, but which we find hard to generalize.

Md.shariful Islam replied on Wed, 2014/09/03 - 12:38am

Great Article!

This is one of the best article i have ever read on code review. i was also spending lot's of time reviewing code that a static code review tools can do. Thank you very much for clarifying almost all aspect of code review.



Greg Brown replied on Thu, 2014/09/04 - 10:15am

"I interpreted the title as 'don't waste time in doing code reviews'"

Same here, and English is my native language.  :-)  I think the title was either poorly worded or it was written that way intentionally as click bait. I think a better title would have been "Don't Waste Time During Code Reviews".


Werner Denzin replied on Sun, 2014/09/21 - 8:18am

Hi, 

Congratulations! It's a great article.

Do you know any static analysis tool for Web (JS, HTML, CSS) mobile development?

Best Regards,

Werner

Comment viewing options

Select your preferred way to display the comments and click "Save settings" to activate your changes.