Code Review

by mmyoji

2 min read

work git

I am currently work with finding a new workplace and an interviewer asked to me "What do you keep an eye on when you review a code of other's?" during the interview yesterday.

This was an interesting question and I take a note of it because I've never written about this.

I omit pull-request or merge-request as p-r in this post.

How to review code

First, I check whether the p-r passes build. If not, I don't start the code review. It doesn't deserve to be checked.

Second, I check the p-r title and description to understand the following:

  • What to solve in this p-r
  • The specification of new feature
  • Background of this p-r
  • etc.

Then start to read the code.

I'll check:

  • Can I understand the code intension?
  • Are the test cases enough?
  • Does the code follow common way or common API usage on the language or the framework?
  • Does the code performance not too bad?
  • Granularity of commit
  • Well-explained commit message

I don't point out even if the code is not the best solution from the point of performance. I can fix it when the performance becomes a problem in future. Or I will point out when I know the reviewee is a high-skilled or a smart person.

From my experiences, the granularity of commits and well-explained commit message are often ignored, only a very few of smart developers or engineers can care about them. In most cases, I don't mention about these things when I recognize the reviewee doesn't have enough ability.

In addition to these check points, I take care of not to talk about preferences. This is really ridiculous and time-wasting. On the contrary, I get bored and disappointed when someone start to talk about such a thing on my p-r.

How to submit pull-request

This part is what I care about as a reviewee. Most things are just opposite things from the section above.

  • Rebase/merge the base branch to follow the latest changes into my branch before asking code review.
  • Ensure my branch passes build
  • Arrange commit granularity for reviewers to review each commit
  • Submit p-r as small unit as possible
  • Submit p-r for a single purpose
    • Don't mix multiple purposes in a single p-r
    • e.g.) refactoring and adding a new feature
  • Contain enough test and test cases
  • Write understandable commit messages.
    • e.g.) why the code change is required, what problem this commit will solve, etc.

After review, rebase/merge the base branch again and ensure the branch passes build, then merge it.

Conclusion

I can't say "This is the best code review style in the world" and we have to struggle with it forever.