Code Review Best Practices

Reviewing code is a key part of DMC's development process. It gives us a key quality check before we integrate code updates, allowing us to catch many errors and ensure we're creating maintainable code. In doing so, it also acts as a mechanism for senior developers to spread their knowledge to the rest of the team.

In this blog, we focus on general best practices for text-based code reviews. DMC also uses a number of graphical programming languages, which have unique challenges in the code review process — some of these ideas apply, but not all. In a follow-up blog, we'll look more closely at some specific procedures DMC follows.

Code review is always a little bit different from project to project, but our experienced code reviewers have collected a set of practices that improve the efficiency and efficacy of our process. Whether you're getting ready to review your first line of code or merge your thousandth request, this blog is for you!

Focus on Value

While reviewing, stay focused on the value added by the code review process. The remainder of this guide includes lots of specific recommendations, but not all recommendations should be followed on every review. Focus on using the review as an opportunity to confirm functionality and ensure maintainability, not to debate an optimal solution. Make sure that time spent on reviews balances the quality and efficiency expectations of the project — a quick internal tool shouldn't be reviewed as carefully as a business-critical data processer. 

Use your Tools

All major source control platforms include features designed to facilitate the code review process, so learning and employing those tools can give you a big boost. DMC uses GitLab as our preferred platform for source control, so the following discussions reference some specific features of GitLab, but most source control platforms have analogous capabilities. We particularly recommend the following features in GitLab:

• Whenever leaving comments, use the review feature.
• Change your settings as needed to facilitate reading changes easily.
• Mark files as viewed to avoid duplicate review time.

Prioritize it

Reviewing code early and often is important and worth prioritizing. Delayed code reviews can leave developers waiting or cause them to forget why they made technical decisions. If appropriate, move larger comments and reworks into follow-up issues to merge the primary MR more quickly. 

Timing matters

Before spending time reviewing code, determine whether a synchronous or asynchronous review will be more valuable. Asynchronous reviews use GitLab comments to allow reviewers and developers to alternate their focus on the review and are most common at DMC. A synchronous review (where the reviewer and reviewee meet) allows for richer interactions but is sometimes less time efficient. It’s often preferred in situations that include new engineers, complex changes, or engineers where a verbal discussion has low overhead. A hybrid option (an async review with a follow-up meeting) is also common in the case that there are outstanding questions, or the reviewer wants to provide more context. You can do this differently for different review passes (a synchronous first review with async follow-up is common).

Look high and low

There's a lot to think about when reviewing code! We use the following list to organize our reviews and make sure we don't miss anything.

1. System: Are any changes to the system design reflected in the code handled appropriately? Examples:
   1. Will the design changes have any unintended consequences on other system elements?
   2. Do any of the changes warrant updates to documentation in the repo (like a README) or project-level documentation?
2. Structure: Does the code have good structure? Examples:
   1. Is functionality broken up into understandable modules?
   2. Is code repetition minimized?
   3. Can code intent be clearly understood?
   4. Could the same functionality be achieved with less complexity?
3. Function: Does the code work correctly? Examples:
   1. Will the code behave as expected even in corner cases?
   2. Are errors and alarms handled appropriately?
   3. Will the changes cause any regression in existing functionality?
4. Patterns: Are project patterns followed well? Examples:
   1. Are comments useful (provide information beyond what’s in the code) and clear?
   2. Are log levels appropriate?
5. Style: Does code follow language style guidelines?
6. Ready: Is the code ready for the branch it's being merged into? Examples:
   1. If merging into a main or production branch, is any test code removed?
   2. Are TODOs appropriately captured in follow-up issues?

Talk about it

When you find something while looking for the above items, discuss it with the reviewee. For async reviews, this should take the form of a GitLab comment; for synchronous reviews, make sure that suggested changes are noted (in a comment or elsewhere).

• If what you find violates clear-cut best practices, provide directive feedback to change it (and reference the source). Example: "Microsoft recommends PascalCase for class names. Please change this variable to match."
• If you don't understand why a piece of code is written the way that it is, ask! You can instead spend time digging in, but, if it’s not clear to you, it could likely be written more clearly regardless. Example: "Can you talk me through why we need separate methods here?" Once you understand the code, recommend restructuring, renaming, or adding comments as appropriate to make it easier to understand.
• If multiple solutions may be appropriate and worth consideration, enumerate them to start a discussion. Example: "I see that you structured this as an interface, but an abstract class would also work. Why did you prefer an interface?"

That's all for our recommended practices! If you'd like to learn more, check out our companion on the process: Code Review Procedures.

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