I have been interested in code review questions for a very long time. Many times one or another problem arose with the quality of the code, then with the climate in the team. Indeed, code review is, if not the only, then one of the most important places for conflicts in the development team.
And recently, in preparation for the next release of the Zinc Prod podcast, I find out that Google has published a Code Review code of practice packed full of valuable thoughts. All the material is quite voluminous and will not fit into one article, so I will try to highlight the most interesting (for me) thoughts.
So let's go
Terms used in the original article:
CL - changelist. But usually this is called Merge Request or Pull Request, so in the article I will use the abbreviation MR
LGTM - Looks Good to Me. In short, when they press the "approve" button. I will use the term "aprov", as more understandable to the population.
Ideality mr
In practice, one can come across various extremes when considering MR. Someone as a whole team zadolivaet comments, until everything is trifled to the point, is corrected, someone looks at about the logic and presses "upru".
An interesting idea is written in the Google document. MR should not be perfect, but it should improve the code base. That is, with each introduced change, the code should get better and better. And if MR adds a lot of good, then you don’t have to find fault with the little things; it’s more profitable to get this improvement faster.
No MR should make the code worse. The only exception is if MR is an urgent fix of something.
Freedom to comment
Despite the fact that you can’t get rid of trifles, nevertheless, you can freely write these trifles to at least every line. The contradiction with the previous paragraph is resolved simply: the reviewer prefixes trifles and nit-picking with the prefix "nit:" from English. nitpick (nitpicking). It is not necessary to fix such comments, however, the author of MR may want to correct something or, even if not, take into account some points for the future.
Facts Over Personal Preferences
Almost always, standard software design principles, based on best practices, are better than tricky bicycles. Therefore, preference should be given to them.
If you can apply several standard approaches, then this is at the discretion of the author.
Direct quote for a better understanding:
Aspects of software design are almost never a pure style issue or just a personal preference. They are based on underlying principles and should be weighed on those principles, not simply by personal opinion. Sometimes there are a few valid options. If the author can demonstrate (either through data or based on solid engineering principles) that several approaches are equally valid, then the reviewer should accept the preference of the author. Otherwise the choice is dictated by standard principles of software design.
If the reviewer and author of MR do not agree with each other
First comes an attempt to reach consensus in the comments on MR. If this does not work, then a personal discussion. If you still do not come to a consensus, then attract team members. But most importantly, MR should not be stuck for a long time due to the disagreement of two people.
Discussed in comments -> Discussed personally -> Discussed in a team -> Moving on
MR check speed
Speed is extremely important. If you give one comment every few days, then the author of MR will complain that they find fault with him and prevent him from working.
If MR is checked very quickly, then any comments will not be a big problem - after all, their fixes will be checked here, and the task will go on.
Google advises you not to be distracted from focusing on your task, but if you are distracted, then see if there is something to revise. For example, he returned from lunch - he revised. Came to work - revised. Etc.
MR viewing order
First you need to look at the MR as a whole. Is it necessary at all, is the code in the right place (or should it be in a separate library), are there any global problems.
Those. it makes no sense to look at some implementation details if the code is not there and not at all.
In general, the viewing order is important in order to give feedback to the author as soon as possible (see the previous paragraph).
When you looked at MR as a whole, you need to go through the main files, i.e. check the most important thing (if it is not clear what is most important, you can ask the developer).
Again, any problems should be reported immediately, even if you have not yet inspected the MR and decided to generally inspect it later.
Next, you need to select a logical sequence for viewing the remaining files. No file should be skipped.
What to look for when viewing
- The code is well designed
- The functionality is well made from the point of view of users of this code, no matter who they are.
- Appearance (if any) should be good
- All the nuances of parallel programming (if any) are taken into account.
- Code not redesigned
- The developer does not overwork: you do not need to write code that may be needed, or may not be needed
- The code has tests
- Tests are well designed
- Names (for everything) are well selected
- Comments on the code are understandable and necessary. They must explain why this is done, and not how it is done.
- Added documentation.
- The code corresponds to style guides.
Too Big MR
Too large MRs must be requested to be broken up. It is almost always possible.
How to write comments on review code
- You need to be polite, not to get personal. Discuss code, not encoder.
- Do not just issue directives for corrections, but explain why you need to fix them.
- Maintain a balance: identify the problem and push the developer so that he himself understands how best to solve it; or immediately issue a ready-made solution. The first develops the developer (strategic benefit), the second improves and accelerates the MR (tactical benefit).
- If the reviewer did not understand at some point in the code and asks the author to explain what's what, then the best answer would be to change the code. So that everything was clear from the code without question.
If you do not agree with your opinion
It is necessary to understand in detail. Perhaps you do not understand something, ask for more information. Perhaps the opposite. In any case, often understanding comes only after a couple of rounds of explanations on both sides.
Fear of upsetting author MR
It happens that the developer is upset if the reviewer insists on some changes. However, most often the developers are upset because of the form of the comment, and not because of the content. Be polite, explain with arguments, and most likely everything will be ok.
"I will fix it later."
If the developer agrees that there is a problem in the code, but asks to fix MR, promising that he will fix it another time, then, according to the guys from Google, this "later" most often never occurs. Therefore, if MR is not some urgent bugfix selling, then you need to insist on a fix.
conclusions
I really liked this document from Google. Especially the life hack with the word "nit", the emphasis on code review speed, and also that MR should not be perfect. It seems to be simple, but as long as this is clearly not considered, it does not reach the point.
If this article seems interesting to readers and picks up a number of pluses, then I will write the following part: the code review process by the author MR.
Update: the second part of the article here
Also, in the upcoming issue of Zinc Sale, we will discuss in detail the review code from different angles. So be sure to subscribe to our development podcast !