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

Comments

[Marcelo Bukowski de Farias]

Well written! There's one thing I disagree: Why wait until a PR is submitted to perform a code review? I find myself asking for feedback and performing reviews myself even before a commit is made, and it leads to much less friction. Besides, it is easier to correct the course when you do it earlier.

What are your thoughts?

[Hideaki Ishii]

Thank you for your opinion, I agree with you!

For example:

• Discussing architecture in the team before developing might be helpful to reduce later review communication.
• Asking a colleague while developing (before submitting a PR) might be helpful.

And I actually often adopt such approaches.

BTW, I didn't intend to write that we have to wait for review until submitting a PR
If there are unclear descriptions that cause misunderstanding in the article, please point them out, I want to improve!

[German Chyzhov]

In my teams we often use such approach - person puts code on review before covering it with tests. This way we minimize a possible wasted effort.
However, of course for complex tasks a selected approach should be discussed with team members before diving deep into a coding.

[German Chyzhov]

Thanks for sharing, that is a brilliant piece.
One question is looks to me not covered here: how do you find a good code reviewers?
For instance, Alice created a PR. Who and how should select code reviewers?
I think, there could be 3 strategies:

1. Every team member (Bob, Ann, Wagner, John) selects any PR's they want to review. Cons: the approach does not guarantee enough number and quality of reviewers, because for instance Wagner and John have never worked with that part of business/tech domain.
2. Only team lead (Bob) is responsible for reviewing all PR's Cons: tech lead could be overwhelmed with different tasks, so review quality would suffer; he becomes a single point of failure
3. Select reviewers based on experience in affected code components, modules, libraries and frameworks Cons: it could require a plenty of time to investigate, especially when the team is 10+ developers.

Please share your thoughts on this question.
What strategies have you used and have you met any issues with them?

[Hideaki Ishii]

Thank you for the comment, and I think it's a good point!

I think, there could be 3 strategies

To be honest, I select the strategies depending on the situation.

I usually select the "3" way and I don't have experience in working in a team whose members are more than 10 (basically 2 ~ 5 members).

• If my PR is very important in the business (core DB design, API design, or something important), I assign reviewers like:
  • A tech lead (required)
  • Other members who will work in the same part in the future (optional)
• If my PR implements a normal feature or fixes a bug
  • Someone who has worked in the same part before, or someone who is good at the part
    • For example, If I implement a front-end feature, I want to get a front-end specialist's review as much as possible (required)
  • It may be good to assign the second reviewer (e.g. junior member) to share knowledge (optional)
• If my PR is very simple (small refactoring or something)
  • Every member
    • In this case, also, It may be good to assign a junior member to share knowledge