Purpose

The purpose of payment gateway code reviews is to improve the reliability and the security of the COP Payment Gateway. The Payment Gateway continuously processes payments for numerous applications sponsored by multiple city bureaus. A formal code review procedure is important for validating that the Payment Gateway is as robust and secure as is possible.

There are many tools that assist in validating the quality of code. Some of these are advanced integrated development environments that include syntax highlighting and usage hints, static code analysis tools, and test suites. All of these are important parts of validating the correctness of code, and they can be very helpful used in conjunction with peer review, but to date there is still no more effective method of finding coding problems than peer review.

Code reviews are also a very cost effective method for finding code problems, since they find them early in the development cycle, when they are much cheaper and easier to fix. This is especially true for the Payment Gateway, which requires nearly continuous uptime for handling sensitive payment information.

Goals

The goal of a code review is to validate that a software component is:

1. Well designed
2. Satisfactory per its functional requirements
3. Satisfactory per its performance requirements
4. Securely coded
5. Properly documented

Participants strive to minimize the quantity, extent, severity, and impact of software defects.

A code review is not intended to find solutions to problems, and there should not be any discussion of solutions in the code review. Time spent in a code review should be devoted strictly to finding as many problems as possible.

These code reviews will also be used as an opportunity to make sure that the development process documentation stays relevant to payment gateway development work.

Participants

Moderator: The moderator manages the code review, ensuring that discussions are kept on topic, time is used efficiently, and that issues discovered in the review are recorded.

Code Author(s): The code author is the person who made code changes.

Reviewer(s): Reviewers read through the required documents prior to the review and to come to the review with at least some prepared comments. They should not have been responsible for the code changes they review in any way.

Required Documents

Change Description

This is a brief description of the change and the reason for making the change. This should not be a long document, it simply provides context for understanding the changes. Optionally, any design documents that may be associated with the change can also be included for this purpose in addition to the brief description.

Static code style analysis results (CheckStyle)

This report is the output of the static java code analysis tool CheckStyle. It is generated against the entire code base of a project each time the project is compiled. The most recent copies of these reports should be provided as inputs to the review. This report should not have any errors listed.

Static code analysis results (PMD and FindBugs)

These reports are the output of the static java code analysis tools PMD and FindBugs. These reports should be generated against the entire code base of a project each time a project is compiled. The most recent copies of these reports should be provided as inputs to the review.

Compilation Log

These results of the latest compilation of the source code will be logged and provided as inputs to the review.

Unit Test Results

JUnit tests will also be run each time a project is compiled. A project is not ready for review unless it compiles and passes all JUnit tests. A report will be provided for the purpose of verifying that no JUnit tests have failed.

Subversion report of changes

This report serves as a guide as to which changes are reviewable. It should provided a detailed report of all changes since the last reviewed subversion revision.

Source code for changed items

This report should provide the source code for each of the files listed in the subversion report.

Procedure

Initiation: Once the Code Author has made changes and thoroughly reviewed and tested them, he prepares the required materials and submits them to the Moderator.

Entry Evaluation: Upon receiving the review materials, the Moderator ensures that all of the materials meet the criteria for performing a review. Specifically, the Moderator ensures that:

• A complete set of required documents has been submitted for the change.
• The subversion report of changes starts at the release where the previous review left off.
• There is a brief description of each change listed in the subversion report.
• The code compiled without errors.
• There are unit tests applicable to each change, and they have all passed.
• Source code is provided for each file that has changed.

Review Planning: Once a complete and correct set of materials is gathered, the moderator sets a time for the review, and distributes the materials together with this document to the Reviewers. Reviewers should be allotted enough time to adequately review the changes presented, typically a week.

Individual Review Preparation: Reviewers are expected to look over this document prior to each review, both to re-familiarize themselves with the review process and to make sure that this document stays relevant to the process.

The Reviewers then examine each code change for potential defects, referring to the Review Guidelines presented below.

The Reviewers are also expected to briefly review the following documents:

• Payment Gateway SDLC
• Payment Gateway Coding Standards

This documentation review serves to re-familiarize participants with payment gateway software development standards and to ensure that these documents stay relevant to current practices.

Group Review: The Reviewers meet to discuss their findings and arrive at a consensus regarding any issues to be addressed prior to considering the changes complete. The beginning of the meeting, up to a limit of 10 minutes, will be dedicated to any findings regarding development documentation. After this initial period, the code author will introduce the changed files one at a time, and the reviewer will respond with any issues he or she has discovered. Subsequent discussion will focus entirely on establishing an agreed upon list of defects. The Moderator will keep the discussion moving so that all materials for review receive adequate coverage.

Rework/follow-up: The Moderator enters the issues into a defect tracking system, and then addresses the issues as he or she sees fit. Once the rework is completed, the Moderator and/or the Reviewers verify that all action items are resolved, and closes the related tickets. If the Reviewers or the Moderator determine that an issue requires further review, the Moderator may call a follow up meeting at which documented issues and the methods being used to resolve them may be discussed.

Exit Evaluation: When the Code Author has completed his or her changes, they will notify the Moderator. The Moderator then verifies that all issues are resolved satisfactorily and closed.

Reporting the Results

The outputs of this review procedure include the following:

• The following input documents: Change Description, Static Analysis, Unit Test Analysis, and Subversion Report.
• A list of the issues generated from the review, with a description of both the issue itself and the final resolution.
  
  

Review Guidelines

Remember: “It’s about the code, not the people.”

Reference List of Review Points

Structure

• Does the code properly implement all elements of the design as specified?
• Are there any uncalled or unneeded procedures?
• Can any code be replaced by calls to external reusable components or library functions?
• Do processes occur in the correct sequence?
• Will requirements on execution time be met?
• Is the code well-structured, consistent in style, and consistently indented?
• Are there any blocks of repeated code that could be condensed into a single procedure?
• Is storage use efficient?
• Are symbolics used rather than "magic number" constants or string constants?
• Are there good reasons why any procedures having extended cyclomatic complexity greater than 15 should not be restructured or split into multiple routines?

Documentation

• Is the code clearly and adequately documented?
• Are there any inconsistencies between code and comments?
• Is the commenting style easy to maintain?

Variables

• Are all variables properly defined and given meaningful, consistent, and clear names?
• Do all assigned variables have proper type consistency or casting?
• Are loop index variables more meaningful than i and ii?
• Are there any redundant or unused variables?
• Are all array references in bounds?

Arithmetic Operations

• Does the code avoid comparing floating-point numbers for equality?
• Does the code systematically prevent rounding errors?
• Does the code avoid additions and subtractions on numbers with greatly different magnitudes?

Loops and Branches

• Are all loops, branches, and logic constructs complete, correct and properly nested?
• Are the most common cases tested first in IF-THEN-ELSEIF chains?
• Are all cases covered in an IF-THEN-ELSEIF or SWITCH block, including ELSE or DEFAULT clauses?
• Does the normal case follow the IF, not the ELSE?
• Does the end of each CASE statement have a break?
• Does every switch statement have a default?
• Are loop termination conditions obvious and invariably achievable?
• Are indexes or subscripts properly initialized, just prior to the loop?
• Can any statements that are enclosed within loops be placed outside the loops without computational effect?
• Are loops nested to three levels or less?
• Does the code in the loop avoid manipulating the index variable or using it upon termination of the loop?

Defensive Programming

• Are imported data and input arguments tested for validity and completeness?
• Are divisors tested for zero or noise?
• Are all output variables assigned?
• Are the correct data being operated on in each statement?
• Are finally blocks used to make sure all files and devices are left in the correct state upon program termination?
• Are there unrelated side-effects of a method call? Is there more than one return statement?

Example Code Review Meeting Agenda

The purpose of this meeting is to find and document project issues that need to be addressed. This should be the sole focus of a code review meeting. The issues will be entered into the issue database as they are discovered. This meeting will be targeted for 90 minutes.

• Process/Documentation Review
• Verify Compilation Report is clean
• Verify Automated Tests are successful
• Review SVN Report
  - Verify that code changes are appropriate and necessary
  - Verify that all issues identified as part of the build were addressed
• Review Check Style report
• Review PMD report
• Review FindBugs report
• Discuss threat modelling findings