Some advice to do a great code review

I work with Git and tools like Bitbucket, GitLab, GitHub… I know my team work hard to do a great work and follow the Gitflow development workflow. However, it is not enough to stop bugs and other regressions to go into our beautiful code base. We talked about that with my team, and we figured out that we should improve our code review sessions inside pull requests.

Often, it isn't the tools, the workflow, or the design pattern that failed, but us as a human being who interact with other human beings. This is the theme of this post: how can we improve our own way to do a code review? How can we better work together?

Your developer teammates shouldn't check the styleguide

In a lot of projects most of the comments are styleguide checks like "you should use space instead of tabs", "You forgot a space here", "In this project, the object keys must be ordered". This is useless comments.

I agree that styleguide should be checked before merging code into the stable code base, but I don't understand why so much team do it manually. Developers' brain is an awesome powerful tool, why should we use it to do basic, redondant tasks? There are many applications that can do it automatically for you as soon as a commit is pushed on your server. Then your developers can spend most of their time writing code which can't be done by a machine.

Another reason to use specific applications to do those tasks is more about human relationship. When I make a pull request and get 25 comments to tell me I added a trailing space at the end of some lines, it just makes me angry.

Why don't the other developers give me advice on real problems about my code? Is this trailing space the only improvement I have to do in my code? Maybe they just hate me.

I know this reaction is not a good one, but I can't act differently! Why? Because those who leave these comments are human beings. Human beings aren't neutral, their reactions can be interpreted. It's not the case with machine. An application only do what it was created to do. No feelings. No judgments.

In my best case, developers should focus on the most valuable issues and automated tools should check for styleguide rules. Either developers' discussion and styleguide warning from the tools should block the merge.

Check unit tests for each pull request

I think the reviewers should check the tests pass each time a pull request is created. They should also check if the author has created new tests for his feature.

The bad thing about tests is they can take a lot of time to be executed and increase the time spent to do a review. The review have to pull the branch, he has to install dependencies, he has to execute the tests. Of course he has to do this every time the author push new code on the branch.

Because of the waste of time, most developers don't do it during a review: and the bug comes creeping in.

To do a better review the tests should be run and the better way to do it without annoying your developers is to set up a CI environment where your tests will be running automatically. The results must be reported directly inside the pull request to inform everybody no matter how good the code is, the tests aren't successful.

In my opinion, the CI should block the merge if unit tests (or other type of tests) fail.

Be kind

Yet another part of the job where human relationship can do worse than better: communication. A pull request is all about communication. A code review is all about communication.

A developer asks other developers for a review, an analysis of his work before merging it. After this step it'll be the team's code but before the merge it still is the developer code and he can be touchy about it.

Also the review comments are written which remove all other communications form but language. The author doesn't hear our voice and the tone of our voice when he read the review. The author doesn't see our gestures or our facial expression. The bigger part of the communication is hidden behind the screen.

Because of that, misunderstanding can appear. Some developers can think a review is agressive. Other developers can think the same review is normal, not agressive at all. Sometime you'll think a review comment is agressive just because you are having a bad day.

My trick to avoid that? Be kind. Do your best to let the author feel your kindness:

• Include you and other developers in the review to be more neutral and inclusive. Use pronouns like our, us, we.
• Don't use imperative style. Use should, could, would, instead of must, have to...
• Specify that it is your opinion and not the ultimate source of truth.
• If the author have to follow your recommandation, you have to explain why. Everything is better when other people can understand the why.

Trick: Your pull requests should be as small as possible

The author can impact how a code review will be done on his pull request. How can he? Git usage.

A developer who wants to receive useful review comments or wants these colleagues to quickly review his pull request, should follow this tiny single rule: one feature = one pull request.

Reviewers are also developers who have tasks to do. Reviewing a pull request takes time. As a reviewer, when I see a big pull request which contains a lot of features I don't want to review it right now but later because I know I am going to spend a lot of time on this pull request.

An other side effect of big pull requests is they are often rushed by reviewers. Try to make two pull requests, one big and one small. In most case the smaller one will get more comments than the bigger one. Why? Because developers are lazy and this won't change when they review pull requests!