Skip to main content

Specific Code Review Guideline for Control (Git)

This code review guideline is written for those in the Control organization working with Git and includes the functional safety related aspects to adhere to. The guideline is directed to both source code level as well as feature level.

For legacy PCP products not using Git, please refer to 3BSE052207 Review Process Guideline for Code Review Guideline

The code must be reviewed according to 3BSE067603, Branch Strategy in Team Foundation.

The review shall be carried out on two levels:

  • Source code level for individual commits via pull requests.
  • Feature level for functionality and design, see Definition of Done (DoD).

Before the code is reviewed, it needs to be ready for review. The code responsible shall follow the applicable points in 3BSE028024 Checklist for Reviews. as well as the code guidelines, see Secure Coding Guideline.

For safety-related development, the following is an add-on:

  1. 3BSE038655 Static Code Analysis Guideline
  2. 3BSE000831 C++ Style Guide
  3. 3BSE023463 C Style Guide
  4. 3BSE042738 C# Style Guide
  5. 3BSE070754 Design Methodology and VHDL-syntax for FPGA design
  6. 3BSE027091 Library Object Guideline
  7. 3BSE057386 Crib – Guideline to Source Code Review Checklist
  8. 3BSE087000 Guideline Auto SVT code review
note

Note that the above mentioned guidelines and checklists, except for Secure Coding Guidelines, are mainly written from the brownfield point of view and are to be updated and integrated.

Source code review​

For source code, the code shall be incrementally reviewed (per commit) with a review team. The review team shall consist of one or more persons who have sufficient knowledge of the code, for example, team members. The code review shall include the applicable points in the 3BSE028024 Checklist for Reviews.

The code shall be reviewed either by formal inspection or walkthrough. The differences between them as defined by 3BSE039242 ABB approach to IEC61508 2nd Ed, Design are:

  • During an inspection, the inspector plays an active, leading role (asking questions). The author/code responsible is passive (giving answers).
  • During a walkthrough session, the author/code responsible plays an active role (explaining the design).

This is interpreted as follows:

  • A formal inspection on the source code level is carried out by creating a pull request to the commit and adding the applicable reviewers (inspectors). The reviewers add their comments to the pull request and the author/code responsible answers and resolves them.
  • A walkthrough requires a meeting where the author/code responsible explains their design. As for formal inspection, any additional comments and/or MoM are added to the pull request and the author/code responsible answers and resolves the comments.

All comments are documented and resolved within the pull request before the code can be merged into a main/integration branch. The code responsible/author is not allowed to approve their own changes.

  • If the reviewer assesses an issue within the commit to be major, they should set “Waiting for author” in the pull request, to hinder the merge before the issue is resolved.
  • If the reviewer assesses an issue within the commit to be minor (spelling error, naming conventions etc.), they should set “Approved with suggestions” in the pull request.

If a merge conflict were to occur, the changes performed to solve the conflict require a new review with the review team before the merge conflict is set to "Resolved".

A review meeting is not mandatory, but recommended for:

  • Conflicting comments
  • Major principal discussions
  • When the Walkthrough method is used for review

Feature review​

For the feature review, the reviewers shall be invited according to 3BSE039313 Names on RACI. A code review record shall be created in which comments are logged and resolved. This could be as a review WI in CMMI or by attaching a RR document to the Feature WI under review. The accountable person is set as the assigned person to the WI/RR and will set the WI/RR to Resolved when ready. The feature review shall include the applicable points in 3BSE028024 Checklist for Reviews.

The code quality shall be assessed for the feature and any possible updates and change-checks are decided. The feature review includes reviewing any interfaces/dependencies to other features.

The feature review is conducted by:

  • Checking that the Definition of Done (DoD) for a feature is fulfilled
  • Checking that the intent of the feature has been fulfilled, i.e. that the functionality has been implemented and nothing has been forgotten
  • Checking dependencies to other features
Owner: Software Development Team