DEV Community

loading...

What is a good code review?

Hideaki Ishii
I'm a Software Engineer, who mainly uses Ruby / Rails, JavaScript / TypeScript, Golang, etc.
Updated on ・5 min read

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

Discussion (5)

Collapse
marcelofarias profile image
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?

Collapse
danimal141 profile image
Hideaki Ishii Author • Edited

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!

Collapse
heroincommunity profile image
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.

Collapse
heroincommunity profile image
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?

Collapse
danimal141 profile image
Hideaki Ishii Author • Edited

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