A Guide to Good Code Reviews

#codequality #agile #tdd #webdev

Making sure that more than one set of eyes has seen all the code that we produce is an important part of software development - it makes sure that we catch bugs, keep our code readable, and share patterns and practices across the teams.

Code review should answer the questions

Are there any logical errors?
Are the requirements implemented?
Are all the acceptance criteria of the user story met?
Do our unit tests and automation tests around this feature pass? Are we missing any?
Does the code match our house style?

The interesting thing about the effectiveness of code review is that it isn't just hearsay, it was measured effectively in the seminal book "CODE Complete":

.. software testing alone has limited effectiveness -- the average defect detection rate is only 25 percent for unit testing, 35 percent for function testing, and 45 percent for integration testing. In contrast, the average effectiveness of design and code inspections are 55 and 60 percent. Case studies of review results have been impressive:

In a software-maintenance organization, 55 percent of one-line maintenance changes were in error before code reviews were introduced. After reviews were introduced, only 2 percent of the changes were in error. When all changes were considered, 95 percent were correct the first time after reviews were introduced. Before reviews were introduced, under 20 percent were correct the first time.

In a group of 11 programs developed by the same group of people, the first 5 were developed without reviews. The remaining 6 were developed with reviews. After all the programs were released to production, the first 5 had an average of 4.5 errors per 100 lines of code. The 6 that had been inspected had an average of only 0.82 errors per 100. Reviews cut the errors by over 80 percent.

The Aetna Insurance Company found 82 percent of the errors in a program by using inspections and was able to decrease its development resources by 20 percent.

IBM's 500,000 line Orbit project used 11 levels of inspections. It was delivered early and had only about 1 percent of the errors that would normally be expected.

A study of an organization at AT&T with more than 200 people reported a 14 percent increase in productivity and a 90 percent decrease in defects after the organization introduced reviews.

Jet Propulsion Laboratories estimates that it saves about $25,000 per inspection by finding and fixing defects at an early stage.

We do code reviews because they help us make our code better, and measurably save a lot of money - the cost of fixing software issues only multiplies once they're in production.

Code review check-list

Not sure how to code review? Here's a check-list to get you started, derived from many excellent existing check-lists.

General

Does the code work?
Does it perform its intended function?
Is all the code easily understood?
Does it conform to house style, standard language idioms?
Is there any duplicate code?
Is the code as modular as possible?
Can any global variables be replaced?
Is there any commented out code?
Can any of the code be replaced with library functions?
Can any logging or debugging code be removed?
Has the "Boy scout rule" been followed? Is the code now better than before the change?

Security

Are all data inputs checked (for the correct type, length, format, and range) and encoded?
Where third-party utilities are used, are returning errors being caught?
Are output values checked and encoded?
Are invalid parameter values handled?

Documentation

Is any unusual behaviour or edge-case handling described?
Is there any redundant auto-documentation that can be removed?

Testing

Is the code unit tested?
Do the tests actually test that the code is performing the intended functionality?
Could duplication in test code be reduced with builder / setup methods or libraries?

Practical Tips

Don't try to be a human compiler

The first and most important thing to remember when you're doing a code review is that you're not meant to be a human compiler. Ensure you're reviewing the functionality, tests and readability of the code rather than painstakingly inspecting syntax. Syntax and house style are important, but style issues are a much better fit for automated tooling than humans. Don't waste time bickering over style and formatting.

Reviewers are born equal

Our teams are built around mutual trust and respect, and a natural extension of that is that anybody can code review. You may on occasion find yourself working on some code that is someone else's area of particular interest or expertise - but better solicit their advice while you're working than wait for a review. Code reviews aren't limited to your technical lead, and likewise, a lead should have their code reviewed all the same.

It's just code, you're not marrying it

Don't be precious about your code as a submitter, and as a reviewer be honest and open. It's just code, make sure it's the best it can be. A code review is an opportunity to make your code the best code it can be, using the expertise of your colleagues.

That's fine!

Sometimes the code is just fine - don't be the person that nitpicks for change without any quantifiable benefit.

Tooling

Code reviews should be performed on change-sets via either a branch comparison URL, or ideally, a pull request.

Stash has replaced our legacy tool (FishEye) for code reviews using Git, and as projects migrate from SVN they are expected to migrate to using branch comparison or pull requests in Stash.

Pair programming and code reviews

As one of the founding practices of eXtreme Programming (XP), pair-programming can be seen as "extreme code review"

"code reviews are considered a beneficial practice; taken to the extreme, code can be reviewed continuously, i.e. the practice of pair programming."

Pair programming exhibits exactly the same qualities of a great code review, with the additional benefits of immediacy - it's impossible to ignore a pair critiquing a design in real time, poor choices barely survive, and work doesn't end up blocked in a code review queue somewhere.

Pairing is usually preferably to code review for regular work, and if code is produced as part of a pair it can be considered "code reviewed by default". Unfortunately due to reasons of availability, location or specialisation, you may need to rely on a more tradition code review with some of the code you write.

Realistic expectations

Like everything a code review is not a silver bullet - and while they've proven to protect us against a large quantity of visibly obvious bugs, code reviews aren't great at spotting performance bugs or subtle threading and concurrency issues.

You'll need to rely on existing instrumentation and profilers for these types of metrics - "mentally executing" and finding these kinds of bugs is unlikely, if not impossible, just beware to not believe your code to be free of error by virtue of the fact that it's been subjected to a code review or pairing session. Ironically, the kinds of bugs that'll slip through a code review or pairing session will by definition be these tricky and hard to detect edge cases.