Blog

In my previous blog Code Review Best Practices, we looked at DMC's general code review practices. These are important ideas to keep in mind, but even in doing so, code review can be a daunting task. DMC reviewers use the following process consisting of multiple reviews back and forth with the developer when reviewing a Merge Request (MR, in GitLab terminology) to make sure we stay efficient without missing anything.

Orientation: Clarify Basics

1. Ensure MR metadata is appropriately defined based on project convention (assignee, milestone, labels, etc.). If the project uses an MR template, make sure it’s filled out appropriately.
2. Understand the issue the MR is solving by reading or reviewing any linked work items. If the MR fully addresses the work items, make sure they’re closed by the MR merging (as appropriate for project workflow).
3. Confirm code passes any automatic checks defined in CI (build, automatic tests, formatting, etc.).
4. If code isn’t fully covered by automatic testing, clearly identify the testing approach and who performed it. If code is easy to test, test it yourself!
5. Read the MR's description to understand the high-level approach to the issue (if no description, ask reviewee to populate).
6. Confirm that the source and target branches are appropriate, and that MR sequencing is clearly defined or not needed (branch structure in the repository graph may reveal that this MR should merge before or after another one).

First Review: Refine Design

Note: this step may not be necessary for small MRs or ones you've been closely involved with already.

Glance it over

First, skim the files to understand the high-level scope of the changes.

1. Review structural elements (interfaces, header files, dependencies, etc.) to understand the relationship between modules.
2. If one or more new packages have been added, review to understand the functionality they add, confirm that a package addition is warranted, confirm that the license is appropriate for the project, and confirm that the package chosen is the best choice.
3. Get a sense of how complex these changes are to assess how long it will take you to fully review.
4. Consider taking a quick look at minor changes to check them off the list and focus future passes.

Take a pass

Go through the code changes and review them, focusing on items 1-3 (System, Structure, Function) from the list in the previous blog.

1. Review the full diff (don't look at individual commits unless you're answering specific questions).
2. If your structural review revealed new dependent elements, consider starting your review at the "bottom" of the dependency tree (a module without any dependencies, or only existing dependencies) to understand how functionality builds up. This may be challenging unless you’re quite familiar with the existing architecture.

Middle Review(s): Check Details

This will happen after the developer responds to your comments from the first review, or you can skip directly to this stage on your first review if there are no large comments on the design.

1. When the developer pushes the review back to you, first look at the comments included in your previous review to ensure all have been addressed (code changed or additional discussion).
2. It can be helpful to look at what has changed since the last review by filtering the commit list.
3. Take a full pass on the code, focusing on items 4-6 (Patterns, Style, Ready) from the list in the previous blog, and send a review.

Final Pass: Pre-merge

1. Make sure all pipelines are passing.
2. Make sure code will merge without conflicts.
3. Once the code looks good to you, Approve it! This means you’re happy with it (depending on the project, you might merge it or have the developer merge).

That wraps up our guidance for code reviews — we hope this is helpful in reviewing your next request.

Learn more about our Application Development expertise and contact us for your next project.