Issues and Criteria When Reviewing Code

Tram Ho

The Goal of Code Review

Code Review is the process of evaluating code written by developers to ensure the code meets quality and design standards. Purpose of code review:

  • Ensure products of the highest quality.
  • Ensure the quality of the codebase improves over time.
  • Self growth
    • Soft Skills:
      • Reviewers and reviewers are trained to present and explain ideas in a way that others can easily understand
      • Learn to listen
    • Hard Skills:
      • Learn, share techniques, professional experience

Code Review Points

Functionality

  • Check if the processing logic is correct?
    • If it is a multi-step process, make sure the number of steps and the order of steps are optimal
    • Correct status update
  • Think like a user to find enough corner cases, edge cases
  • Watch out for concurrency issues (deadlock, race conditions, …)

Thiết kế

  • Do code changes change the architecture of the code base?
  • Will the code changes integrate with the rest of the system?
  • Is this the time to add this feature?
  • Are code changes extensible?

Readability

  • Naming: an understandable variable name is a variable name long enough to represent what it is and what it does, but not too long to read.
  • Style: should agree on a coding style
  • Comments: should be concise, clear and mainly explain why (why) instead of what (what)
  • Complexity: When a developer writes code that is more general than necessary, or adds a feature that the system does not currently need. As such, it increases the complexity of the system.
  • Duplication: does the logic of the newly added code match any previous function?

Consistency

Consistency is very important, especially in technical work. Consistency in the codebase makes code easier to read, avoids unnecessary bugs, makes porting easier, and facilitates more convenient teamwork.

If the team has a clear consensus on the style guide, convention, and codebase, the developer should follow to create consistency.

Testing

Testing is very important. The test is like going to a regular health check-up. If your health has problems, your doctor will make recommendations and adjustments to improve your health. Testing helps the software system run properly and develop sustainably.

We need to provide full unit test, integration test, end-to-end test.

Documentation

Changes to build, test, interactivity, critical processing logic, or release code all need to be documented accordingly.

Good Things

Code review is not only focused on finding bugs, but we need to celebrate and disseminate good practices to other developers. Sometimes it’s more valuable to praise developers for doing something good than to show them what they’re not doing well.

Code Review Process

  1. Reviewee (developer) done feature or fixed bugs
  2. Reviewee creates a merge request with a branch name in the format feature/create-user, feature/FC-4201, fix/update-user
  3. Reviewee ensures successful code compilation, unit tests, and automation tests
  4. Reviewee notifies reviewers to schedule code reviews
  5. The reviewer makes a checklist of the tasks that need to be reviewed according to the above criteria and the necessary criteria for features and bugs at that time.
  6. Reviewee evaluates and implements according to the consensus of both parties
  7. Reviewer reviews and approves the merge request

Attitude While Reviewing

  • Polite
  • Contribute
  • Learn and educate without criticizing or disparaging

How to Write Code Review Comments? (Write a Comment How?)

  • Be polite and kind. Avoid disparaging comments that make reviewers angry, leading to disagreements, difficulty in absorbing and listening from both sides, even causing internal conflicts.
    • Don’t: “Why are you using threads here when it clearly has no performance benefit”
    • Do: “Concurrency here increases the complexity of the system. Because it does not help increase the performance of the system. Instead of using multiple threads, you can use single thread for simplicity.”
  • Explain why you made that opinion. It will help reviewee better understand and learn more knowledge and experience.
  • Offering solutions. There is no need to give a specific, detailed solution because the reviewee is the person who implements the feature or fixes the bug. They have a chance to practice and can come up with a better solution
  • Label the severity of the comment:
    • TODO (Critical): needs to be handled immediately because code changes affect the system’s codebase, security, …
    • NIT (Nitpick): secondary issue. You should take care of that but it shouldn’t have a big impact.
    • OPTIONAL : “I think this way is better but it is not mandatory”
    • FYI (For Your Information): “I want you to take care of this at this point. You can think about how to handle it later.”

Resolving Conflicts

  1. Listen. Both sides need to listen and understand the opinions of both sides
  2. Analyze the advantages and disadvantages of the solution of the two sides and other solutions
  3. Analyze the context, requirements, find out the order of priority of the properties of the system (especially interested in the scalability and security of the system)
  4. Choose the solution that meets the above requirements
  5. If you still can’t find a solution, you should consult with:
    • The whole team
    • Technical Lead
    • Head of Engineer

Best Practices

  • Use information, technical knowledge, and data to eliminate biased opinions and opinions . Explaining by including relevant principles will be more convincing . For example: SOLID, KISS, …
  • Less is more . When we focus on a merge request for too long, efficiency will decrease over time and can lead to more bugs. Small Commit → Short Review → Better Code . So how many are small commits? According to Google dev data: ~250 lines of code / CR
  • Checklist . To avoid mistakes when reviewing, we need a checklist. At the beginning, the checklist may be a little tedious, but it will be added over time. For each feature, there may be different things to check.
  • Leverage Tools . “A good developer is a lazy developer”. Use the Static Code Analysis tools before reviewing, such as SonarQube, …
  • Speeding up code review helps to reduce the heavy atmosphere of the review, limiting complaints from developers. In addition, it saves both reviewer + reviewee’s time

References

If you find it interesting, please invite your friends to the group. What system do you need to design, you can comment for everyone’s reference.

Thank you guys

The group is in need of a writer and graphic designer, if you are interested, please comment below. When you join, you have the following benefits

  • Improve writing skills, professional skills
  • Sharing research materials
  • Connect, learn from the brothers in the group

Facebook: https://fb.com/groups/systemdesign.vn

Discord: https://discord.gg/SyekpYxdzz

Share the news now

Source : Viblo