Code Buddy: The Art of the Weekly Code Review

As a software engineer for Intermountain Healthcare, I have been tasked with implementing a plan for code reviews on our team. After thinking about it for some time, I have decided to implement weekly code reviews as a nice balance of achieving the goals for having code reviews while overcoming some of the problems involved with more formal code reviews that happen at the end of the project. A code review that happens at the end of a project has limited usefulness for the project itself. The later in the project life cycle the review is held, the harder it is to act on the issues. It is often too late in the project schedule to do anything about issues identified as part of the code review. The decision to meet the schedule versus fixing the code will always lean toward the schedule. Holding code reviews earlier in the project allows issues to be corrected with less impact to schedule. On the other extreme is pair programming where code is continuously reviewed. While there is a lot to be said for this concept, there is a lot of drawback as well. One of the most important characteristics of an effective reviewer is being dispassionate. A person involved on a continuous basis with the code will tend to have a motivation to let things slide to meet the schedule. A person who is not so involved in the project can review the code with a more objective eye. Code Buddy Somewhere in-between the extremes of pair programming and end-of-projects code reviews is the code buddy. A code buddy is some one who reviews the code on a regular, in our case weekly, basis. To ensure objectivity, the code buddy does not work on the code base being reviewed. Instead, the code buddy is assigned from the people working on a different project or a different part of the same project. To see why the code buddy works, a review of why code reviews are needed is helpful. As a team, we talked about what we wanted out of a code review. Those items follow with how a code buddy will or will not achieve that goal. Also, how we can measure the outcome for continuous improvement. Produce higher quality products This is one of those nebulous goals that is hard to measure. What this really needs is a definition of what is meant by higher quality code. Find more bugs There are several good static code analyzers like Checkstyle and FindBugs. which can identify bugs that are commonly overlooked. In addition to these tools, an effective code reviewer will identify bugs and potential bugs that are overlooked by the developer. The earlier in the process a bug is identified and corrected, the less costly the bug is to the project in time and money. Bugs can be caught throughout the process in roughly these steps, in order of increasing cost:

• not written - the cheapest bug to fix is the one never written
• caught by the developer during coding, by running unit tests or through continuous integration - these bugs are quickly addressed and cost very little
• caught by a code buddy - only slightly more costly than a unit test
• caught by QA - now the bug will need to be reviewed and possibly effects the schedule
• caught by a formal code review at the end of the project - might slip the schedule or may never be addressed
• caught by a user - the most costly in time, money and reputation

By identifying issues earlier in the process, a code buddy who reviews the code weekly helps meet deadlines and keep costs down. Follow standards and best practices Each organization identifies its own standards and best practices. Using automated tools as part of the continuous integration process will ensure adherence to some the standards. Others cannot be automated and it takes a human looking at the code to ensure that the standards are being met. The sooner a deviation from the standard is identified, to easier it is to correct. Looking at the code weekly ensures that the code does not deviate too far before being corrected. Code buddies will also need on-going training in the standards and what to look for while reviewing. This will reinforce the standards for the whole team. Identify security threats In this day and age, security needs to be part of every project. However, security concerns are often separate from the business logic the developer is seeking to implement. By taking a regular step back for the business logic and looking at the code as a objective third party, the code buddy can help see security issues that a developer will overlook. There are all sorts of security issues and a weekly code review of a snapshot will only be able to identify a subset of the issues. For example, the code buddy will not be able to see how seemingly in secure components interact in insecure ways. For this reason, a thorough security audit of the whole project should be conducted at certain milestones. Find common solutions Sometimes a developer write from scratch something that has already been written, reinventing the wheel. There are high quality libraries available both internal to an organization as well as from third parties. A code buddy can help reduce the overall code base by identifying these reinvented wheels and suggesting better solutions. Makes it maintainable The first question a reviewer should ask is would I want to maintain this code? If the answer is no, then the reviewer should identify the specific issues and bring them to the attention of the developer. It is important not to criticize the developer, but focus on the code. Mentor and cross-train each other As people review each other's code, they naturally learn things they can use in their own code. Prevents the silo effect The silo effect is what happens when a developer works under time constraints without anyone looking at the code. Corners get cuts, short cuts taken and really strange things happen in the dark. A regular review sheds needed light on the code and encourages developers to write it right the first time. Improve performance Like security, performance happens a many levels. A code review can identify some obvious problems, however any issues identified in a code review should be reviewed by a profiler to ensure that there really is a performance issue. Sometimes efforts to improve performance have the opposite effect. Verify unit tests are being written Unit tests are most effective when written early in the project. A weekly code buddy can verify that code has corresponding unit test. Code buddy process Each week the developer will create a code review in Crucible, which is a code review tool that integrates with source control like subversion and cvs. The code review consists of all code committed in the last 7 days. The members of the team, a code buddy and a code captain, either a team lead or other who helps facilitate the review, are invited to the review. Using crucible, the code review can be setup in a matter of moments. The reviewer is notified by email and logs into the crucible server and does the review. Crucible shows the reviewer only the code that has actually changed, think diff. This allows the reviewers to keep up on the changes without having to dredge through lots of code that has not changed. Also, if the committer attached the Jira issues, the reviewer can easily see the motivation for the change. The reviewer can make make comments online. There is no need for a formal meeting. Instead, each logs into the tools and make comments. The other reviewers are notified by email when comments have been made, allowing them to respond in a timely manner. Once all the reviewers have finished, the code either passes or the developer agrees to make the suggested changes. This may involve making new Jira issues to track the changes. The changes will naturally be reviewed in the following weeks as they are made and committed. Encouraging effective code buddies Some steps to ensure that the reviewers are being effective

• on-going training on standards, security, etc.
• switch code buddies every few months - codes people from getting too comfortable
• provide a checklist of things to look for - one follows

Code Buddy Checklist

• Would you want to maintain this code?
• Is the intent of the changes in the code readily understood either from the code itself, the comment changes, the JIRA referenced or other supporting documentation?
• Are there any security flaws?
• Are there easy better ways of doing this? Things like: use standard libraries, a simpler algorithm, reduce complexity.
• If a complex solution is required, is it properly documented in the code or the javadoc?
• Does the code meet standards?
• Are there unit tests for the public methods of public classes?