What is a good code review?

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 :)