Contact us

Code Review Best Practice

Code review overview

What is the review code

  • The process by which programmers review and evaluate another team member’s code
  • At the same time, it is also the process of discussing and giving suggestions to help improve the quality of the code.

Benefits of code review

Personally:

  • Improve your level.
  • Learn how to communicate and give suggestions to others, as well as verify your knowledge.

About work:

  • Capture what happens in the project
  • It is an important step to reduce errors
  • Better code quality => better project quality.

Common standard of code review

Shared:

  • Ensure that the state of the entire codebase improves over time
  • There is no “perfect” code, only “better” code
  • Technical facts > personal preferences

Individual:

  • Need to be able to learn and improve for each of their PRs
  • Need to be responsible for the PR that I have reviewed
  • Avoid repeating the same mistakes

=> Reviewer should approve PR when it definitely improves the situation of codebase, even if PR is not perfect.

Review code like

When reviewing code what do we look at?

Common Mistake

  • Formatting: Where is the space or break line, use tab or space, …
  • Style: Where to declare variables, how to pass params?

When reviewing the code, we should not pay attention to the above two issues, because basically the tools support us to do that, or take advantage of them. Instead, I think we should focus on the following

  1. Design
  2. Readability & Maintainability
  3. Functionality
  4. Tests
  5. Performance
  6. Security
  7. Document

1. Design

  • Are the Change Lines suitable for the current architecture of the codebase?
  • If the codebase has different standards or styles, is the style of the Change Lines appropriate?
  • Do Change Lines follow the appropriate design patterns ?
  • Change Lines follow the fundamentals like
    • SOLID
    • DDD (Domain Driven Design)
  • Are the Change Lines in place, e.g. Regarding Orders, are they in Order Services?
  • Can Change Lines reuse some old functions? -Are Change Lines over-engineered?

2. Readability & Maintainability

  • Is naming (of fields, variables, parameters, methods and classes) easy to understand and properly represent its function?
  • When reading the code, Unit Test do you understand what it is doing?
  • Error message represents correct or not?
  • Comments are properly written?
  • Docs have been updated?

3. Functionality

  • Will the Change Lines work as the developer wants or not?
  • Are there Unit tests that guarantee this?
  • Is there potential for errors such as, for example, using “and” instead of “or”?

4. Tests

  • Is there UT (Unit Test) for new/corrected code?
  • Is there a UT for complex logic segments?
  • Are you understanding what UT represents?
  • Do the UTs match the requirements?
  • Did the UTs miss any important cases?

5. Performance

  • Do Change Lines negatively affect current performance?
  • Pay attention to issues that can affect performance
    • Call to another service, timeout
    • Handling with database
      • N+1 queries
      • Deadlock, …
    • Thread related issues, memory leaks, …

6. Security

  • Use tools to check for basic security errors (if possible). Pay attention to raw queries
    • SQL Injection
    • Cross-site Scripting
    • …
  • Check if the data needs to be encrypted
  • Check if there are ** exposed keys**

=> Usually frameworks already avoid basic security issues.

7. Document

A well-written guide can also help onboard new team members and prevent misunderstandings. Investing the time to create a comprehensive document guide can save time and money in the long run.

How to do better code review?

  • Steps of the review process **
  1. Overview 1 turn of CLs (Change Lines)
  2. Double-check the main parts of the Change Lines
  3. Check the rest of the Change Lines in logical order

1. One-shot overview of Change Lines

  • First need to understand the spec of the CLs, and confirm with the dev, do the CLs match the spec? => If not, you can reject immediately.
  • If this problem happens too often => need to review the team’s working process.

2. Double check the main parts of Change Lines

  • Find “main” files of Change Lines, usually those are the files with the largest amount of logical changes.
  • Please review this “main” part carefully first => provide context for all other parts of the Change Lines
  • If the Change Lines are too big, maybe the dev should split the Change Lines into small parts
  • If you see design-related problems => need to report back to the dev immediately.

3. Check the rest of the Change Lines in logical order

  • Try to find a logical sequence to reivew through the Change Lines while making sure not to miss any Change Lines.
  • Usually, when we have reviewed the “main” Change Line, we just need to review the files in the order that the review tool suggests.
  • Sometimes it’s also helpful if you read the UT before reading through the main Change Lines ⇒ to help you get an idea of what changes are supposed to be in the works

Notes when commenting on PR

  • Respect each other, give constructive, gentle comments.
  • Explain why? **(Can) make other suggestions if disagree with developer’s solution
  • Consider pointing out the problem with giving direct instructions. Because besides the main goal is to make the best Change Lines, the other goal is to improve the developer’s capacity.

NOTE: NEED TO REVIEW ONE CODE

Notes to improve code review speed

Summary

The above article has a few points that I would like to summarize as follows:

  • PR improvement of codebase is acceptable
  • Automate as much as possible
  • Review code with respect, learn from each other

Please look forward to the next part. If you have any questions or suggestions, please leave a comment below. Thank you for reading this article.

Source: Nguyen Thanh Tien – Senior Software Engineer

Popular