Code Review Process

A code review should accomplish a number of things, listed below, through exposing code to a larger set of skilled programmers then was originally involved with the initial implementation of the code.

Because writing software is much like any other form of engineering the mechanics of the product are vitally important. For example, the review should ensure that applicable design patterns are used where they should be, and not used where they shouldn’t be, and that they are correctly implemented. That error conditions are handled correctly and that input is properly checked for validity. It should assure that the code is readable and will be easy to maintain and that deprecated or outdated methods are not used.

Innumerable lines of code have been written over the decades. Many of these lines could have probably been avoided if code was made modular and those modules were used. The review should ensure that repetitive functions and methods used throughout the program are collected and modularized. If those modules would be helpful outside of the scope of the application they should be easy to remove and use in other applications. More importantly the review should check that the application uses existing modules, either in-house developed ones or third party modules.

One of the largest complaints about code is that there is never enough documentation or the right documentation. The review should make sure that the requirements and design documents are accurate and that the code is documented thoroughly and legibly. This includes not only JavaDoc comments but also comments within the methods themselves.

Because of the nature of this department and it's use of parallel development, almost no one is familiar with each other’s code. Therefore, after the code review, people should walk away feeling as if they understand the code well enough to be able to pick it up and work on it in a very short time.

Before the review is preformed the following steps should be done.

• A version of the code that is thought to be working should be frozen until after the review. Meaning no new features or major changes should be made to the code base. Minor bug fixes are acceptable.
• The requirements, design documents, and location of the actual source code should be sent to everyone participating in the review. The documents should be read thoroughly and in their entirety before the review. The code may be viewed to allow the other developers to gain some familiarity with it before the review. No comments should be sent to the coder before the review however.
• Optionally, and optimally, the code would also have a test harness that could be reviewed by the other developers while reviewing the code.

During the review the code will be displayed to everyone, via an overhead projector for example. Developers are encouraged to bring printed copies of the code as well. This will help on eye strain from looking at the display for long periods of time and facilitate personal note taking about sections of code being reviewed. The author of the code will then begin to walk through his code, in whatever order they believe makes the most sense, and describe what is occurring in the code. Reviewers should ask questions and comments along the way about the code. These questions and comments should be formed in such a way as to facilitate the stated goals of the review. Things like why a particular method was used for doing something, what a particular function is really doing, suggestions for bettering the code, etc. are all perfectly valid. If changes are suggested they may be made at the behest of the author of the code. Major code modifications, however, should be made after the review. During the review the reviewers are encouraged to be constructively critical of the code. They are expected to tear the code apart at the finest grain. The author of the code should expect this during the review and should not take personal offense at this. However, the reviewers must offer their criticisms in a professional manner. Outside of the author and reviewer roles there will be two other roles people will need to fill during the review. The people who are to fill these roles will be designated prior to the review. The author may not be considered for either of these roles and those that fill these roles are expected to actively participate as a reviewer of the code as well. The facilitator will be responsible for, as the name suggests, facilitating the review process. They will run the keyboard and mouse of the machine displaying the code and will make the minor code changes requested by the author during the review. They may also do things like highlight a particular section of code in discussion or whatever else they feel may help during the review process. More importantly, however, the facilitator is responsible for making sure that discussions remain on topic and constructive. They may, at any point, end a particular discussion (perhaps requesting it be carried offline), table a discussion, or identify a discussion that has gone off on a tangent and refocus the discussion on the original issue. The note taker, the second role, is responsible for producing documentation that records what occurred during the meeting. At a minimum they should record the following items:

• Who was at the review
• The date and duration of the review
• Changes made to the code during the review in a fairly high level manner, that is they don’t need to record the exact syntactical change to the code, just the logic changes.
• Major modifications that need to be made to the code at a later date.
• Good methods/algorithms for handling common problems that are found to be useful by some of the members of the people participating in the review. This will be fairly subjective and reviewing members are encouraged to ask the note take to record a particular method/algorithm.
• Common items that should be added to the list of the checklist of things to keep in mind during any review, for example a particularly common problem found throughout different sets of code.

Within a week of the completed review the following post-review items should be done:

• Notes from the review should be posted in their designated spot.
• Major modifications discussed in the review should be made to the code (depending on how major the changes are this may take more then a week)
• Updated documentation and review checklist should be posted.