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
Then start to read the code.
- 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.
I can’t say “This is the best code review style in the world” and we have to struggle with it forever.