Recently, I read articles about the best practices of code review and noticed that these articles focus on finding bugs, practically ignoring other components of the review. Constructive and professional discussion of the problems found? Never mind! Just find all the bugs, and then it goes.

So I had a revelation: if it works for code, then why won't it work in a romantic relationship? So, meet a new e-book that will help programmers in their relationships with their loved ones (the cover on the illustration to the left).

My revolutionary book will teach you proven technicians to identify the maximum number of flaws in your partner. The book does not cover the following areas:
')
• Discussion of problems with empathy and understanding.
• Assistance to the partner in eliminating deficiencies.

As far as I can understand from reading the literature on code review, these parts of the relationship are so obvious that they are not worth discussing at all.

How do you like this book? I guess that you do not really like it.

So why do we conduct code review in this way?

I can only assume that the articles I read are from the future, where all developers are robots. In this world, your colleagues welcome the careless criticism of their code, because the processing of such information warms their cold metal hearts.

I'm going to make a bold assumption that you would like to improve the code review in the present, where you work with people, not with robots. Even more courageous assumption that a good relationship with colleagues is the main goal, and not just a variable to speed up the correction of errors (minimizing the cost-per-defect parameter). How would your review methods change to reflect these circumstances?

In this article we will discuss techniques that suggest that code review is not only a technical, but also a social process.

What is code review?

The term “code review” can mean different actions, from simply reading some code from behind the developer to a meeting for 20 people, where you parse the code line by line. I am referring here to a formal and written procedure, but not burdened by a number of meetings to inspect the code.


The author , who wrote the code and sent it for review, and the reviewer , who analyzes the code and makes a decision when it is ready to be added to the general code base of the project, take part in the review code. Several reviewers may participate in the process, but for simplicity, suppose that he is one.

Before starting the review, the author should create a list of changes , where he lists all the changes he wants to make to the general code base.

A review begins when the author submits the list of changes to the reviewer. It takes place in several rounds . Each round is one complete cycle of reception and transmission between the author and the reviewer: the author sends the changes, the reviewer responds with a response to these changes. Each code review consists of one or more rounds.

The review ends when the reviewer approves the change. This is usually accompanied by a LGTM message, the abbreviated phrase “looks good to me”.

Why is it so hard?

If a programmer sends you a list of changes that he considers amazing, and you answer him with a detailed listing of the reasons why he is wrong, such communication requires a certain delicacy.

“This is one of the reasons why I don’t miss IT, because programmers are very unattractive people ... For example, in aviation, those who overestimate their abilities are already dead.”

Philip Greenspan, co-founder of ArsDigita, from the book Founders at Work .

It is very easy for an author to perceive criticism of his code in the sense that he is an incompetent programmer. Code review is an opportunity to share knowledge and make meaningful engineering decisions. But this is impossible if the author perceives the discussion as a personal attack on himself.

As if even without this, difficulties are not enough, here you have to express your thoughts in writing, which further increases the risk of misunderstanding. The author does not hear the tone of your voice, can not perceive body language, so it is important to carefully select the wording. For an author who has taken a defensive position, an innocuous note like "You forgot to close the file descriptor" may sound like "I can not believe that you forgot to close the file descriptor here!" You are such an idiot! ”

Techniques

1. Give computers a boring piece of work.
2. Issue style arguments as a style guide.
3. Start review immediately
4. Start at a high level, and go down below.
5. Generously use code samples.
6. Never say "you"
7. Write reviews as requests, not commands
8. Justify with principles, not opinions

Give computers a boring piece of work.

When meetings and emails are constantly distracting you, it is difficult to make time for a thoughtful analysis of the code. Mental power is even less than time. Reading colleague code is cognitively difficult and requires a high level of concentration. Do not spend these resources on tasks that a computer can perform, especially if it performs them better.

An obvious example is errors with spaces indented. Compare how much effort it takes to identify such an error from a person, compared to the use of an automatic formatting tool:

What is required for manual editing	What is required when using the automatic formatting tool
Find extra spaces and invalid spaces. Write a comment calling for the correct formatting of the code. Re-read your remark and make sure that it is worded clearly, without any hint of accusations. The author needs to read the remark. The author corrects indents in the code. The reviewer verifies that the author has corrected everything correctly.	. . . . . . . Nothing!

The right side is empty because the author used a code editor that automatically formats spaces whenever you click the Save button. Well, the worst end, when the author sends his code to check, the continuous integration system reports the wrong spaces. The author corrects the problem before the reviewer noticed it.

Look at other mechanical actions that can be automated, these are the most common:

Task	Automated Solution
Check builds	A continuous integration system such as Travis or CircleCI .
Check Auto Tests	A continuous integration system such as Travis or CircleCI .
Check that indents and spaces match the corporate style.	A code formatting tool such as ClangFormat (for C / C ++) or gofmt (for Go).
Identifying unused modules or unused variables	Static code analyzers such as pyflakes (linter for Python) or JSLint (linter for JavaScript).

Automation helps you as a reviewer make a more valuable contribution. If you ignore a whole class of problems, such as the order of importing modules or the naming convention for source files, then you can concentrate on more interesting things, such as functional errors or insufficient readability of the code.

Automation is beneficial to the author. With it, he can detect careless errors in a few seconds, not in a few hours. Instant feedback speeds up learning and simplifies error correction, because the author’s context is still in working memory. In addition, the author is much easier to perceive the message about a stupid error from the computer, and not from you.

It’s necessary for everyone to work together to implement this type of automatic checks in code review workflow (for example, hooks before commits in Git or web scams in GitHub). If the review process requires the author to start such checks manually, then you lose most of the advantages. The author will inevitably sometimes forget about checking, which is why you have to mess around with simple errors that automatic checking could fix.

Issue style arguments as a style guide.

Discussion of style is lost time in the code review process. Of course, the uniform style is important, but the discussion of the code is not the right time to debate where to put the curly braces. The best way to eliminate controversy and style from the whole process is to have a style guide.



A good style guide defines not only external design elements like naming conventions or indentation rules, but also how to use the features of a given programming language. For example, JavaScript and Perl are packed with functionality - there are many options for implementing the same logic. The Style Guide defines One Correct Programming Method, so you will no longer see that one half of your team uses one set of language functions, and the other half a completely different set of functions.

As soon as you have a style guide, you no longer have to spend review cycles on exchanging messages with the author in disputes about the best way to name files. Just follow the guide and move on. If your manual does not contain instructions on a particular issue, you usually don’t have to argue about it. If you encounter a question in style that is not described in the manual, but worthy of discussion, decide it with the team. Then write the solution in the style guide, never to return to this discussion.

Option 1: Adapt Existing Style Guide
If you search the Internet, you can find published style guides — and borrow them for your own use. The most famous are Google's style guides , but you can find others if these do not fit. By adapting an existing document, you get all the benefits of having a style guide, without wasting time writing such a document from scratch.

The disadvantage is that all organizations adapt these rules for internal needs. For example, Google’s guidelines are very conservative regarding the use of new language features , because they have a huge code base that should work on everything: from a home router to the latest iPhone. If you have a four-person startup with a single product, then it makes sense to choose a more aggressive strategy in using the latest features or language extensions.

Option 2: Gradually supplement your own style guide.
If you do not want to adapt an existing document, you can create your own. Every time when there is a discussion on style during the review code, raise before the whole team the question of what should be the official agreement. When you reach agreement, secure this decision in the style guide.

I prefer to keep our manual in Markdown format in the monitoring system version (for example, on GitHub Pages ). So all changes go through the standard review procedure - someone must explicitly approve the changes, and everyone in the team can raise any problem. Wiki and Google Docs are also suitable.

Option 3: Hybrid approach
By combining options 1 and 2, you can adapt an existing manual and simultaneously maintain a local document to expand or rewrite the database. A good example is the Chromium C ++ manual. There, as a base, they took the C ++ manual from Google, but made their own changes and additions.

Start review immediately

Regard code reviews as a high priority task. When you study the code or write a review, take your time, but start doing it immediately - ideally, within a few minutes.



If a colleague sent a list of changes, it is likely that his work was suspended until you received your review. In theory, the version control system allows you to create another branch and continue working, and then merge changes into the review with a new branch. In reality, there are about four developers in the world who can do this effectively. For all the others, the disentangling of tripartite diffs takes so much time that it levels any progress that can be made while waiting for a review.

If you start a review immediately, then create a favorable cycle. The entire workflow becomes solely a function of the size and complexity of the list of changes made by the author. This encourages authors to send small, precisely worded lists. They are easier and more pleasant to consider, so your reviews will speed up, and the favorable cycle will continue.

Imagine that your colleague implemented a new feature that requires changing 1000 lines of code. If he knows that you can review a list of changes of 200 lines in about two hours, he will break this function up into fragments of approximately 200 lines and complete the review of the entire function in one or two days. But if any review code takes you a whole day, regardless of size, then the function approval will take a week. Your colleague does not want to wait a whole week, so he would rather send you larger lists, 500-600 lines each. They are harder to consider, and the reviews will become poorer, because it is harder to keep the context for the change by 600 lines than by 200.

The absolute maxim of one round review should be one working day. If you are busy with a problem with an even greater priority and do not fit in one working day, let your colleague know about it and give him the opportunity to choose someone else for the review. If you have to refuse to review more often than once a month, then it is likely that the team needs to reduce the pace of the race in order to keep the sane development practices.

Start at a high level, and go down below.

The more notes you write in each particular round, the greater the risk that the author will feel depressed. The exact limit depends on the developer, but the danger zone usually begins in the region of 20-50 notes for one round of review.

If you do not want to drown the author in the sea of ​​comments, limit yourself to only high-level edits in the first round. Concentrate on issues like redesigning a class interface or splitting complex functions. Wait for these problems to be corrected before moving on to low-level questions, such as naming variables or clarifying comments in code.

Your low-level notes may become irrelevant when the author makes a high-level edit. Transferring them to a later round, you protect yourself from the nontrivial work on a careful selection of words for comments on these controversial topics, and the author will not have to handle optional comments. This technique also segments the levels of abstraction on which you concentrate in the review process, helping you and the author to clearly and systematically go through the list of changes.

Generously use code samples.

In a perfect world, the author of the code will be grateful for any review. For him, this is an opportunity to learn and protection against errors. In reality, there are a lot of external factors, because of which authors may negatively perceive the review and resent that you make comments on their code. Maybe they are in a hurry because of the deadline, so any hitch, except for instant approval, seems to be an obstacle. Maybe you don’t work together so long and they don’t believe that your comment is made of good intentions.

A good way to improve the perception of the code review by the author - to find an opportunity to give him a gift. And what kind of gifts do all developers like to receive? Of course, code samples.



If you facilitate the work of the author by writing some of the changes that you propose to make, you will demonstrate that you generously share your time as a reviewer.

Suppose your author is not very familiar with the function of list inclusions in Python. He sends for review a code with the following lines:

urls = [] for path in paths: url = 'https://' url += domain url += path urls.append(url) 

The answer in the style “Can this be simplified with the help of list inclusions?” Will cause irritation because he needs to spend 20 minutes learning the function he has never used before.

The author will be much happier to receive such a note:

I propose to consider the list inclusion of this kind:

 urls = ['https://' + domain + path for path in paths] 

This technique is not limited to single line changes. I often create my own code branch to show the author a great conceptual idea, such as splitting a large function or adding a unit test to cover an additional boundary situation.

Use this technique only for clear, undeniable improvements. In the above list inclusion example, few developers will argue with reducing the number of lines of code by 83%. Conversely, if you write a lengthy example to demonstrate a change that is “better” for your personal taste (for example, a change in style), then such an example of code will give the impression that you push your preferences persistently, rather than being generous.

Limit yourself to two or three code examples for each round. If you start writing code for the entire author's list of changes, it creates the impression that you do not consider the author himself to be able to write this code.

Never say "you"

This sounds a bit strange, but listen: never contact the author personally during the review code process.

Your decisions should be based on ideas for improving the code, not on who came up with these ideas. Your colleague has spent a lot of effort on his list of changes and code, and he is probably proud of the work done. A natural reaction to the criticism of his work is to become in a defensive position.

Match words for recall to minimize the risk of defensive posture. Make it clear that you are criticizing the code, not the developer himself. When the author sees in a comment a personal appeal to him, it distracts his attention from the code and transfers attention to the person. This increases the risk that he will take the criticism personally.

For example, here is an innocuous comment:

You made a mistake in the word "safely."

The author can interpret this remark in two different ways:

• Interpretation 1 : Hey, buddy! You incorrectly wrote the word "safely." But I still think you're smart! This is probably just a typo.
• Interpretation 2 : You made a mistake in the word "safely," moron.

Compare this with a remark in which there is no “you” appeal:

sucessfully -> successfully

The second note is simply a correction, not an assessment of the author.

Fortunately, you can easily rewrite your comments, avoiding the word "you."

Option 1: Replace "you" with "we"

Can you give this variable a more descriptive name, like seconds_remaining ?

turns into:

Can we give this variable a more descriptive name, like seconds_remaining ?

“We” reinforces the collective responsibility of the whole team for the code. The author can go to another company, just like you, but in one form or another there will be a team that is responsible for this code. It may seem silly to say “we” when it comes to a change that the author clearly has to do alone, but it’s better to look silly than the accuser.



Option 2: Remove subject from sentence
Another option to avoid personal appeal is to use an abbreviation that excludes a subject from the sentence:

We need to think about renaming the variable for a more visual name, like seconds_remaining .

The same effect can be achieved with a passive voice. I usually in technical texts avoid passive voice like the plague, but this can be a useful way to get around the problem with personal handling:

This variable should be renamed for a more descriptive name, like seconds_remaining .

Another option is to rephrase the sentence in the form of a question that begins with the words "What about ..." or "How about ...":

What about renaming this variable for a more descriptive name, like seconds_remaining .

Write reviews as requests, not commands

The revision code requires more tact and caution than regular communication, because here there is a higher risk that the discussion will slip into a personal dispute. It would seem that reviewers should show more politeness and courtesy in the review code compared to personal communication. But I found in a strange way the opposite situation. Many people will never tell a colleague, "Give me that stapler, and then bring me some soda." But I have seen many cases where reviewers issue reviews in such a command style, such as “Transfer this class to a separate file”.

But do not be annoyingly polite in your comments. Design them as requests or suggestions, not as commands.

Compare the same remark in two different ways:

Note issued as a command	Remark as request
Transfer the Foo class to a separate file.	Can we transfer the Foo class to a separate file?

People like to control their work. Such a request gives the author a sense of autonomy.

Such requests also increase the chances that the author will respond politely. Maybe he has his own reasons for making this choice. If you make a comment in the form of a command, then any author’s disagreement takes the form of disobedience. If you make a comment as a request or a question, the author can simply answer.

Compare how belligerent the conversation seems to be, depending on the design of the initial comment:

Review, decorated as a team (aggression)	Feedback, issued as a request (cooperation)
Reviewer : Transfer the class Foo to a separate file. Author : I do not want to do this, because then he will be too far from the class Bar . Customers almost always use them together.	Reviewer : Can we transfer the class Foo to a separate file? Author : We can do this, but then it will be too far away from the Bar class, and customers usually use these two classes together. What do you think?

See how much more civilized communication was when you designed an imaginary dialogue as proof of your thesis made a remark in the form of a request, not a command?

Justify with principles, not opinions

When you make a comment for the author, explain the proposed change and its reason . Instead of the phrase “We should divide this class into two”, it’s better to say “Now this class is simultaneously responsible for both downloading the file and parsing it. It should be divided into a class for downloading and a class for parsing according to the principle of common responsibility . ”

If you justify comments principles, the discussion takes a constructive form. When you quote a specific reason, like “You need to make this function private in order to minimize the open interface of the class”, the author cannot simply answer “No, I prefer to do it my own way”. And if he answers like that, it will look stupid, because you have shown how change allows you to achieve the goal, and he simply declared his preference for some method.

Software development is both an art and a science. It is not always possible to formulate exactly what is wrong with this code fragment from the point of view of established principles. Sometimes the code is simply ugly or unnecessarily complicated, and it is difficult to articulate exactly why. In such cases, explain as you can, but maintain objectivity. If you say “ It seems to me difficult to understand”, then this is at least an objective statement, in comparison with the phrase “ This is a tangled code”, which is a value judgment with which not everyone can agree.

Provide supporting evidence in the form of references where possible. The relevant section of your team’s style guide is the best link you can provide. You can also refer to documentation for a language or library. StackOverflow answers with high ratings are also appropriate, but the further you move away from authoritative documentation, the shakier the evidence becomes.

Part 2: coming soon

In a couple of weeks I will post the second part of the article. Keep in touch; we’ll look at additional tips there, including:

• Work with too large revision code.
• Recognition of opportunities to give a high rating.
• Compliance with the scope of the review.
• Actions in desperate situations.

UPD. Published the second part of the article.