When it comes to code inspection, people's attention is usually focused on who is doing it. But the developer who wrote the code plays just as important a role in the process as the reviewer. There are almost no recommendations for preparing code for inspection, so authors often make mistakes simply out of ignorance.
This article compiles the best practices for participating in code inspection as an author. When you finish reading, you will start sending the inspectors such perfect sets of changes that they will burn with love for you, no less.
But I don't want code inspectors to glow with love for me
And they will still be. Humble yourself. No one in the history of mankind has yet complained on his deathbed that he was too loved during his lifetime.
Why improve code inspection?
Learning effective inspection techniques will make life easier for the inspector, the team and, most importantly, you.
- Accelerated knowledge transfer process. If you prepare your set of changes properly, the inspector's attention will be drawn to the areas in which you need to grow, rather than boring stylistic flaws. And when you make it clear that you value constructive criticism, the reviewer begins to put more effort into the feedback.
- An example for others. Good code inspection practices in your execution will set the bar for those around you. Good techniques are usually adopted, which means that it will be easier for you when someone from your colleagues sends you the code for review.
- Minimum conflicts. Code inspection often creates tension on the team. If you approach the matter responsibly and deliberately, disputes will arise less often.
Golden rule: value the reviewer's time
This seems obvious, but I often come across authors who treat inspectors as their personal quality control specialists. Such people will not strike a finger to catch at least some of the errors or to make a set of changes convenient for checking.
Your colleagues come to work every day with a finite supply of attention. If they devote some part of it to you, then they can no longer spend this resource on their own tasks. Making the most of their time is a matter of simple justice.
Code review is much more successful when the participants can trust each other. Your inspector will spare no effort if he knows that you will take his remarks seriously. If you perceive the examiner as an obstacle that needs to be overcome, then you will receive much less valuable recommendations from him.
Technique
1. Review the code yourself
first Before sending the code to a colleague, read it. Don't limit yourself to catching errors - imagine seeing this fragment for the first time. What can be confusing about it?
In my experience, it can be helpful to take a break between writing and editing code. People often send a fresh portion of the code at the end of the day, but it is during this period that they are most likely to miss something by negligence. Wait until morning, take a fresh look at the set of changes, and then pass it on to a colleague.
What idiot wrote that?
Synchronizing Cron Jobs with the lunar cycle: I added synchronized logic to keep our ETL
pipeline in tune with nature. Simulate a validator's work environment as much as possible. Use the same comparison utility as he did. Stupid bugs are found better in these utilities than in your regular code editor.
Don't expect yourself to be perfect. You will inevitably send the code with a piece that you forgot to delete after fixing a bug, or a stray file that you were going to remove. It's not the end of the world, but it's still worth keeping track of. Notice patterns in your mistakes and figure out what systems you can implement to prevent them. If the same errors slip through very often, the inspector will conclude that you do not value his time.
2. Write a clear description of the changeset
In my last mentoring job, I had occasional meetings with a more experienced programmer. Before the first meeting, he asked me to grab a software design document that I had compiled. As I handed him the papers, I explained what the project was and how it relates to the goals of my team. My mentor frowned and said bluntly: "Everything that you just said should be on the first page of the document."
And he was right. I wrote the document with the expectation of developers from my team, but did not take into account that it might be read by outsiders as well. The audience was not limited to the boundaries of my department - there were still partner teams, mentors, people who made decisions about promotions... The document should have been written so that everything was clear to them. After this conversation, I always try to present my work in the context necessary for understanding.
The changeset description should include any background information the reader might need. Even if you are writing a description to target the reviewer, keep in mind that he may have less contextual information than you think. In addition, it is possible that other colleagues will also have to work with this code in the future - when viewing the history of changes, they should be clear about your intentions.
A good description outlines what the changeset is for and why you chose to do it that way.
If you want to dive deeper into the art of writing great descriptions, read “ How to Write a Git Commit Message ” by Chris Beams and “ My favorite Git commit ” by David Thompson.
3. Automate Simple Things
If you rely on the inspector for such little things as a floating brace or problems with automated tests, then you are wasting his time.
Check if everything is in order with the syntax? I would turn to the compiler, but it's a pity to waste his time.
Automated tests should be considered part of the standard procedure as part of the team. Inspection begins only when the entire series of automated checks in a continuous integration environment has been passed .
If your team is tragically delusional and rejects continuous integration, set up an automated checkout in-house. Implement pre-commit hooks , linters, and formatters into your development environment to ensure that all conventions are followed and the intended behavior persists from commit to commit.
4. Make sure that the code itself answers the questions
What's wrong with this picture?
I don't understand the purpose of the function.
And, this is in case the Frombobulator is passed during the call in the absence of frombobulate implementation The
author helped me figure out the function, but what about the next reader? Digging into the history of changes and re-reading all the discussions? It's even worse when the author comes to my desk to explain everything in person. Firstly, it prevents me from concentrating, and secondly, no living soul will have access to the voiced information anymore.
When an inspector expresses confusion about a point in the code, your job is not to clarify the situation for that person. It is necessary to clarify the situation for everyone at once.
- Hello?
- When you wrote bill.py six years ago, why did you have t = 6?
- Glad you called! This is due to the 6% sales tax.
- Of course!
- A great way to justify an implementation decision.
The best answer to any question is code refactoring that removes it. What can be renamed to make it clearer how to change the logic? Comments are an acceptable solution, but they definitely play out against the background of code that naturally documents itself.
5. Enter changes fractionally
Sprawling boundaries is a bad practice and can often be seen when inspecting code. The developer undertakes to fix a bug in logic, but along the way he comes across some flaw in the interface. "Well, since I'm working with this piece anyway," he thinks, "I'll fix it at the same time." As a result, confusion begins. The reviewer will now have to figure out which changes work for task A and which ones work for task B.
The best changesets do one thing . The more precise and simple the change is, the easier it is for the inspector to keep the context in mind. By sharing unrelated changes, you also get the ability to have multiple peers review in parallel, so the code will come back to you faster.
6. Separate functional
and non-functional changes Another rule follows from the scaling constraint - functional and non-functional changes should not be mixed.
Developers with little experience with code inspection often break this rule. They make some sort of breakdown change, and the code editor immediately changes the format throughout the file. The developer either doesn't grasp what happened, or decides it's even better and sends code where one functional change is buried under hundreds of lines of non-functional ones.
Will you find a functional change?
Such confusion is like a spit in the face of an inspector. Format-only changes are easy to check. Functional changes too. One functional breakdown in the ocean of format changes can be wasted and crazy.
In addition, developers like to mix refactoring into major changes in a timely manner. I strongly approve of code refactoring on the part of colleagues, but not when they simultaneously implement new behavior.
Behavioral change buried in the midst of refactoring
If a piece of code needs both refactoring and behavior changes, you need to break the process down into two or three sets:
- Add tests for the current behavior (if there are none)
- Refactor the code without changing anything in the tests
By keeping the tests intact in the second step, the reviewer can ensure that refactoring does not break behavior. Accordingly, at the third stage, he will not have to figure out what is refactoring here, and what works to change behavior - after all, you separated one from the other in advance.
7. Break up changesets that are too large
Bloated changesets are like the ugly cousins of sprawling boundaries . Imagine a situation: a developer comes to the conclusion that in order to implement functionality A he will first have to correct the semantics of libraries B and C. If there are few changes, then no matter what, but very often these modifications spread out and the output is a huge list of changes.
The complexity of the changelog grows exponentially with the number of affected rows. If the changes affect 400 or more lines, I look for a way to chunk them before requesting an inspection.
Maybe instead of doing everything at once, it will be possible to first work with the dependencies, and in the next set, implement new functionality? Is it realistic to keep the code sane if you implement half of the functionality now and the other half in the next phase?
Thinking about how to break up the code to get a working, intelligible piece can be tedious. But this allows for better feedback and creates less complexity for the reviewer.
8. Accept criticism kindly
The surest way to spoil the whole test is to take criticism with hostility. It can be difficult to resist, because many developers take pride in what they do and perceive their work as an extension of themselves. And if the inspector makes tactless comments or goes to person , then this further complicates the matter.
Ultimately, you, as the author, are free to decide how you respond to comments. Treat the inspector's words as an objective discussion of the code that has nothing to do with assessing your personality. If you get defensive, it only gets worse.
I try to take any comments as an attempt to help and benefit. If the reviewer finds a stupid mistake in the code, I am instinctively drawn to start making excuses. But I hold back and praise his observation instead.
- For January and February 1900, it won't work.
- Exactly, well noticed!
Oddly enough, the fact that the inspector finds unobvious flaws in the code is a good sign. This suggests that you are good at preparing files for scanning. In the absence of sloppy formatting, bad names, and other conspicuous things, the reviewer can go deep into the logic and design and make more valuable observations.
9. Be patient if the inspector makes a mistake
From time to time it happens that the inspector is simply wrong. He can misread perfectly normal code just as well as you can plant bugs. Many developers in such cases begin to fiercely defend themselves. They are offended that someone is criticizing their work, and even not based on anything.
However, even though the reviewer is wrong, you still have a lot to think about. If he misread something, what is the likelihood that other people will make the same mistake in the future? Wouldn't readers have to study the code back and forth, just to make sure that the bug they see is not here?
- There is a buffer overflow here, because no verification was carried out whether there is enough memory in name to accommodate all NewNameLen characters.
- Is it in my code? Unthinkable! The constructor calls PurchaseHats, and it calls CheckWeather, which would throw an error if the buffer length didn't match. You would first read 200,000 lines of code and then dare to admit the possibility that I was wrong somewhere.
Think about how to refactor, or add comments so that at a glance you can understand that everything is in order here. If the misunderstanding stems from the use of little-known features of the language, rewrite the code using mechanisms that do not require thorough knowledge.
10. Give an intelligible reaction
I often find myself in this situation: I send comments to a person, he updates the code taking into account some of them, but at the same time he remains silent. There is a period of uncertainty. Either he missed the rest of the comments, or is still in the process of correcting. If you start checking what has already been done, hypothetically, it may turn out that this piece of code is still in work and I am wasting my time. If we wait, we can get into a stalemate - we will both sit and wait for the next step from each other.
Establish a certain order of actions in the team so that it is always clear who has the "baton" at the moment. The process should not be allowed to slow down just because no one understands what is happening at all. This is easy to do - just leave comments at the changeset level, from which it is clear when the right to act is transferred to another participant.
Correcting the headers> Updating the code according to the comments> Everything is ready, please look.
Any comment that requires you to take action should be responded to by confirming that you have taken it. Some code inspection tools provide the ability to flag processed comments. If not, use a set of simple text bullets like Done. If you disagree with the comment, politely explain why you decided to refrain from correcting.
Some tools like Reviewable and Gerritt suggest tagging processed comments
. Balance your reactions against the effort of the reviewer. If he has detailed a moment so that you can learn something new for yourself, do not limit yourself to the mark “Done”. Answer thoughtfully to show that you value his work.
11. Skillfully solicit missing information
Sometimes the inspector's comments leave too much room for interpretation. When they write to you something in the spirit of "I don't understand this function," the question immediately arises, what exactly is not clear to a person. Too long? Is the name unfortunate? Need to better document?
For a long time I wondered how to clarify vague comments and not inadvertently enter into confrontation. The first thing that came into my head was to ask: “What is incomprehensible in her?”, But it sounds grumpy.
I once deliberately sent a vague remark of this kind to a colleague, and he disarmed me with his answer:
"What can be changed to make it better?"
I like this phrase because it expresses openness to criticism and lack of hostility. Now, when I get vague feedback, I respond with some variation of the basic motive "What to change to make it better?"
Another useful technique is to try to guess what the reviewer meant and take the lead in correcting the code accordingly. If you get an "incomprehensible" comment, take a closer look at the code. Usually something that can be improved in terms of clarity is found. The edits let the inspector know that you are ready to make changes, even if they had a different idea of the final result.
12. Give priority to the reviewer
In tennis, if it is not entirely clear whether the ball went out-of-bounds when serving, it is customary to interpret the situation in his favor. This rule should also be followed when inspecting your code.
Some design decisions are a matter of taste. If the reviewer thinks that your ten-line function would be better divided into two five-line functions, the objective truth is neither yours nor his. Which option is better depends on preference.
If the inspector makes a proposal and the arguments in support of his position are approximately equal for you, accept his point of view. After all, of the two of you, he is the one who takes advantage of a fresh look at the code.
13. Reduce code transfer time
A few months ago, a user made a small change to an open source project that I maintain. I unsubscribed with comments a few hours later, but he already disappeared with the ends. A few days later I returned and found that there was still no answer.
Six weeks later, the mysterious developer reappeared on the project with revisions. Although I appreciate his work, the long break between the two stages of verification led to the fact that I spent twice as much effort on the inspection. I had to re-read not only the code, but also my own comments in order to remember what was generally discussed. If the person showed up in a couple of days, there would be no need for additional work.
Intellectual costs for code inspection with short (left) and long (right) transmission times
X-axis - time; y-axis - inspector memory; blue shading - context restoration; blue fill - code check; gray fill - waiting; red shading - additional costs
Six weeks is, of course, an exceptional case, but I often see long, unjustified pauses in my team. Someone submits a set of changes for review, gets feedback, and postpones the edits for a week because they were distracted by another task.
In addition to the fact that later you have to spend time recovering the context, semi-finished code fragments create complexity. They make it harder to keep track of what is already drained and what is not yet. The more partially processed changelists are bred, the higher the likelihood of merge conflicts, and no one likes to mess with them.
Once you've submitted your code for inspection, your top priority is to follow through. If the process stalls due to your fault, then you are stealing time from the inspector and complicating the life of the whole team.
Finally
As you prepare your next changelog for submission, consider what factors you can address and use those opportunities to make the review productive. As you participate in a code review, check for things that regularly slow down the process and waste resources.
Remember the golden rule: value the reviewer's time. If you give him the opportunity to focus on the really interesting parts of the code, his comments will be useful. If you force him to waste time on little things or unravel confusion, so much the worse for both of you.
Finally, think carefully about your answers. Misunderstandings over trifles or thoughtless words can derail things with alarming speed. Passions run high when it comes to criticizing someone else's work, so carefully avoid anything that might lead the examiner to think that you are attacking him or treat him without proper respect.
Congratulations! If you've read this far, you may already consider yourself a code inspection expert. Your inspectors are most likely already on fire, so you should treat them humanly.