\nWell, here's a process that has proven effective for us when conducting code reviews. You can find other articles on the topic of code review here. here (2015)<\/a> and here (2017)<\/a>.<\/p>\n
Why?<\/h2>\n
During the course of a project, it was discovered that a module had already been reviewed. A corresponding note in a table maintained specifically for this purpose had been accidentally changed \u2013 with the effect that the module was reviewed again (by project members who joined later and who, of course, had no way of knowing this). This inevitably happens when the status of an item is tracked separately from the item itself.
\nOn top of that, manually listing modules with findings or modules yet to be reviewed is error-prone and time-consuming (and a boring task to boot). There has to be a better way to do this without resorting to potentially expensive tools!<\/p>\n
Where?<\/h2>\n
The goals of the new process were quickly defined:<\/p>\n
- \n
- \u201cSingle-source\u201d \u2013 ie no separate documents detached from the code<\/li>\n
- Easy tracking of the review status of source modules<\/li>\n
- Traceability of reviews<\/li>\n
- As few additional tools as possible<\/li>\n<\/ul>\n
With what<\/h2>\n
At MEDtech, we conduct code reviews using Doxygen comments directly in the source code. GitLab supports branching\/merging during reviews and traceability, as well as the development itself. This means that Git workflows and Doxygen are already established in the projects.
\nLuckily, Doxygen generates custom lists, e.g., in HTML format, using the "@xrefitem" tag. This allows us to generate lists of modules with their respective review statuses (see below) from the source code. These can be copied into the milestone review document, for example (although this step could also be automated \ud83d\ude09).<\/p>\nPreparation<\/h2>\n
To avoid typos and typing in @xrefitem comments, the following aliases are defined in the Doxygen configuration file:
\n
\nALIASES += "review_done=\\xrefitem reviews_done \\"Code review done\\" \\"Code Review: Modules done\\""
\nALIASES += "review_missing=\\xrefitem reviews_missing \\"Code review missing\\" \\"Code Review: Modules with without reviews\\""
\nALIASES += "review_finding=\\xrefitem review_findings \\"Code review finding\\" \\"Code Review: Modules with findings\\""
\n<\/code>
\nThe tags @review_missing, @review_finding, and @review_done are used like other Doxygen tags, but should be placed in the file header comment. The text after the tag will then appear in the generated document (e.g., HTML) below the file name:<\/p>\n![\"\"](\"https:\/\/medtech-ingenieur.de\/wp-content\/uploads\/2022\/06\/Bild1.png\")
<\/p>\nWith this configuration, Doxygen creates entries in the "Code review: Modules done," "Code review: Modules missing," and "Code review: Modules with findings" lists. Ideally, this should be possible at the push of a button (think of the project management) or at least be done in the CI pipeline.
\nAs a special treat, the management receives a KPI in the simplest way and the development team a merit badge \ud83d\ude09<\/p>\n![\"\"](\"https:\/\/medtech-ingenieur.de\/wp-content\/uploads\/2022\/06\/Bild2.png\")
<\/p>\n![\"\"](\"https:\/\/medtech-ingenieur.de\/wp-content\/uploads\/2022\/06\/Bild3.png\")
<\/p>\n![\"\"](\"https:\/\/medtech-ingenieur.de\/wp-content\/uploads\/2022\/06\/Bild4.png\")
<\/p>\nSo the goal has been defined \u2013 now for the path to get there.<\/p>\n
The process<\/h2>\n
In the following, we'll run through a code review for a fictitious software module (as shown in the graphic below). The review item (=code) will go through various states during the process, which change based on the actions of the people involved, the "developer" and "reviewer." The states are:<\/p>\n
- \n
- review missing<\/li>\n
- review findings<\/li>\n
- review done<\/li>\n<\/ul>\n
start<\/h3>\n
One thing in advance:<\/b> We have created a graphic that visualizes the following steps. You can find the graphic at the end of the blog post<\/a>.
\nThe code review process begins with the creation or revision of our software module in the status \u201creview missing\u201d. The developer creates a Review ticket<\/b> (hereinafter referred to as Issue<\/b> (referred to as) in GitLab and assigns it to the reviewer. This also creates a separate Git branch including merge request<\/b> created (alternatively, the review can also be done directly on the feature development branch). The merge request should use the WiP ("work in progress") status to prevent anyone from accidentally closing the merge request. In GitLab, you can do all this with one click ("Create branch and merge request") within the respective issue.<\/p>\nReview<\/h3>\n
The reviewer now begins their work by inserting the following template into the code header (a reduced template example is provided here for clarity). It contains the status alias along with the last committer's initials, the commit date, and finally the commit hash.
\nNote:<\/b> Since Doxygen removes line breaks within a comment block, manual line breaks "@n" are required for better readability in the output format.
\n
\n\/**
\n* @file exampleUnit.c
\n* @brief example for code review demonstration
\n*
\n* @review_finding <REVIEWER>, <DATE>, <HASH>
\n* @n 1. Completeness of implementation
\n* @n 2. General code quality
\n* @n - 2.1 Null pointer check:
\n* @n - 2.2 No infinite loops:
\n* @n - 2.3 No open Todos in code:
\n<\/code>
\nThe relevant findings are documented in the comment structure, e.g.:
\n
\n* @review_findings RVW, May 3, 2022, <HASH>
\n* @n 1. Completeness of implementation: ok
\n* @n 2. General code quality
\n* @n - 2.1 Null pointer check:
\n* @n RVW finding: Missing Check in foo()
\n* @n - 2.2 No infinite loops: ok
\n* @n - 2.3 No open Todos in code: ok<\/b>
\n<\/code>
\nNote:<\/b> Changes to the review tag are highlighted in bold.
\nIn the unlikely event that no findings are found during the review, the reviewer changes the status to "review done" (but we're not finished yet). Usually, however, there are findings after the first review\u2014as in this case. This causes the reviewer to change the status to "review finding." They then commit the changes and assign the issue to the developer.<\/p>\nediting<\/h3>\n
The developer processes the findings. He can create a finding...<\/p>\n
- \n
- \u2026 reject (reject<\/b>): In this case, the developer and reviewer disagree. It should be discussed whether a fix is needed or not.<\/li>\n
- \u2026 repair (fix<\/b>): the developer agrees with the reviewer that this should be reworked and fixes the finding.<\/li>\n
- \u2026 accept (accept<\/b>): the developer agrees with the reviewer that rework is needed, but cannot correct the finding (e.g. due to time constraints).<\/li>\n<\/ul>\n
The actions are recorded in the review tag:
\n
\n* @review_findings RVW, May 3, 2022, <HASH>
\n* @n 1. Completeness of implementation: ok
\n* @n 2. General code quality
\n* @review_findings RVW, May 3, 2022, <HASH>
\n* @n - 2.1 Null pointer check:
\n* @n RVW finding: Missing Check in foo()
\n* @n DVP fix: used semaphore for function foo().<\/b>
\n* @n - 2.2 No infinite loops: ok
\n* @n - 2.3 No open Todos in code: ok
\n<\/code>
\nThe whole thing is then checked back in. The developer now reassigns the issue to the reviewer.
\nA notice:<\/b> The review status will not be changed when the developer makes the revision!<\/p>\nRe-Review<\/h3>\n
The fixes and rejects are reviewed in this review step and commented on accordingly. If the reviewer is satisfied with the changes and\/or the developer's reasons for the rejections, they confirm this with a simple "ok" in the review tag. This comment change is checked back in \u2013 because to complete the review in the next step, we need the corresponding revision number (=hash) for traceability. Finally, the release document should reference the version containing the complete review comment.
\n
\n* @review_findings RVW, May 3, 2022, <HASH>
\n* @n 1. Completeness of implementation: ok
\n* @n 2. General code quality
\n* @n - 2.1 Null pointer check:
\n* @n RVW finding: Missing Check in foo()
\n* @n DVP 2022-06-03: fixed: used semaphore for function foo().
\n* @n RVW 2022-06-04: confirmed<\/b>
\n* @n - 2.2 No infinite loops: ok
\n* @n - 2.3 No open Todos in code: ok
\n<\/code>
\nIf there are still doubts about the implementation or the developer's decision, another round of revisions will be carried out.<\/p>\nDiploma<\/h3>\n
Let's assume that the reviewer and developer agree and the review can be completed. In our example, we finally change the review status to "@review_done," enter the Git hash of the previous commit, and clean up the now-superfluous finding comments:
\n
\n\/**
\n* @file exampleUnit.c
\n* @brief example for code review demonstration
\n*
\n* @review_done RVW, May 3, 2022, 002e9d43bde307687aee6bd8bc3075838645290e
\n*\/
\n<\/code>
\nIf findings persist, the status \u201c@review_finding\u201d will not change even after the review is completed.
\nAfter the commit, the reviewer removes the WiP status of the MR (=changes the MR title) and assigns the merge request to the developer who will perform the merge. Clicking the Merge button within the merge request in GitLab automatically closes the associated issue and the merge request itself after the associated branch has been successfully merged into the master branch. This concludes the code review of our module.<\/p>\n![\"\"](\"https:\/\/medtech-ingenieur.de\/wp-content\/uploads\/2022\/05\/CodeReviewProcess-206x300.png\")
<\/a>Code review process (click to enlarge)<\/figcaption><\/figure>\n Summary<\/h2>\n
In this blog post, we've shown that comprehensible code reviews are feasible and practical with the initial goals and the tools GitLab and doxygen. Of course, we're interested in your thoughts or\u2014even better\u2014whether you've established "simpler" processes yourself, because, as we all know, more complexity is always possible, and brevity is the soul of wit.<\/p>\n
- \u2026 repair (fix<\/b>): the developer agrees with the reviewer that this should be reworked and fixes the finding.<\/li>\n
- \u2026 reject (reject<\/b>): In this case, the developer and reviewer disagree. It should be discussed whether a fix is needed or not.<\/li>\n