If you are unfamiliar with a code review, at its core, a code review is a team analysis of code with the intent to find and fix bugs. With a little effort you can turn your code review process into a fantastic group activity that improves the codebase, increases codebase understanding, shares individual knowledge among team members, and improves the overall quality of a product.
To have an effect code review, I will walk you through what I feel is an effective and adaptable code review process based on my own experience. With so many different software department organizations, it is difficult to cover every possible scenario. I urge you to compare this process to what you currently perform, and see if your organization can benefit from any of these ideas.
The Most Common Mistake
A code review is not a design review. If serious design flaws are revealed during a code review then you need to improve the overall software process in your organization.
Design issues should have been resolved during the design phase. I know what some of you are thinking: “I don’t always know how the design will work itself out until I write some code.” That is perfectly fine. Prototyping code can be performed during the design phase. Learn what you need to from the prototype. With the lessons learned from the prototype, a solid design should be created and reviewed by the team before implementation.
Design reviews are a topic I plan to discuss in the future. For now I will just state the following: If you don’t perform a design review before the code is implemented and major design changes are required after the code review, then you have wasted some amount of time during the code implementation of the design. You have also wasted the code reviewers time because they will have to review the code again after the design changes. My hope is that after finding this wasted time over and over, developers interested in improving productivity in their group will try to improve their organizations software process.
Several members of the software development team have a role to play to help make the code review process effective. The following section describes each role and their part in the review process.
Management: Managers need to create two simple assets that will help both the development team and themselves. A skill chart, and a module knowledge chart. We will discuss the application of these charts a little later. Regarding developer bug metrics, it is extremely important for managers to make this a positive experience for the developers. Don’t collect author bug counts and try to use that information against the authors, unless of course you enjoy destroying team morale and looking for replacement developers. This goes double for non-technical managers. Now back to the charts, for simplicity, let’s assume this manager is only managing one software project.
Skill Chart: The skill chart is a list of all technical skills relevant to the project being managed. The second piece of information on this chart is the name of every person being managed. Fill in the skill list for every member of the team. This chart is useful for large teams where developers may not know the skill set of the other members of their team.
Module Knowledge Chart: The module knowledge chart should list all the modules in the project being managed. The second piece of information on this chart is the name of every person being managed. Fill in the knowledgeable members of the team for each module. This chart is useful for large teams where developers may not know what part of the application is know by other members of their team.
Code Author: The person or persons who authored the code to be reviewed. It is important for this person to either have or develop a “thick skin”. When you submit a code review you are asking for a peer evaluation of your work. Keep that in mind when you receive the feedback, and remember this process exists to help everyone on the team.
Reviewer: Any person performing the code review. This person could be a manager, developer, or tester depending on your organizational layout. Reviewers should keep in mind the author may take the review personally, so keep it professional when presenting change requests.
Code Review Sheets
There are plenty of software tools (similar to bug tracking tools) to help you out with the code review process. If a software tool interests you, do some research and find a tool that can replace what I am about to discuss. For those of you who are new to this process, you can get started by creating a code review spreadsheet. This is a simple electronic resource that can be distributed to the team. Note: This description assumes you are using source control.
A code review sheet can be a simple spreadsheet used to track everything that occurs during the code review process. A simple code review sheet should contain two sections:
Code Review Sheet Header
|Change Request||Bug 2127|
Author The author or authors of the changes to be reviewed.
Change Request The change request should reference a writeup of all the changes that were to occur in this change set. It could reference one or more of the following: a requirements document, design document, or bug report.
Source Code Location The location where the source code can be retrieved by reviewers. Ideally, this location is a self contained branch in source control. After retrieving the code, it should be easy for reviewers to compile and run the software.
Revision Start And Stop The start and stop locations in the repository history. These are references to the revision history for the specified source code location.
Code Review Sheet Defect Log
|File List||Line Numbers||Comments||Defect Type|
|/modules/network/NetworkAdapter.cpp||101-127||Need to deallocate memory in ~NetworkAdapter()||Memory|
|/modules/users/Users.cpp||15||Document Users() constructor||Documentation|
File List The file list should contain a relative path to all files changed in the change set. If a reviewer finds multiple bugs in one file, the row containing a file name should be duplicated leaving one line for each bug in every file.
Line Numbers Help the author narrow down the location where changes need to occur. The line numbers are provided by the reviewer.
Comments Comments should describe what the problem is with the code at the specified line numbers. Avoid using terms such as this and that. Use class, function, or variable names when referencing problems. Providing specific descriptions and solutions will aid the author in making code review changes.
Defect type The defect type is useful if you would like to start tracking metrics for the bugs found. The defect types need to be decided on at an organizational level. I feel it is important to mention that management should not be using code review defects against the authors. These bugs were all found before shipping the product to the customers and the reviewers deserve praise. If you were going to do anything useful with the data, you should show a cost savings for the company by performing code reviews. Bugs that make it into the hands of the consumers can be quite costly to fix.
Choosing The Code Review Team
When choosing the code review team, this is where the Skill Chart and Module Chart will come in handy. When you have this information, look at the change set along with the two charts and select a review team in which each member will benefit in some way from the code review.
A simple application of this idea is to select a reviewer who knows more about the modified module than the author making the changes. To find a second team member, perhaps find someone who knows less about the implementation language than the author. In this scenario the author and at least one reviewer bring something positive to the review process. Keep in mind, there are a number of criteria you can feed into the selection process. Module knowledge, language experience, relevant tool experience, knowledge of innovations from the academic world, and work experience immediately come to mind.
The point of the selection process is to create a team that can teach each other, share knowledge, and improve the quality of the end product.
Code Review Process
The code review process is defined by the following elements:
- Prepare Code Review Sheet
- Select Review Team
- Understand Requirements
- Static Analysis
- Verify Dependencies
- Active Analysis
- Review Meeting
Prepare Code Review Sheet
It is important that the author reviews their own code (self-review) before submitting a code review request. Upon completion of the code changes, the author should fill out the code review sheet header and the file list with the assumption that their code is in perfect condition. Filling in the file list removes duplication of effort because this list will be used by each reviewer.
Select Review Team
When the author has filled out the code review sheet and a review team has been selected, the reviewers should receive a sheet from the author and begin the code review in the near future.
It is best if the code reviews are performed individually by the reviewers for proper scrutiny. If you perform the code review as a group in a meeting, each reviewer may not be bringing their best ideas to the table. Reviewing in a meeting also allows people to slack off with the hopes that others are paying more attention which defeats the purpose of this process.
Reviewers should do their base to make the turn around time on review request as short as possible. It can be difficult to drop what you are doing and perform a code review, so wait until a good breaking point, then perform the review in one shot to help minimize interruption with your current project. After all, the reviewers will need the same in return very soon. This is where making change sets in a branch becomes beneficial. With this approach, an author can work on a separate project while waiting for a code review.
The reviewers need to get up to speed on what changes were requested of the author. A reference to the initial request can be found in the header of the code review sheet. Reviewers can use this information to verify the change set is relevant to the change request.
Once the requirements are understood, the static analysis should be a very straight forward task. The code should be analyzed looking for the following problems:
- Code Smells
- Compiler Warnings
- Mistakes Implementing The Design
- Deviation From Coding Standards
- Deviation From Best Practices
If your organization does not already have a set of coding standards, you need to define them for a code review to be effective. Without the coding standards, the review can become an unproductive free-for-all.
If you don’t already have a best practices guide for the language used in the project, find one and make sure everyone on the team has access to it. You can even point your organization to an online resource. There are plenty of agreed-upon code practices available on the Web.
If you run across a problem not covered by coding standards or best practices, find an agreed upon solution and update one of those guides to save time for the next review team.
Verify that all relevant documentation that should have been updated was in fact updated. Some document sources include:
- Help Documentation
- Training Guides
- API Guides
If a software interface was modified in the code, make sure the client code was updated to use the new interface. Sometimes the function signature stays the same, but the arguments into the function and the value returned from a function take on a new meaning with the new change set.
Run any possible tools to verify the code performs as expected. These tools may include the following:
- Executing a linter
- Executing unit tests
- Executing the modified application
- Checking for compiler warnings
When the code analysis is complete, the author and code reviewers should meet and discuss each change request. Everyone should be compiling a list of lessons learned during the review. As an author, you should reference the lessons learned when performing your next self-review (before passing out the code review sheets) to check for mistakes that you commonly make.
Without the review meeting, there would be little to no interaction among the group members and the same amount of knowledge transfer. With an experienced team, you may be able to forgo this meeting.
Resolving Heated Debates
“Arguments” are inevitable. Keep in mind the author may feel personally attacked during this process even when a personal attack was never the intent. There is no reason for these heated debates to become uncivilized. The point of the review is to improve the quality of the overall product, learn from each other, and improve the codebase. When the meeting fails to progress there at least a few different ways to move on.
If the implementer has satisfied the requirements, and passes the best practices and coding standards test, but a member on the code review team is still unhappy, determine what the exact issue is. See if this issue can be resolved by updating the best practices or coding standard. If you feel it is truly an important issue, you should get the criteria on which you are being judged updated.
If the dispute cannot be resolved through some addition to the coding standards or best practice, create a table with benefit and trade-off columns and judge the two solutions. As a code review group, rate the benefit and trade-offs and how they relate to the initial change request.
With so many implementation possibilities, if the implementation strategy satisfies requirements and everyone involved in the review had a chance to comment on the design before the implementation, just let the implementer decide on the final solution.
If a resolution cannot be reached, consider tabling the issue and bringing the idea in question to the next team meeting. I think this is the worst resolution. You were all hired to be problem solvers, why weren’t you able to come to an agreement?
To reiterate, the goal of the code review is to improve the codebase, increase codebase understanding, share individual knowledge among team members, and improve the quality of the product. By performing code reviews you can perform all of these tasks with one group activity.
Your software development methodology may include some sort of paired programming activity. I enjoy paired programming for some projects and as a team of two it is nice to get immediate feedback on the code you are writing. I do find that on some days one person in the pair can get carried away at the keyboard and the second person can fall behind. If you find yourself falling behind in this scenario, you may want to consider a code review when the pair of you are done coding.
The skill chart and module chart have additional uses beyond the code review process. Management can use these tools to verify every important area on a project is covered by more than one person. You never know when someone might decide to look for a new job. New team members can look at the chart and determine who the address specific questions to without interrupting the whole team.