How to do a better code review?

#discuss #codereview

We recently started doing a code review of every PR that our team members make.

Most of the time I worry about commenting on something and requesting changes from them. Because in my head I am thinking what if this sounds rude to them or what if I am wrong, or what if they don't feel good about my comments?

Have you witnessed a similar situation? How do you handle that?

Top comments (8)

Avalander • May 6 '20 • Edited

I have a few personal guidelines I try to follow when reviewing pull requests.

Do not comment on style matters that we have not agreed upon as a team. Yeah, I like single quotation marks and they have used double quotation marks here, but we haven't agreed on either so let it go. If it really bothers you, start a discussion with the team about code style instead, but pull requests are not the place for that.
Try to understand the reasoning behind decisions that I don't agree with. For instance, instead of saying it's bogus to create a class for this logic when it could easily have been just a function, I'd say is there any reason to use a class here instead of a simple function?. If I don't find the response satisfactory I can always start a discussion later, but more often than not either I missed something that justifies the design, or the author will be like oh yeah, hadn't thought of that, I'll change it.
When asking for changes, offer code suggestions. I'd say something like this logic is repeated in these three places, we could create this higher order function code example and then compose it in these places like that another code example. Then the author will have a clearer idea of what I mean. Plus, if they are lazy they can just copy paste my suggestions.
If something is not critical but I think can be done better, offer it as a suggestion for the future without requesting a change in the current pull request.
Make positive comments. I can't stress this enough, but if you only take one thing from my post, take this. Especially if you are just starting with pull requests or as a team. If you learn something you didn't know, see a smart trick, the author applied something you taught them previously, or simply think a piece is particularly elegant, write a comment to tell them. You can hardly show too much appreciation to your colleagues. Showing genuine appreciation goes a long way to create trust within the team, and it's easier to take criticism in a part of the code if you're shown appreciation in another part.

The bottom line is try to make sure that the code is good enough, and that you all are learning in the process, not try to make it as if you would have written it.

Rajesh Dhiman • May 6 '20

Hi Avalander,

Thanks a lot for the suggestions. These are very helpful. I'll follow them all.

Gergely Gombos • May 6 '20

Here is my experience.

A good approach is not requesting, only suggesting changes, and pointing out actual requests i.e. where the current situation is not acceptable. Always tell why so that the PR creator can work out a solution.

Also, it's important to know what is (your) preference/opinion vs. an actual issue with a code (bug, not clean enough etc.).

And most importantly, don't commit into the code unless you want to drive people nuts. :D

I think that if you're unsure then ask instead of stating things, and suggest instead of requesting. Assertive communication goes a long way, and every PR submitter needs to get used to getting their code reviewed.