Code Review - good practices

Code review - definition

Here's how Wikipedia defines a code review

Code review (sometimes referred to as peer review) is a software quality assurance activity in which one or several people check a program mainly by viewing and reading parts of its source code, and they do so after implementation or as an interruption of implementation. At least one of the persons must not be the code's author. The persons performing the checking, excluding the author, are called "reviewers".

Having that in mind we can define essentials of this process.

The main purpose of code review is the overall improvement of the code health by continuous improvement of:

Maintainability
Readability
Understandability
Functionality
Scalability

It's important to remember one golden sentence from Google's Standard of Code Review:

There is no such thing as “perfect” code - there is only better code.

Usually, perfection of the code is a matter of opinion - I think we are all familiar with the never-ending tabs vs spaces debate ;)

Code review - advantages and disadvantages

Of course, in an article like this, some pros and cons must be given for implementing such a process in the project workflow.

Let's start with some advantages:

First line of defence against coding errors - usually a careful developer reading through code may find some logical or styling errors.
Improves knowledge of the codebase - if code reviews are regularly done by most developers in the project then everyone automatically has at least a general idea of how most features work. This helps prevent the formation of knowledge silos, allowing the team to be more flexible in case of a developer's unexpected absence.
Double-check if requirements are fulfilled - careful reviewer may notice that some requirements are not fulfilled by just reading the code.
Improves code readability - it's easy to overlook some blank lines or leave typos during the development process, but these mistakes can be easily pointed out during CR. However, code readability is not only about typos and spaces. The more complex the code is the harder it gets to read. Reviewers may help you find overcomplicated code blocks and simplify them a bit.
Knowledge sharing - some neat tricks may be used to create a given solution, which other developers may not know. Also, less experienced developers may learn from their more experienced colleagues.

That's quite a lot of advantages, let's now move to disadvantages. I'll be frank, I had serious troubles with finding them, but nonetheless I've managed to come up with some, and I think they are not minor ones.

Delays the completion of each individual piece of work - an additional step is required between development and testing - code review. This phase usually takes some time, another developer has to read changed code and add his or her review. Later the author has to reply to the comments and apply requested changes. Afterwards, further developers' checks have to be performed... and eventually, maybe at some point everyone will be happy.
Requires the team's attention, which could be needed in other areas of the project - other developers, have to switch the context in their minds and sacrifice their own time for sake of performing good code review instead of focusing on delivering other tickets.

Are code reviews worth doing? In my opinion, definitely - there are many more pros than cons. Well, you may think that this is just my opinion and some other developers could think quite opposite. There is a bit of truth in that. However, if it wasn't worth doing then it wouldn't be so popular across the community, would it? ;)

Comments

Comments are one of the most useful tools which can be used during reviewing code, but there are some principles how one should write them.

Principles of comments in code reviews

Technical facts and data overrule opinions and personal preferences - that means if it's known that some approach is faster or better optimised then it should be used, even if the developer prefers different approach.
On the matter of style, the style guide is the absolute authority. source: xkcd - Code Quality
Aspects of software design are almost never a pure style issue or just a personal preference.
If several approaches are equally valid, then the author is right.

Additionally, the comments must follow some non-technical best practices, they should:

Be kind
Explain your reasoning
Encourage developers to simplify their code or to add comments
Maintain a reasonable balance between giving explicit directions and pointing out the problems
Be about the code (not the developer) source: xkcd - Code Quality 2

On the other hand, it is also very important to know how to properly respond to the comments:

Don't take it personally – Comments are (or at least should be) about the code, never about the developer. Don't be overly attached to your code, one day it WILL be changed. Remember that CHANGE is the very basis of software development.
Critique is an attempt to help – By the critique you gain new knowledge about functionalities or edge cases you hadn't thought of.
Never respond in anger to CR comment! – Angrily written comment may only inflict conflicts, nothing good comes from the conflicts.
In case of misunderstanding of the code – a suitable code clarification should be the first response – If someone doesn't understand a piece of code, then answering her/him in the pull request's comment is of no use for another developer, who may have the same problem in the future. The best way to avoid such situations is trying to clarify and simplify your code. If that's impossible for some reasons, leave a proper comment in the code.

Suggestion

Recently a new type of comments have emerged - suggestion type. It allows you to highlight a line of code and type the suggestion how to change it. Then the author may click apply button to commit these changes. That means that the author doesn't need to go to its local environment to adapt the change, commit it and push it. All of that may be done completely remotely.

In my opinion, all of previous rules still applies to these comments. However, if the suggested change is self-explanatory you may omit your motivation, but if the change is more complex you should think about adding a few words.

Conflicts

It is important to remember that code review is not always about correcting everything that someone says. You may of course disagree with the comments. Not all the suggestions are equally beneficial for the overall codebase health. Some of them may be even more conflictive than the others. A very neat visualisation of the reward-conflict ratio basing on the comment's type is presented on the diagram below:

The group of Low Reward – High Conflict should be usually covered by linters and style guides. Additionally, we don't have to analyse Low conflicts comments too much - they are quite straightforward and (as their name suggest) not likely to ignite conflicts. The group we should really focus on is High Reward – High Conflict. There are a few steps how to deal with the conflicts:

Assume the author is right and try to look at the issue from his or her perspective.
Discuss the issue in the comments
If you can't reach an agreement in the comments, try to discuss it face to face and remember to apply a summary in the comment.
If the conflict still goes, reach the Tech Lead's opinion, whose decision is final.

Keeping these steps in mind, I hope you will be able to avoid unpleasant situations in the project.

Code Reviews – good practices

We have already discussed all the essential principles of code review together with advantages and disadvantages of this process. Now, it's time for the main event of the evening - good practices.

It is always good to ask yourself these few questions while reading the code:

Are there any tests for this functionality?
Isn't the line too complex? – over-engineered lines drop the readability as they usually hide their main meaning.
Does it solve present problems? – it's important to focus on the current problem and not to try solving future problems.
Are the names descriptive enough?
Can this code be reusable? – if the code has a potential to be reusable it's always good to extract it for a generic use.
Are comments understandable? – the comments in the code have one job, help other developers understand your code, if they are not understandable then they should be changed.

Code review is not only for finding mistakes. It's always a good place for encouragement or praising the author. If you find that:

The code is well-designed
The code isn't more complex than it needs to be.
The developer used clear names for everything.
Comments are clear and useful, and mostly explain why instead of what.
Performance checks.

Then don't hesitate, and add some kind words. Let the author know that you are glad for what he did.

At the end I would love to share with you some thoughts how to be a better code reviewer:

Don't be afraid of nits *
Approve even if there are some stylistic problems. – point them out in the comment and press the approve button, the author will change that
Don't accept PRs that degrade the code health of the system.
Read changes line by line – it's important to read every line of the changes in the code.
Read the full list of changes before saving any comments. If software enables that feature (e.g. GitHub reviews) that's great. Otherwise, (e.g. BitBucket) write your comments as you read the code, but don't post them. Almost every time, when I use this technique, I remove some comments/questions because the explanation is below - in the code, I haven't read yet, only because of the order of the files in the browser.

* nit (from Urban dictionary):

Short for 'nit-pick'. Usually being used in code review(programing) meaning: a small change that may not be very important, but is technically correct.

When to perform code reviews

As we get closer to the end of this article, you may have noticed that we haven't talked about one crucial matter - when you should perform code review. The answer is clear, “As soon as possible!”. One business day is a maximum time for a response. Of course, don't interrupt yourself as soon as you notice that there is a new pull request. Switching context is time-consuming and if you just got to the writing trance don't pull yourself out of it immediately. Remember about the PR and perform code review as soon as you finish this mind-consuming task or just after you make a (coffee) break. In my experience I can say, that I love to perform code reviews as the first thing in the morning. With a cup of coffee or tea in the hand, it helps me get to the work trance easily. Of course, later in the day I always check if there are some responses or new PRs.

I hope this article helped you with better understanding of the code review process. I am sure, with a bit of training you'll become a great code reviewer and by that even more sensible developer.

Bibliography:

Special thanks

Special thanks for xkcd for a permission to use their images in this blog post, it wouldn't be the same without them.

I would also like to thank all the people who have spent their own time on a review of this blog post - it means a lot to me!