Code review: how to be a good writer
Hey! I work at Joom on the infrastructure team. In my practice as a code reviewer, I regularly encounter the fact that the author does not understand that the reviewer is not a magic black box into which you can throw any changes and get feedback on them. The reviewer, like the author, as a human being, has a number of weaknesses. And the author should (if, of course, he is interested in a quality review), help the reviewer as much as possible.
I want to tell you how the author of the code can simplify the work of the reviewer and thereby increase both the quality of the review and the productivity of the reviewer. This article may well be used in your internal corporate documentation as a guide for preparing changes for review. It, in fact, was compiled from such a guide.
Why do we do a code reviewYou probably know this without me. Therefore, I will tell you about the very basics, without going into details.
Code review slows down development. This is the main argument that you will hear from the opponents of the review. And this is true, but not all. The review will really slow down your development. Changes will indeed take longer all the way from keyboard to production. However, by slowing down development "here and now", the review allows you to make the complexity curve of system change more flat. I do not pretend to be scientific, but I illustrated this in the graph above.
He shows the concept: a quality review process reduces the rate of increase in complexity. And this is the main value of the code review. This is the value that business understands, since we are talking about the speed of development, which directly reduces the cost of development and increases competitiveness. But if it is suddenly planned that your project should not survive for a long time the point of intersection of the graphs, then the review may not be introduced.
At a primitive level, a code review is also a model of how the written code will interact in the future. If the reviewer does not have any problems with understanding, then there will be no difficulties when reading or debugging the code in the future. And vice versa, if the reviewer cannot understand anything, then this is write only code. Its development and debugging will cost more.
At a higher level, the review keeps the architecture of your system intact. The Review resists the unjustified introduction of elements that contradict the system (also known as crutches). The review promotes a consistent approach to problem solving, which also reduces project complexity.
Next, we'll focus on the most fundamental part of the review process - understanding the code.
How a reviewer does a reviewFor the review process to be such, the reviewer needs, no more and no less, to understand the essence of the changes. This is the basis of the process, without which the review makes no sense. It takes time to understand. And this is not the amount of time that can be spent during the break, "drinking coffee." It takes time to understand, comparable to the time it took to write the code. Sometimes even more. Reviewing is a full-time job, along with writing code. It's even more complicated in some ways.
The reviewer is often not as immersed in context as the author. This also makes it difficult to understand the essence of the changes. For the first time in his life, the reviewer can see the code in which the changes are made. The reviewer may not know anything about the problem the author is solving. The reviewer may not understand why all this is being done. Providing contextual information to the reviewer in a timely manner saves the reviewer's time and improves the quality of the review.
If we are talking about some non-trivial changes, then the same analysis process takes place in the reviewer's head as in the author's head. And if in the course of this analysis different solutions may arise that do not have obvious advantages over each other, then the reviewer will have to spend time to understand why the author settled on a certain solution. The author, in the end, could make the only correct decision, but the reviewer, in the absence of justification from the author, will spend his time on this justification.
It happens that the author, along with the main changes, does something that is not related to the solution of his problem. When faced with such a reviewer, he spends additional time to understand how these changes are related to the solution of the main problem.
Another big time-consuming reviewer is writing comments. Yes, there are comments that relate to some, conditionally, cosmetic aspects. However, even they can be useful to provide accompanying information such as a link to the desired part of the document on the style of the code. And it all takes time.
It should also be remembered that a reviewer, being a human, tends to postpone the start of a review until later if he sees a review of increased complexity. A polite reviewer may hesitate to demand to split a large Pull Request into several and will pay for this with his time and quality of the review.
How to help a reviewer conduct a quality reviewThe main task of the author, who cares about the quality of the review process, is to simplify and speed up the work of the reviewer as much as possible. An author who dumps the result of his work on the reviewer without additional processing shows an extreme degree of disrespect for the reviewer's work.
Before you turn your changes into a Pull Request, you should break them up into logical chunks, if necessary. The comfortable size of the review ends with about 500 lines of change code. Valid is about 1000 lines. Anything beyond 1000 lines should be split into smaller Pull Requests.
If you, like me, do not break your changes into chunks of a comfortable size in advance, then this will have to be done before submitting your changes for review. The excuse that you need to spend time on this does not work. When you decide to submit 1000+ lines of code for review, you actually value your time more than the reviewer's time. According to our rules, the reviewer always has the right to demand that changes be split into smaller chunks. We always ask colleagues to be understanding if he takes advantage of this. With experience, it becomes easier to organize your work so that you do not have giant Pull Requests in which "nothing can be separated."
Separately, it is worth mentioning the changes made to the code using auto refactoring or sed. The volume of such changes can be very large. Having automatic changes along with meaningful changes complicates the review. Always separate autorefactorings into separate Pull Requests if their volume is comparable to the amount of new code you write.
The next part of the preparation is to inform the reviewer about the essence of the changes. As I wrote above, the reviewer needs to know: what was done, why it was done, how it was done, and why it was done exactly that way. The reviewer may already have some of the information. Some are in the code. Everything that is not obvious from the code should be written in the description of the changes.
If the task implied some kind of non-trivial analysis, then the author should be prepared that the reviewer will carry out the same analysis. And the reviewer may come to a different conclusion. It often happens that the author, in the process of solving a problem, went through the stage of a “simpler” solution and rejected it in the process for some reason. To save the reviewer's time, it is worth including the rationale for certain decisions in the description of the changes. This rationale will remain in the project, allowing you to more confidently change the decision if the considerations that guided the author when making a decision have lost their relevance.
There are situations when the reviewer may have his own strong opinion about how the problem should be solved. In this case, the author should discuss this with the reviewer in advance in order to exclude a situation that is extremely uncomfortable for all parties when the reviewer requires to redo a significant part of the changes. You may even have to meet and speak with the reviewer to justify your decision and convince the reviewer or make his decision.
So, the author has prepared the changes for the review. Is it time to send a reviewer? Not! Here comes another important stage, self-revelation. By self-review, I mean exactly the review carried out by the author himself. Not just a cursory visual inspection. You need to try to conduct a full-fledged review. Responsibly conducted self-survey will save a lot of time for the reviewer. Surely the code contains some remnants of debugging constructs, commented out code, TODO comments, meaningless formatting changes, unnecessary code, and so on. Looking through the eyes of the reviewer will also help assess the complexity of the review and provide an opportunity to add additional information to the review that was missed during preparation. I consider the author's neglect of self-review to be a manifestation of extreme disrespect for the reviewer.
In general, I consider it acceptable to spend about 10% of the time spent on writing the code on preparing for the review. This is the author's time, which we will exchange for saving the reviewer's time and improving the quality of the review. It should be remembered that a quality review can easily require 20% or 50% of the time spent by an author writing code.
Only now the author can send changes to the reviewer with a clear conscience.
Then the life cycle of the Pull Request begins. The reviewer must either approve it or ask for changes. To simplify the reviewer's work, the author should add a comment to each requested change. It could very well be a short "OK" or "Fixed" if nothing else is required. Make sure you understand what the reviewer is asking and that you understand his reasoning. You should not unconditionally accept any requests for changes, thereby elevating the reviewer to a near-divine rank. Review is a reciprocal process. If the reviewer does not understand something, he asks the author about it, and vice versa. In case the author is not very experienced, the reviewer should make special efforts to describe change requests in order to take advantage of the opportunity to share his experience with the author. There are also controversial moments when the argumentation is not strong enough to persuade both sides to a common point of view. Taking into account the fact that the author is in a more vulnerable position, I believe that, all other things being equal, the advantage should remain with the author.
Do not make changes to your Pull Request that are not what the reviewer asked you to do. This is extremely confusing. You should also refrain from rebase and similar actions.
Got approval from the reviewer? Great, another well-written and well-designed change has been added to the project!
|Vote for this post
Bring it to the Main Page