Code Review¶

In an ideal world, all new code that is written will get reviewed by one of the more experienced developers in the group. This is a really useful way of

• finding bugs (studies show that 30%-50% of bugs are caught with one hour of code review)
• ensuring high quality code by:

• checking that the code is adequately tested
• checking that the code is documented properly
• checking that the code has the correct style

• but most importantly spreading knowledge of the code

We certainly aim to review major chunks of code written by developers new to the project.

Requesting a Code Review¶

If you would like a piece of code reviewed before it is merged, set the workflow field to Under review and email your project supervisor (Chris Rogers if you don't know who the project supervisor is).

If you have an existing piece of code and you would like it reviewed, raise a new issue listing:

• any relevant Issues
• list of files to be reviewed
• list of features to be reviewed
• any special comments or features

You might want to prompt someone to actually do the review. It would probably be helpful if you could go through the checklist below yourself and ensure that everything is okay to the best of your ability before the review.

Performing a Review¶

You might want to arrange a phone call beforehand to allow the developer to walk through the code.

Reviewers should look at the following things:

• Doxygen documentation:
  • are all of the files documented? (sometimes doxygen ignores files e.g. if there are syntax errors)
  • Does the documentation make sense? Is it reasonably well laid out?
• Now look at the code
  • Is the code clear?
  • Any bugs?
  • Any badly laid out logic?
  • Any cut n paste?
  • Are functions performing one clear logical task (ie. no more than 10-20 lines of code)?
• Now look at the tests
  • Are the tests complete? (Estimate test coverage, look for holes)
  • Are the tests good quality? Do they check edge cases (arrays out of bounds, bad user input, etc) or just run thruogh the code and check "it works"
  • Are the tests reasonably clearly laid out?
• Finally look at the coding style
  • Does the code conform to the style guide? Worth going through the list of things that are in the style guide but not in cpplint.py here
  • Does it pass cpplint.py or pylint?
• Use some of the automated tools
  • Any problems flagged by Jenkins?
  • Any problems flagged by Coverity?
  • Try running valgrind (C++)
  • Try running coverage (python) lcov (C++)
• Modifications to build script
  • Any new third party bash scripts follow the usual pattern?
  • Does scons check for library existence correctly?

Writeup¶

In the original code review requesting issue, stick a summary of your findings.

You should make a recommendation that either:

• The code is good
• The code is okay but needs some tweaks
• The code is okay but needs some changes followed by a further review

Once both reviewer and developer are happy you can close the issue.

Sometimes it's a good idea to give the developer a phone call to explain results, especially if you recommend a further review.

See Also¶

Googley...

http://en.wikipedia.org/wiki/Code_review
http://en.wikipedia.org/wiki/Fagan_inspection
http://www.codinghorror.com/blog/2006/01/code-reviews-just-do-it.html