What is a good code review?
Tags: codequality, codenewbie, codereview
Published: 2020-11-14 13:46:43

I've put together a list of points like what is important I think in code review.

## Purpose of Code Review

- To ensure the quality of your product
  - People basically make mistakes
  - Two heads are better than one
- To share knowledge with the team
  - Sharing knowledge of the code with the team at all times avoids the problem of personalization, e.g., "Only A-san knows this feature, I don't know...".
  - It's a great opportunity where you can read other member's code and ask questions if any.
- To share responsibility within the team
  - When something goes wrong, only the person who wrote the relevant code should never be blamed.
  - It's the responsibility of the team not to notice the problem at the time of review (or until the code is deployed), so we should fix problems as a team.

## Reviewer edition

### Points to keep in mind when reviewing

- What should you review in PR?
- Does the code meet the Acceptance Criteria for the relevant Issue?
  - It's necessary for reviewers to check the behavior as well in your local if the code affects the UI.
- Is the code design OK?
  - Is it extensible?
    - Can you update the code easily when some specification changes?
    - If it's a disposable part, you would be able to refactor it later, but if it's a core part of the product (such as DB design), it's better to review the design carefully.
  - Are responsibilities for classes and methods properly separated? (Single Responsibility Principle)
    - Is the implementation easy to test?
      - Hard to write tests -> In many cases, the responsibilities of the class or method are wrong/bad.
      - The code may include too many dependencies or may try to do too many things.
      - If you see a method name like `a_and_b`, it obviously means to do more than one thing, and it can be often isolated the methods.
  - Is dependency OK?
    - Model depends on View, etc.
  - Is error handling well?
    - Doesn't the code return a server error when it's clearly a client error in the API.
    - Can you understand/solve the problem from the error information? (traceable?)
- Are the tests enough?
- Are there any bugs?
- Are existent features still working well? (Some feature is not broken?)
- Does deploy the code cause downtime?
  - If yes, is the deployment flow proposed properly?
- Are there any security risks?
- Does the coding style follow the rules within the team?
  - Basically, it's better to automate it like "check by Lint -> fail in CI if something is needed to fix it" instead of discussing it in a review.

### Mindset

- You should assume that you are going to maintain the code later on.
  - If you think, "This is going to be tough to maintain," you should do your best to improve the quality for your future self by review.
- Let's review it in terms of "how would you design it if you were to implement it?".
  - Comparing your design to theirs will help you enhance your design skills and would lead to more constructive discussions.
- If you feel some implementation isn't good, you should point out the code, not the person.
  - When pointing something out, let's always give reasons why you think it's not good.
    - If it turns out your point is incorrect, you should apologize honestly, and please appreciate the reviewee for discussing it.
- Let's ask questions about the parts you don't understand.
  - The reviewee can notice mistakes from the questions, and even if there are no problems, both can learn, and the reviewee can feel confident in their implementation.
- Let's answer questions from the reviewee sincerely.
  - Your review sometimes may confuse the reviewee due to the knowledge gap, in that case providing sample code or sharing links to articles may be helpful.
- If the discussion is going to be difficult on a PR (e.g. there are so many comments in a PR and discussion is still going on), it might be better to suggest having a meeting or pair programming or something like that.
- It's better not to push your preferences too far.
  - If it's a matter of preference, you can basically respect the reviewee's approach.
    - If you wanna share your preference, it's good to use tags like FYI, IMO, nits, etc to inform the point is not serious.
  - If it's about coding style, it might be a good idea to suggest if the team can reconsider the lint rules and coding conventions.
- Let's actively praise what you think is good
  - Reviews are often mainly about pointing out things, so if you think it's good and you learn something through the review, let's praise the reviewee's code and design.
  - This kind of communication often makes for a better team atmosphere (I guess).

## Reviewee edition

### Points to keep in mind when being reviewed.

- Do you specify what you want people to review in your PR?
  - Acceptance criteria
  - Is the PR scope clear?
    - What has been implemented in the PR?
    - What is not implemented in the PR?
      - If you are leaving a TODO, do you create a separate issue for it?
- Does the code meet the Acceptance Criteria for the relevant Issue?
  - Always self-review before requesting a review, so that the reviewer doesn't have to review so many points.
- Is the volume of the PR appropriate?
  - Huge PRs make it difficult for reviewers to notice problems when reviewing, so let's avoid them as much as possible.
  - Can we split up the PR and request a review?
    - For example, if the PR is large and includes a library update, it looks like it can be split into a PR for the library update and a PR to resolve the actual issue.
- Is the implementation simple?
  - Is the code you wrote still understandable to anyone, including you, a few months later?
- Did you leave supplementary comments in the code or PR if some implementation may not be able to understand your intentions in the code alone?

### Mindset

- You should be able to answer any questions about your implementation.
  - If you refer to something, share the reference link as well.
    - Official documentation is better than someone's blog post.
- Let's ask questions if you don't understand a reviewer's point.
  - let's follow the reviewer's points after you are convinced them (don't follow without understanding/considering)
- The reviewer's point of view is beneficial to you.
  - Pointing things out in a review is a great opportunity to learn how others think better, and you may notice your mistakes and it would lead to your growth.
  - You may sometimes feel disappointed when you are pointed out, let's be strong and overcome :)