Elevating Software Quality: A Guide to Effective Code Reviews

Code reviews are crucial in the software development process for enhancing software quality, facilitating knowledge sharing, and reducing maintenance costs. This article provides a set of comprehensive guidelines and best practices for code reviews that software engineers can introduce in their organizations. These guidelines are particularly well suited for teams or organizations maintaining end-to-end ownership of development, testing, and CI/CD (Continuous Integration/Continuous Development) pipelines.

Importance of code reviews

Ideally, every change released in your pipeline should be reviewed. No matter if it’s trivial, any change not reviewed should not be released to production. If it is a small and obvious change, then it will be easy to submit for review and approval. Without a review process, you could miss the context of the impact or problems a seemingly small change can cause for your service. This is especially true if a software engineer is new to the organization. There are no scenarios where code reviews should be optional before releasing change into production.

Code Review Guidelines

Submitter Responsibility

• Break change into multiple steps/code reviews - Strive to break the change into multiple code reviews or send incremental code reviews instead of a single big code review. This helps to reduce code review time. For example, if it would take >2-3 days to make the code change, consider dividing the task into multiple sub-tasks & sending separate code reviews for each sub-task. Do not try to solve multiple problems in the same code review. As a general rule of thumb, your code review should not exceed more than 1 page in the code review tool.

  
  

• Communicate strategy - If you are working on a complicated change, summarize the strategy in SIM/Wiki & present it to the team or 1-2 potential reviewers in the team before actually writing the code. This would help to get early feedback on strategy & reduce code review time/iterations.

  
  

• Local builds or dry-run builds for your environment should pass before the change is merged into your pipeline.

  
  

• Integration tests should pass locally - Integration tests related to your code change should pass locally. If core functionality is changed for the package, make sure to run all integration tests. For new functionality, new integration tests should be added.

  
  

• Write clear git commit message - Blog post summarizes 7 rules of great git commit message, highly recommended to internalize them.

  • Here’s a quick summary:

    • Separate subject from body with a blank line

    • Limit the subject line to 50 characters

    • Capitalize the subject line

    • Do not end the subject line with a period

    • Use the imperative mood in the subject line

    • Wrap the body at 72 characters

    • Use the body to explain what and why vs. how

      
      

• Every change should have an associated tracking ticket - Add a tracking ticket link for the ticketing system you are using in your organization. Include motivation, background & investigation details in the ticket. If it is a simple change, the ticket would have a single line. But having such reference helps backtrace background in the future.

  
  

• The code review description should include relevant details - Make sure the ticket link from the commit message is included in the CR description. Ticket or review description should cover testing results (Before/After output to show that you change works as intended) as well.

  
  

• Code review/commit description should match changes - Do not slip in non-relevant changes in commits or code reviews.

  
  

• Do a self-review before publishing the code review or an update. This will help reduce the number of iterations on code review.

  
  

• Add 2 reviewers & team in all code reviews - As a general rule of thumb, it is recommended to have a 2 approvals policy for all code reviews. It would be preferable to have one of 2 reviewers be well aware of the component/code path. A team should be added for awareness of changes & anyone who has time will be able to look at/comment on the code review. If any of the reviewers or submitters added more than 2 required reviewers, please make sure all required reviewers have approved the code review before merging.

  
  

• Distribute code reviews - Try to distribute your code reviewers across the team. This will help to make sure the same engineers are not getting all code reviews, help distribute knowledge across the team & submitter will get feedback from different engineers.

  
  

• Do not send code reviews until all the above are done.

  
  

• Address all comments before pushing change - Even if you receive 2 ship-its & there are unresolved/open comments, make sure to address those before committing your change.

  
  

• Expect to receive code review feedback after the change is committed - Make sure to review feedback & comments are addressed in follow-up code review if needed.

  
  

• Rebase - If you need to do a major rebase with merge conflicts, you must get approvals again. It should be quick as the change was already reviewed.

  
  

• Be accountable - Make sure the CI/CD pipeline is green after you commit the change & your change does not break the pipeline.

Reviewer Responsibility

• Understand the change - As a reviewer, you should have a very good understanding of the problem, change & code path. If you need help in understanding be sure to take the time or discuss with the submitter/engineers in the team.

  
  

• Time - Make sure to give feedback in a defined time, which can be agreed upon within your team or org. If you don't have the bandwidth, communicate that, so the submitter can identify an alternative reviewer.

  
  

• Check code change for:

  • Context - Make sure to check the change for the context. Is this the right change? Is this the right package/module?

  • Description - Make sure the code review/commit description matches the actual change.

  • Functionality - Make sure the change is functionally correct. Handles error cases & correct error codes are defined where applicable.

  • Assumptions - Make sure the change has correct assumptions & works in all conditions.

  • Maintainability & readability - Make sure the code is easy to understand, does not have repetition, has proper naming & is readable.

  • Performance & efficiency - Make sure code uses efficient data structures & algorithms. Consider CPU & memory utilizations are not made worse.

  • Security - Make sure new security holes are not introduced with the change.

    
    

• Check unit testing coverage - Make sure the change comes with good testing coverage (if the code is unit testable & it's easy to add unit tests). Testing covers both success & error cases.

  
  

• Check integration tests - Make sure existing integration tests cover the code path updated in change & if it does not, new integration tests are added.

  
  

• Check git commit message recommendations are followed.

  
  

• Be Accountable - Consider yourself accountable for the code change. You “ship-it”; you own it. Don't ship the code you don't fully understand or you don't feel confident about.

  
  

• Be cooperative - In the scenario where the submitter & reviewer do not agree on the code change, involve other engineer(s) in the team to get suggestions & unblock progress.

Conclusion

This article presents a set of generic code review guidelines for software engineering organizations to set up best practices for the software development process.