How do I conduct a code review

How do I conduct a code review

At the beginning of the year, I started doing mentoring in ReactJS and NodeJS programs built by the company. The more code I reviewed from the project and mentee's home tasks the more I realized that I needed to review code in sequence and define a template to maximize the value. In this topic, I'm going to note down my review code structure and hopefully, it's helpful to you.

Ticket Understanding

First, I as a reviewer need to understand the purpose of the pull request(PR) or merge request(MR). If it's empty, I'll ask for an update and have a read on the ticket based on the branch name. By understanding the high-level requirement and scope, following with acceptances or definition of done(DoD). I'm in the same context as the author. It helps me review easily and explore more edge cases related to the ticket.

Check the PR/MR format

Second, I will check and verify my format to reduce the time I review, ideally about 1 hour. My good PR/MR should have an optimized size, about 200-400 lines of code (LoC), and should have a description in detail for example:

  • Branch name and PR/MR title included ticket number.
  • PR/MR filled description in detail, including checklist, and test steps, assigned to reviewer(s).
  • The author implemented both code changes and unit tests to reflect requirements.
  • The author tested locally and verified the PR/MR matched requirements.
  • Reviewer tested locally and verified the PR/MR matched requirements.
  • The PR passed the continuous integration pipeline (CI).

Implementation review

Third and the most important step is to review the implementation of the author. I will start in order: clean code, clean design, error handling, framework-specific, and unit tests.

Clean Code

In this section, I will check the code based on naming, functions, and common principles from easy to hard, from less to more time.

Naming (variables + functions + classes)

  • Good name should be short, descriptive, and searchable [ ex: isOpen, selectedUser ]
  • Classes, objects should be noun phrases. [ ex: User, Role, StorageService, userToUpdate ]
  • Methods should be verb phrases. [ ex: onPasswordChange, resetPassword, sendEmail ].
  • Should apply "the bigger scope, the shorter name and reverse". [ ex: updateUser method should have a bigger scope and shorter name than updateUserRole ]
  • Solution Domains should use computer science terms, design patterns design, and algorithm names. [ ex: hashMap, UserRepository ] 
  • Problem Domains should use non-technical words and be simple to understand ( even if it's the longer name ) [ ex: disabledTabOnUserRole ]

Functions

I split functions into separate sections because I come from the functional programming world ( one of the programming paradigms ). Based on my experience, functions should have

  • Should have a small size, ideally about 25 LoC.
  • Should do one thing ( one purpose, be a command or query, not both).
  • Should have one layer of abstraction.
  • Should throw errors on negative or un-handled cases.
  • Avoid side effects in the function.

Clean Design

In terms of good design. I usually follow best practices recommended by many developers like some common principles like DRY(Don't repeat yourself), YAGNI(You Ain't Gonna Need It) and SRP(Single Responsibility Principle) to make sure the function has good design. Even if changes contain structural modification or initiation, I will check if they can use any design pattern like Factory, Singleton,...etc to achieve to best result I can get.

Error Handling

Error handling is one of the most missing points in the code review process. I usually focus on 2 points, parameter validation, and default value. 

Each function's parameters should have a data type check(automatically by framework/library or manually ). it helps prevent values null or undefined, data type mismatch leads to unable to call methods or attributes.

Most outside functions should wrap in try-block, inside one should throw errors on specific errors.

Also, be careful with boolean logic, careless or weak logical thinking can lead to missing/wrong cases in boolean logic.

Framework Specific

Depending on the library you're using, there are comments and issues related to it like adding dependencies to ReactJS's hooks or remembering to un-subscribe observable objects in Angular to avoid memory leaks. I don't have any specific comments but I believe you know what to do.

Unit tests

Even though I note down unit tests I am honestly not an expert, But make sure that you've run unit tests and have coverage about 80% or at least 70% if it's urgent. Try your best to cover all cases your code implemented. 

What a long blog right, this one took me like a week or something, my ass is on fire on the current project. If you've read this line, Thank you very much. I'll be back in the next blog. In case you have anything to discuss, to build, and to deliver. why don't have a chat right? Have a good day and drink enough water. #tuanhuydev