Code review is the systematic examination of source code by a team, primarily to improve software quality. Sometimes, the review team can be different from the team that originally developed the software. I present a few guidelines and share my thoughts on what makes for an effective code review.
There are a few pre-requisites for code reviews to be effective. These range from organizational culture, team dynamics and experience of the team with of the code review process.
Sponsorship: The company's senior management should believe in improving software quality. They should allocate the necessary resources and time. Ofcourse, architects, team-lead and the team members should be quality conscious and be convinced of the need for code reviews to achieve superior code quality. They should feel that code reviews are a worth their time and not just another check-box in some process document. This is the number one requirement for an effective code review.
ROI Expectations: Code reviews, when conducted properly, will improve the code quality. Sometimes, code quality cannot be measured by defect count alone.
- tend to improve the overall code structure and there by aid maintainability.
- promote refactoring which improves code re-use.
- improve the code to handle the corner cases.
- improve the test cases and there by improve test coverage.
- fix critical and rare defects - most of the multi-threading and concurrent access bugs tend to fall into this category.
- improves team morale.
As you may have realized, many of the results brought about by the code reviews sometimes cannot directly be quantified. So it becomes difficult to establish the Return On Investment (ROI). It is important to establish the ROI expectations correctly. The returns on time and effort spent on code reviews will be realized over a longer time period.
Team dynamics: The team should be self-conscious and guide its own operations. The team should have clear objectives and the atmosphere should be informal where people feel free to express their opinion.
Transparency: The team should document their guidelines and policies. Some of the examples are:
- coding conventions
- database naming and design guidelines
- secure coding practices
- other policies that the team decides
Code reviews can become very subjective. People forget as to what guidelines/conventions were agreed upon. Sometimes these guidelines / conventions may have been updated. So documenting these guidelines / conventions is very important. These documents serve as a reference and force the team to be objective in deciding if certain code meets certain policy / convention or not. If the team feels the need and agrees then a new guideline / convention may be added or an existing one updated.
Team building: The team should use this occasion to:
- build rapport with their team mates
- learn from the subject matter experts and increase their domain knowledge about the product
- learn architecture, design patterns and other best practices from more senior team members
- educate their fellow team members.
Code Review Process
Team should discuss and agree upon a code review process. This should be documented and should be frequently updated based on team's feedback.
Agenda & Scope: Code review should be treated similar to any other team meeting. It should have an agenda and scope. The scope should include the code being reviewed and it should address if there is any special emphasis - like performance, security etc.
Preparation: The review team should get the code being reviewed well in advance along with all the relevant documentation. The review team members should go through the code individually and list their concerns before coming to the code review.
Tools & Automation: Team should take advantage of the many tools that are available to perform code analysis. Many of them are highly configurable and team should come up with these configuration rules. These code analysis tools should be automated and be a part of the build process. These tools will help to catch simple bugs and effectively save a lot of time/tedium for the team.
Some of the tools in this category are:
Code review: The individual team members have a list of issues that they would have prepared prior to the code review meeting. The review team addresses these issues in order. The issues fall into the following categories:-
- code does not follow naming convention - this should have been addressed by the coding and naming convention documents and tools
- code has a defect
- code has a defect that rarely occurs - as mentioned these are very critical and mostly related to multiple threads or concurrent access.
- code has a security issues - buffer overflow and pointer magic fall into this category.
- code violates business rules - clarify with domain experts
Action items: Code review is an intense activity. Lot of ideas are generated and discussed. It is very very important to take notes and capture important ones as action items in the change management system. Some of the issues captured will be defects. Some would get filed as new features or future ideas. Yet some other action items can be to update the business rules or other documents. In all the cases the issues will be given proper priority and handled as 'change requests'.
By capturing the action items in the change management system, code review process can be well integrated with the development process.
Follow up: To gain adequate benefit of the code reviews, the team has to allocate sufficient time or resources to follow up the issues raised by the code review. That is why it is very important to capture the issues raised by the code walk into a change management system so that these are given proper priority and so that the issues are not lost.
Offline & Virtual Teams: If the team is geographically distributed or if the team is huge then it presents its own challenges while conducting code reviews. The code review process remains largely the same except that the communication now happens through e-mail, message boards and WIKIs.
Code reviews present their own unique challenges. These are some of the pit-falls to watch out for.
Witch-hunting: Don't mis-use code reviews just to criticize and settle old-scores with your colleagues. The reviewee should trust the reviewer to provide him with honest and constructive feedback. Likewise, the reviewer should be willing to share his domain knowledge and subject expertise to improve the design/code quality. Lack of trust can either totally prevent any form of feedback so as to not to offend the reviewee or it can go to the other extreme and feel like witch-hunting.
Deadlock: Sometimes team members do not agree with each other on the proposed solutions to an issue raised during code review. The existing policy and convention documents fail to resolve this argument. In such a case it is best for the team-lead or architect to come to a conclusion and resolve this issue. If the issue is not time-critical then capturing this issue in the change management system (or issue tracker) and moving on may be a good alternative.
Lack of Trust: Code reviews are mistakenly perceived as management's lack of trust on the programmer. Such an attitude and perception is counter productive. Management must make every effort to promote trust and prevent such perception.