Your guide to a constructive code review published 12/12/2019 | 5 min read

Hello everyone, welcome to a new story on devspedia. Today I'm discussing best practices to achieve a successful and a constructive code review.

Why code review?

Code review is a great way to achieve better code quality, and most importantly share great knowledge and experience between developers working on this project.

Most developers tend to use code review to keep code quality and style for both open source and closed source projects, and when combined with automated checks (automated tests), it can becomes a bit easier to review.

What should you know before reviewing code, or asking for review?

First of all, all developers (both the PR owner, and the developers reviewing it) all are on one purpose, is to get the full potential of code review, and share benefit across all parties.

Keep your PR as minimal and as focused on the task it tries to achieve as much as possible.

With this in mind; there are 3 minimum tasks to do before submitting your PR:

Read the contribution guidelines. (should I change/refactor something?)
Make sure your code is aligned with the projects' code style guides. (Oh, got to replace that for loop with .map).
Don't forget to write test cases to cover your code. (Oops, tests are failing).

Once you're done with your checklist, and know that your PR is ready for submission:

Here are a few note to consider while having your code reviewed

Try to absorb as much knowledge and experience as much as possible during review, reviewer is here to give you that.

Reviewer is trying to help, and increase code quality and maintainability (from his point of view).
It's easy to communicate the wrong intention online, make sure you're 100% clear on what you're saying, avoid making personal replies to the reviewer, and also if you found a comment from the reviewer that feels personal, just ask for the intentions clearly, in anyway, try to assume good intentions always.
Don't force push, or overwrite PR's history at all. This will confuse your reviewer and make it harder to keep track of what have you fixed in your last commit based on their comments, try to push comments feedback separately.
Try to respond to every comment.
Understand that the review is on your code, not yourself.
After review, and PR approval, only merge when you're sure all is good, reviewer may have missed something, or you noted an issue, in this case, try to communicate and resolve it before merging.

For the reviewer

Don't make it a Gateway Review.

First of all, give yourself a chance to understand what this PR is about, and what it tries to achieve and then:

Simplicity is king. Communicate ways to make code simpler, while still solving the problem, and not impacting scalability, security or performance.
If you have reached to a point where discussion took so much time, comments going back and forth, and you're just discussing an alternate way of doing something, then just let the author decide on the final solution.
When you propose something, or alternate solution, take into consideration that author may have already considered it. Ex: Don't say: How come you skipped our existing validators all together here?!. But instead: How about using our existing validator here?
And finally, remember that your role in the code review is to provide feedback on the PR, and not a gatekeeper.

Set rules together with your team

Make sure to agree together on the definition of "Done" for the PR, when it should be signed off, or else, you'll find your self in a ping-pong type of code review, or endless review, where the reviewer keeps commenting, and you keep making changes, endlessly.

Finally

I'd like to note, code review is a crucial part of any software development process, and is important and beneficial for everyone involved, it helps keep code quality as best as possible, and gives you and your team mates a chance to share tremendous amount of experience and knowledge during the reviews.

So it's important to have it stay like this, and not to misuse it, as an author, do your best to make sure your PR is actually ready for review as much as possible, and not just a PR ready for tons of comments.

And for the reviewer, also make sure you are helping your team mate achieve a best possible contribution to the code, you help him fix his issues, and know more, but that's all. You're not a code defender.

I'd like to leave you with this video, which discusses in details all types of code review, how you should do it, and more:

-----

Thanks for reading devspedia, I love you, and I'll see you the next time :)