Code review humanly (part 2)

This is the second part of the article on how to properly communicate and avoid errors in the code review process. Here we will talk about how to bring the review to the end and avoid unpleasant conflicts.

The basics are set out in the first part , so I recommend starting with it. But if you can’t wait, here’s a brief summary: a good reviewer not only looks for bugs, but also provides honest feedback to help a colleague improve his level.
')

My worst revision code

The worst code review in my life was for a former colleague, let's call it Mallory. She began working for the company several years before me, but only recently moved to my department.

Review

When Mallory sent the first changelog for review, the code was pretty raw. She had never written in Python before and never developed a system on top of the clumsy legacy system that I had to maintain.

I have faithfully documented all the problems that I managed to find, totaling 59 pieces. If you believe the read literature on the code review, I did an excellent job: I found SO many errors. I really had something to be proud of.

A few days later, Mallory sent a new list of changes and a response to my notes. She corrected the most simple errors: typos, variable names, etc. But she refused to solve high-level problems, such as unspecified code behavior for corrupted input data or the fact that the control flow structures in one of its functions were at the sixth nesting level. Instead, she sadly explained that fixing these problems is not worth the development time.

Angry and frustrated, I sent new comments. My tone was professional, but I went over to the field of passive-aggressive: “Can you explain why we need ambiguous behavior for distorted input data?” As it is easy to guess, Mallory was becoming more and more stubborn.

Cycle of bitterness

It was Tuesday, it's been a week. Mallory and I were still exchanging letters about the same code review. I sent her my last comments in the evening of the previous day. I deliberately waited until she left work, because I did not want to be in the same room with her when she received the letter.

All morning I felt physical heaviness in my stomach because I was afraid of the next round of the review. When I returned from dinner, I saw that Mallory was not in the workplace, but new changes came from her. I guessed that she also did not want to be in the room when I received them.

My heart was beating ever more hard in the chest as my disturbance increased with each of her answers. I immediately began to drum on the rebuttal of the keyboard, pointing out that she had not accepted any of my proposed changes, and at the same time did not provide any excuses so that she could approve her code.

This ritual was repeated every day for three weeks. The code hasn't changed much.

Intervention

Fortunately, our most experienced colleague, Bob, broke this circle. He returned from a long vacation and found that we violently throwing notes at the code review to each other. Bob immediately assessed the situation as a dead end. He asked to post a review to him, and we both agreed.

Bob began his review by asking Mallory to compile new lists of changes, highlighting two small libraries, about which we didn’t argue at all, 30-50 lines each. When Mallory did this, Bob immediately approved the changes.

Then he returned to the main list of changes, which was reduced to about 200 lines of code. He made a few small notes that Mallory corrected. Then approved the list of changes.

All Bob's review ended in two days.

Communication matters

You might have guessed that the conflict did not really arise because of the code. There really were problems there, but they clearly could have been solved by a colleague who possessed the skills of effective communication.

It was an unpleasant experience, but I am glad to remember him. He made me reconsider my approach to code review and identify areas for improvement.

Below I will share some techniques that will help you avoid such unpleasant situations. Later, I will return to Mallory and explain why my initial approach had the opposite effect and why Bob’s approach turned out to be absolutely brilliant.

Techniques

9. Strive to increase the code quality level by only one or two steps.
10. Limit feedback by repeating examples.
11. Respect the review area.
12. Look for an opportunity to break big reviews.
13. Praise sincerely
14. Apply review when trivial edits remain.
15. Avoid deadlocks in advance.

Strive to increase the quality level of the code by only one or two steps.

Although in theory your colleague may have a desire to correct absolutely all the shortcomings in the code, but his patience is not unlimited. If you endorse round after round with approval, he will quickly get annoyed because you have more and more brilliant ideas about how to improve the code.

I myself give the code a grade, like in school, from A to F [from six to stake in the American education system - approx. lane] If I get a list of changes that corresponds to the estimate of D (3), then I try to raise the author to the assessment of C or B-. Not perfect, but good enough.

In theory, it is possible to raise the quality level from D to A +, but this may require up to eight rounds of review. By the end of this work, the author will hate you and will never want to send you the code again.

You might think: “If I approve a code of level C, will this not lead to the whole code base being level C?” Fortunately, this will not happen. I came to the conclusion that when I help a colleague to rise from level D to C, the following list of changes from him already starts from level C. After a few months, he starts sending code that starts from B and goes to level A for the last revision code .

Grade F is intended for code that is either functionally incorrect or so messy that you cannot be sure of its correctness. The only reason to delay approval is if the code stays at level F after several rounds of review. See the chapter on dead ends below.

Limit feedback by repeating examples.

If you notice that the author repeated the same type error several times, you do not need to flag each case. It is not worth spending time writing the same thing 25 times, and the author also does not want to read 25 repetitive remarks.

It would be normal to name two or three separate cases of errors of this type. For everything else, just ask the author to correct all the errors of this type, and do not list each specific case.

Respect the field of review

There is an example of incorrect actions when a reviewer discovers something next to the code from the list of changes - and asks the author to correct it. When he satisfies the request, the reviewer usually finds that the code is better, but he is now inconsistent, so there are some other minor changes to be made. And then a few more. This continues and continues until a specific concise list of changes grows, incorporating a bunch of extraneous things.

If a hungry little mouse appears at your door, you will want to give him a cookie. And if you give, he will ask for a glass of milk. He wants to look in the mirror to check that he has no milk whiskers, and then he will ask for scissors to get his hair cut ...

- Laura Joffe Numerov, "If you give a mouse a cookie"

: , .

: