“Once upon a time we did a code review, writing comments in the mail, indicating the line number. It was very funny. From the pros: no one looked at anything on diffs, looked in the IDE. But there was also a minus: after some merge, the line numbers changed. "
Alexander Makarov, Yii
“Our company has an interesting concept - a chair request . This is when, within the framework of one office, a developer rolls up to you in a chair and says: 'Look, this is faster than creating a pull request.'
Anton Morev, WormSoft
Recently, there was a public recording of the SDCast podcast about code review on YouTube. We have selected and deciphered the most interesting from the issue.
Full audio version on Spotify, Ya. Music and as a file for download
Full video version on Youtube
Don't treat code reviews like checking something or looking for bugs
Sergey Zhuk, Skyeng: I believe that the fundamental goal of a code review is to share knowledge within the team and find the best solution. The team reviews the proposed changes - and knowledge about the domain is evenly distributed among its members. The author of the solution understands how the code he thought was perfect can actually be improved.
Therefore, code reviews are not something to be avoided or done faster. This is an investment in the business and the team: the code gets better, the team gets better. Yes, at first I didn’t like it when the request was wrapped (and who would like that).
But then I began to view it as a learning process, on a par with reading books or participating in conferences.
I felt this plus myself, as a developer I grew up with this approach.
But there is a nuance. Surely many of you have come across requests for a thousand lines, where refactoring, and a feature, and some minor changes were mixed. By mixing different things in one request, we complicate both the sharing of knowledge and the life of the reviewer: it will be harder for him to assess whether the behavior of the system has changed, whether the business requirement has been fulfilled, whether the problem as a whole has been solved - and is the solution successful?
We noticed this moment in our team and introduced a rule: in one pull request, do not interfere with changes of a different nature. You send refactoring separately, separately feature and separately small changes.
Sergey's report on the practices of his team. The text version is here .
These requests are usually reviewed quickly and easily, and are much more likely to receive quality feedback. And on the part of the maintainer, there are pluses: if you like refactoring, and the feature with a bug, then you can merge refactoring separately.
Alexander Makarov: I agree that you shouldn't try to spend as little time as possible on code reviews. Opened, like the brackets are fine, this code does something - it doesn't work that way. If the reviewer cannot vouch for the code to the end, it means that the code review has not been carried out.
Therefore, we have now begun to compile a fairly large number of guidelines for ourselves, and one of them talks about the code review.
Part of the Yii team's guideline .
But to the thesis about individual small pull requests: I had situations when a lead came and introduced something like that. The team was hostile. How did you get it?
Sergey Zhuk: There was a team in parallel with which we interacted and used their API. They made a bunch of features in a month, we did just a little. That is, initially we didn’t focus on the delivery of features, but on the quality with this approach. And we agreed with the business that we will release it more slowly, but we try not to break anything. A month later, one of the neighbors broke down, then another. But we don't. And on this example, everyone understood everything.
Konstantin Burkalev, SDCast:I had processes for implementing code review in general - and it was not easy, although everyone seemed to understand that it was good, yes. You say, "Guys, let's merge through a pull request." They say to you: "Yes, there is a small feature."
The principle here really works well that when something breaks, you say: “But if you made a request, we would have looked and it simply didn’t come to that.” The idea is for people to understand mistakes from their own experience. Trying to impose doesn't always work.
How quickly to review
During the broadcast, voting took place among the audience. Here is one of them.
Konstantin Burkalev: June is especially often like this: “Well, you watched my request, is it ok there? I wrote it, clenched my fists and wait ”.
And I have my own task, I can only get there in the evening or I don’t know at all ...
Sergey Zhuk: In our team, review has always been a priority. Therefore, there is a regulation - when a pull request arrives, you finish what you are doing now, so as not to lose the context, and go to review. Because what you are about to do is still in the process. And that task has already been done.
The code has already been written, it remains only to look, merge and upload it to the product.
That is, I finished something of my own, switched, quickly looked and continued to work. Probably, 3 times a day I get distracted from my tasks for review. But, again, you have to understand, in my team everything is divided into small pull requests, 200-300 lines of code each. Accordingly, a minimum of time is spent.
"Who reviews is less important than whom"
Konstantin Burkalev: This begs the question - who should review. Lead only? Only the senor? Or can and should be given to someone lower in competence, just so that a person tries to grow?
And when asked what to review, people answered like this.
Alexander Makarov: If you have a lot of people in your team, and the lead has created a bottleneck out of himself, it slows down the whole team and as a result makes the team much worse. As a lead, when I worked at Skyeng, I had 15 pull requests a day at their peak, and not the smallest ones. I set aside time for review in the morning and evening.
It is necessary to review everyone. Except, perhaps, for stories where "fire, everything is on fire" - it won't get any worse there.
I'm messing up, it's okay - even though I've been using the same project for many, many years. Therefore, now I am trying to invite the maximum number of people to watch my request. This is good and it should be.
It's another matter whether everyone should trust the review. I had guys who could figure out great, but they had big problems with focus: and, say, one spent 6 hours on a review. I had to teach people how to manage their time.
If we are talking about commercial companies, I am in favor of identifying who has this responsibility and who can review at will - and how much time per day you can spend on review, depending on this status.
Anton Morev:As I see, there are different processes. If we are doing some feature that needs to be released in a short time, we are not up to letting June see the code. But if we are doing some kind of internal product or the deadlines are not high, yes, it's a great practice to let June review what the lead or senior developer did. So the Juns will better understand what is happening in the project as a whole and how the decision-making mechanism works.
"This is a reject right away"
Sergey Zhuk: Skyeng implemented a check for a commit message: there must be a task number in JIRA, otherwise the pull request cannot be merged. Sometimes, you open the code, you just don't understand what's there - you open a task and you can understand something.
Otherwise, at first it was tough, then we decided to reject it only if the task was completely wrong, or the team architecturally disagrees. And so: we put an approval, merge, write a comment - and if the author of the pull request wants, he will return and fix it. There, he will correct small roughness or use some kind of pattern. What are your reject practices?
Alexander Makarov: The criteria completely coincide with yours - if the task is not done well, of course, you have to wrap it up. Even if the code looks fine and architecturally everything is cool.
, : , .
For example, we say: “Guys, let's take a test”. There are exceptions, of course, but tests are very important. It is clear from them whether the person understood the task correctly: again, if he tests classes and methods, and not a use case, this is already suspicious. We have now screwed up infection , this is mutation testing. Tulza leaves the same tests, but modifies the code and runs. And if the changed code passed with the same tests, then part of the code is not needed - you can just take it, delete it, nothing will change.
A bit of backstage.
Also, of course, security and performance issues - there will be a rejection here. We do not reject trifles: either we ask to fix it, or we fix it ourselves - we push directly into the pull requests of those who made them, and we already show on the finished code what, how and why we did it.
Anton Morev:Regarding what you said - is the problem solved? It happens that a developer, while solving one problem, has solved another. It is not necessary to reject such situations. Or at least figure out how the task was moderated.
Konstantin Burkalev: But the moment of linking commit messages with a task tracker seems important to me. There are everyday tasks in which it helps a lot. Do you know when: “Listen, we did something similar within the problem about the button”. You find the task about the button, understand the number, go to the repository log, find the diff of those commits and see: indeed, we have screwed this and that, it can be moved here.
But the opposite also happens. I am looking at the source code of one project here and I come across the action684 function in the backend.
I write to a friend, I ask, what is this cool name in the code. He answers - a reference to the task in the tracker. “Why come up with a name for a function, you’re writing it as part of a task” ... Thresh, which should not exist in a normal world, of course.