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 beings 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 a lot of 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.
An other 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!
Top comments (3)
Many postings on code-review explain what good code should be, refreshing to read a posting on how a code-review should be done.
On pull-requests: I'd go even further, many times you should not do reviews via pull-requests at all. When you develop code in a team, all code should be (re)viewed by at least one other team member, preferably by more. Pull-requests are not the only way to do that, and when you work in a team, where everyone is on-site or easily reachable virtually, pull-requests are an overhead at best, and ineffective at worst.
If you pair- or mob-program, you have code-reviews built in your development ( from this friendly answer on twitter ). But even if you program at your own, instead of creating a pr and continue on your next jira-ticket, you can ask a team-member to review your code directly. Ask your colleague to sit next to you, while you let him or her view your changes, or share screen if you work remotely.
I totally agree with you but I see some advantages to use Pull Requests:
master) because of his own code, he can provide missing informations before the merge.I don't tell we shouldn't do IRL code review, peer and mob but do pull request review in addition of the other forms of code review.