Contents 1 Code Review FAQ 1.1 What is a code review? 1.2 What is the purpose of a code review? 1.3 What constitutes a bug/defect? 1.4 What should happen during a code review? 1.5 When should we perform code reviews? 1.6 What should I do if I find something wrong during a code review? 1.7 How do I request a formal code review? 1.8 What are the questions we want to ask about the code? 1.9 What are the Requirements for the software? 1.10 What information do we need to keep track of during a code review? 1.11 What types of issues are we looking to capture during code review? 1.12 What are some common errors encountered during code reviews? 1.13 What should be a candidate for code review? 1.14 How much code should be reviewed in a single code review session?

Code Review FAQ

Back to Code Review Overview

What is a code review?

• A code review is a process by which developers examine source code in order to discover bugs, scrutinize coding conventions, and look for potential bottlenecks and resource leakage.

What is the purpose of a code review?

• The intent of a code review is to catch bugs/issues/defects before the offending code is deployed to a production environment.
• To transfer knowledge of implementation details to the rest of the team.

What constitutes a bug/defect?

• Code that does not satisfy its requirement.
• Code that does not work as intended, regardless of requirements.
• Code that does not perform as expected.
• Code that is difficult to read and maintain.
• Code that is not commented.

What should happen during a code review?

• During a code review, reviewers should focus only on reviewing code. Making code changes during a code review is discouraged. Making changes to the code while reviewing the code can be counterproductive because the reviewer’s recommendations might be overturned by other developers. We want to avoid making multiple changes to the same bug/issue. The reviewer should deliver comments/recommendations as the final artifact of a code review. With that said, we should develop a checklist of coding conventions (including javadoc/comments) that CAN be corrected during a code review.
• This approach might not be “cool” with everyone, so we can come up with an alternative if necessary. For instance, I have read that some teams tag a set of files they are going to review and then collaboratively work on those files, making edits where necessary. If we adopt a read/write policy for code reviews, we should make sure to focus on a small set of files.

When should we perform code reviews?

• Pre-commit Code Review: The first line of defense is the developer who wrote the code that is being committed. This developer should go through the same process that we outline
• Post-commit Code Review: After code has been committed, developers should review the committed code. Each new code commit should be scrutinized by several members of the development team. We have set up an RSS feed to facilitate this approach. Developers should subscribe to this RSS feed
• Scheduled Code Review: On occasion we should perform code reviews on large code sets (i.e. the form entry component, an entire branch). These code reviews should be performed by a team of reviewers and should be scheduled ahead of time.

What should I do if I find something wrong during a code review?

• Report it – Send email to developers list
  • We need to make sure that this is a constructive process. The reviewer should not slam or even poke fun at the developer. Code reviews are meant to be both constructive and instructive. Not all of us have the same level of understanding of the code, so please be kind.
  • In addition, the developer should not be defensive about his/her code. If the developer disagrees with the reviewer (which is totally reasonable), that developer should state their case and ask other developers for their opinion.
• Clarify it – Send email to developer asking for clarification

How do I request a formal code review?

• TBD

What are the questions we want to ask about the code?

• See the Code Review Checklist.

What are the Requirements for the software?

• We need to create a template for requirements
• If requirements are not written before the code review they should be drafted during the code review to improve traceability.

What information do we need to keep track of during a code review?

• Software Files/Versions
• Scope of Review
• Bugs Found
• Required Next Action
• Ticket Number
• Date of Fix
• Date of Follow Up

What types of issues are we looking to capture during code review?

• Maintainability issues
  • Following code conventions (readability)
  • Following comment/javadoc conventions
• Features not being implemented correctly
• Requirements not being followed
• Deadlocks
• Bottlenecks/Performance Issues
  • This is more appropriate during testing since developers should perform routine profiling and load testing. However, some performance issues are relatively obvious.
• Candidates for Refactoring
• Comments on Design/Implementation
• Resource Leaks – database connections, etc
• Error Handling – does code handle errors correctly
  • We need a coding convention for exception handling
• Bugs – does the code do what it’s supposed to
  • Do we have JUnit tests defined for this feature?
• Security issues – not all that common, but we should have a list of common issues so we know what to look for
• Traceability – we currently don’t have good requirements, so this is difficult
• Code Quality – link to coding conventions
• UI (JSP, Javascript) vs. Java components

What are some common errors encountered during code reviews?

• TBD

What should be a candidate for code review?

• "... it makes sense to concentrate on code that links objects together, rather than internal behavior of an object, since that’s what unit tests are for. That made a lot of sense to me — the unit tests exercise the objects themselves, which means the risk of defects shifts more to the integration of objects. Targeting the reviews at the integration code is much more likely to catch exactly the sort of defects that unit tests would miss."
• Recently committed changeset
• Code that may be a candidate for refactoring
• Code that “integrates” with another subsystem
• Code that has been deemed “buggy” by a developer or implementer
• Code that a developer for which a code review has been explicitly requested
• Code for which it is critically important to be stable

How much code should be reviewed in a single code review session?

• According to the Stellman-Greene site, “many teams have found that it takes about two hours to review 400 lines of code”.