What is the point of this review?
Do you do code reviews on your current project or do you think it's just a waste of time?
At interviews, I sometimes ask why code reviews are conducted. You may be surprised, but one seasoned senior, with 10+ years in game development, and who also got an A in embedded development for the impudence of receiving a red diploma, answered “because everyone does it” – this is, of course, a rare, neglected case, but it happened. Another thing that happens is “because the sensei said so” (lead, grandma, azhal boss – substitute your own). Another percentage of people answered, “because that's how they do it at Google”, and when, mind you, this is not the first person who seriously says this at interviews, I begin to doubt the ability of our tech-HR to conduct an initial interview, and he, it must be said, sometimes also writes product code and regularly undergoes these same reviews along with the entire development team. Surely the people who came shouldn't doubt?
For what?
We shouldn't do something just because “everyone else does it” – at least that's what my parents taught me when I was a kid. Sensei-Principal usually doesn't just say things and force you to follow development rules, there were good reasons for that in the form of bugs and nights of crazy debugging before that. He has all these principles of developing quality software written in zeros and ones on top of the gray matter. And, of course, doing something just because it supposedly corresponds to agile, scrum or some waterfall, looks more like a cargo cult than meaningful actions of developers with shippy projects.
Code Review is not only and not so much about code, but about people, team and interaction, and also about money. If you ask your lead whether a code review is necessary, he will answer – of course, because a mistake missed during the review is his headache, a task delayed in the future or a new bug. On the other hand, if you approach the producer with this question, especially before the milestone – yes, in agile he will turn all these code reviews of yours – a week before delivery, a wagon of tasks and two trains of bugs – features need to be sawed, and not practiced in the epistolary genre and write letters to each other. Developers' time and the company's money are key things, if you look at the reasons for conducting a code review.
Time is money when we are professional software developers, and games are very complex software. It may not be medical equipment or trading terminals or even space software, and a game crash does not lead to someone's death or multi-million dollar losses, but a bad game simply leads to the closure of the studio, as has happened more than once. And code review takes time, often several times more than was spent on writing the code. It's like ABS in a car, ABS does not help to brake, but makes the car controllable when braking, while preventing braking with maximum efficiency.
If you don't do it properly, that time will be wasted. If you don't do it effectively, and by effective I mean overly strict requirements, there will be some benefit, but will it be worth your time?
How?
And when candidates ask about our code review process, I start by answering the question of why this is all being started. In most cases, we can now afford not to conduct it at all, with well-established tests and a normal QA department, the total time for changing the code and fixing possible errors is less than writing code + review iterations.
Higher speed of feature iteration and their delivery to the end user, in our case game designers, faster field testing and bug fixing. Such is the paradox, without code review the efficiency of work becomes higher, but, as they say, there is a nuance, the quality of the code as a whole begins to fall. Therefore, the main reason why we conduct code review is to maintain the “health” of the code. This is the earliest stage of control over changes in the project architecture. And the sooner we detect possible problems at the architecture level, the cheaper it will be to fix them, or at least prepare for possible problems.
There are other approaches, even before coming to game development, I had to work in a couple of Central Research Institutes, developing various kinds of underwater fishing complexes of varying importance under the supervision of gray-haired professors, SNS and prochik KTNS, where a week of “code inspection” was the order of the day, after all, the junior research fellows had to be occupied with something, otherwise they all drink tea and sit at their computers and play solitaire. A couple of particularly distinguished ones were selected from each department, who were sent to one large room and given printouts of programs and had to debug this in their heads and review it with notes in the margins. The especially lucky ones got old computers, luckily with the third stump and the ability to run lines. At the end of the week, it was necessary to submit a report on the errors found in the algorithms, write tests for the functions, by hand, damn it, on a piece of paper. It was terrible, because you drop out of your project for a couple of weeks: you inspected the code for a week, and then got back into your own and your colleagues' changes for another week. There were some positive moments, though, some errors were found during these “code review raids”.
Why?
Today we have autotests, unit tests, design tests, functional tests, integration tests, level tests, “red and blue” QA teams, there is even ONE qa engineer who is trying to teach LLama to play our game, llama really doesn't care about all these attempts, but we believe in you, Sanya. All this allows us to catch a lot of errors that could only be detected manually before.
But this whole system of tests, which tortures the code in module and acceptance test hell for two hours per merge with the regularity of Fabien Barthez, misses errors. It is not the tests' fault, they only catch cases that we thought of, there is a small influence of test coverage synergy, but in the experience of other teams, such coverage should be close to 80% for it to start showing results, but unfortunately not on this project. The reviewer can point out places for which there are simply no tests. Sometimes it also happens that the same erroneous logic in our program is superimposed on errors in the tests themselves, and voila – the commit slipped through and broke the build. Who is to blame for the build not being built? – the programmer, of course.
Another reason for code review is the efficiency of algorithms. But this is not a spherical O(n) in array enumeration, usually this is an expertise from a person who is immersed in the system and knows its nuances, how efficient the code will be in a live system. At the very least, you should not try to estimate performance during code review, you will take into account at least a dozen factors out of hundreds that really affect. Only a profiler can do this while the entire application is running. And with aggressive compiler optimizations, the probability of correctly predicting how the optimizer will change the new code tends to zero, well, except for perhaps the simplest cases.
Understanding and readability of code is probably one of the aspects where code review is really useful. No sophisticated code analyzer (PVS, Sonar, CodeCube) can determine how readable, supportable and consistent a given code fragment is with the style of a team, studio, or department. In addition, the author of the code keeps most of the algorithm and interactions of the system in his head, and it is rarely possible to simply and clearly transfer these nuances to the code. It is especially difficult with variable names and code structure, everyone on the team has their own vision, and no two will be the same, so compromises have to be made.
The more important is the readability of the code when teaching colleagues, understanding the technical aspects of systems, or applied design patterns. The best part of code review is the opportunity to look at the code without pressure from the owner of a feature or bug, to study the code without the burden of contextual knowledge and notice things that both the author and the reviewer did not know before, discuss them and find the best possible solution. Moreover, the learning process works both ways: if I look at someone else's code, it is an opportunity to show how other techniques or language capabilities can be used, if mine is criticized, then the comments and knowledge I receive will also stay with me because they will be used during edits.
What is even more important than readability, time and money, and what review time is really spent on, is the interaction within the team, after the changes are accepted, the responsibility for the code no longer lies with one person. Having another fellow mind who read (and most importantly understood) makes the code the property of the team, I really want to believe in this.
Who?
Another implicit feature of code review is the opportunity to talk about refactoring. For a moment, we conduct code reviews to make the changes we make readable, understandable, and maintainable. Hmm, but refactoring is also done to make the code more readable and maintainable. And only another sommelier can assess how much less smelly the new code has become. As with new code, there may be new names, new class structure and design, and often new dependencies between parts of the architecture.
During my time working on different projects, I encountered different approaches to the code review process. A couple of times we sat down as a team and discussed which changes should be sent for code review and which should not. This was another step towards understanding and acquiring a good habit of creating atomic commits, which helped many times later. Not only in pet projects, but also within the team, you have to convince people to make atomic, small and easily revertible commits.
Some of my colleagues are very critical of the code review process, both in relation to themselves, painfully accepting comments, and in return, having the opportunity to criticize each other. Of course, a certain healthy part of criticism should be present, without it there will be no progress in learning. But it is important to understand that this is not all. As I wrote above, first of all, we conduct code reviews to maintain the “health” of our code and secondly to learn from each other. However, admitting that your code needs to be checked and, possibly, improved is not always easy. We write the code ourselves, and other people with different ideas and vision of the architecture check it.
And convincing some team members of the benefits and necessity of code review is sometimes very, very difficult. Making it a mandatory part of the process is not an option, the author of the commit will still see the reviewers as malicious “breakers and improvers”, and the reviewers will perceive this process as a labor duty and a waste of working time. I have seen developers imitate the review process more than once: those who do not like reviewing code, or only like to write their own code, but do not delve into someone else's and understand someone else's logic, simply called “friends” who formally approved their code or wrote minor comments to create the appearance of verification. I repeat that it often takes more time to delve into and understand the changes than to write them.
Management often views this messing around with code as a waste of time that can be the first to be cut when time pressure arises, and this is often the case. As with other “quality” project maintenance techniques, code review time is the first to be cut, similar to not refactoring or writing tests. But rest assured, not doing code review leads to x2 bugs and brings closer a week of guessing on heisenbugs when it's time to pay off technical debts.
Once there was a difficult milestone before sending a build, various tasks that were not mine took up so much time that by the end of it 52 hours were tracked for code review. The conversation with the lead and PM afterwards was not very pleasant, I was happily slacking off on my own tasks during this time, judging by the report in Jira. Conversations about the importance of code review were also received coolly. That time I got off with an “ouch-ouch-ouch” and a fine minus two days of vacation.
In any case, code review has long been a part of our development pipeline, and we train all newcomers to comply with it. But sometimes there are excesses, when, for example, a PR arrives for a month of work, including dozens of new files and thousands of changes. The commit was necessary because it introduced a transition to a new NPC behavior system (Behavior Tree). Considering that the attention of an ordinary programmer is not unlimited, you can well check a dozen files and keep in mind the connection of about a hundred changes. Anything beyond this will definitely slip away. How much time did it take to check? I checked ten files out of 320, spending about two days on this, then I just voted up.
Must?
After that incident, I began to honestly warn my colleagues that I do not review commits larger than 10 files and hundreds of changes, either the commit is smaller in size, or auto-rejected, or not me. Small commits have their own aesthetics. I don’t remember which of the software engineering masters derived the dependence of review time (I think it was McConnell in Code Complete) – the feedback time is the square of the number of changes per line analysis time (from one to 5 minutes).
He also introduced the concept of review context. It is very wasteful of time to look through a huge sheet of code without any comments, to understand what this code should do and how it works. If your colleague is at the next table, it will not be difficult to sit down together and look through and discuss the changes, but when this is not possible, the function on four screens becomes a real headache. Therefore, recently, in addition to the changes themselves and tests for them, we also require context, the presence of tests and a correctly formatted description.
You might say that the purpose of a code review is to check changes, and not to be able to write a short essay – “how I fixed this this summer” to a PR. But think about it, a commit description is the finalization of a task, and if you can't formulate in five or six short sentences what exactly was done, then there is reason to think about whether everything that needs to be done in the task has been done.
When?
Less than 1,500 characters passed before we got to the code review itself. As I noted at the beginning of the article, code review time is the author's time, and maybe more than one. Time for automated tests, QA time for checking changes, time for rollback or finalizing changes.
The code author waits for feedback and plans his steps based on what was sent for review. In addition, if there are any comments or even discussions, they are better perceived and recorded when the code was written recently. I do not urge you to drop everything and look at the changes as soon as they come, but by postponing this until tomorrow you shift the overall execution time by at least a day. If there is no time, it is better to say so and invite another person responsible for the review. Indirectly, by the lack of time for code review, you can judge the overall workload of a person; if he often refuses or delays reviews, perhaps he has too many tasks.
Don't delay checking your code, this is a good opportunity to use pomodoro time10-15 minutes at the end of the hour to distract yourself from the current task and give your brain time to rest. And you also need to learn to trust your colleagues so as not to delve into complex algorithms. Again, I judge only by my colleagues, trust appears after one and a half to two years of working together, by trust I mean the ability to give a task and look at the code diagonally. Good tests will reveal algorithm errors, and you will not have to spend additional time to understand every detail of its operation (while the code should still be readable and understandable). But on the other hand, you need to devote enough time to checking and not trust your colleagues completely, relying on their authority. Gurus often do not write the cleanest and most correct code. They are also people, they also make mistakes, and they also benefit from feedback from other specialists.
Other things you might find in a code review include obvious errors, ambiguous tests, bad algorithms, and unclear code that needs to be documented with a comment if it can't be done with names or code structure.
But there are things that the team and I agreed will be taboo, like formatting, style code, or long detailed discussions. If you still write nits about an incorrectly placed asterisk, closer to the type, or next to the name, then you should probably look at autoformatters – when spaceships plow the expanses of the Bolshoi Theater, it is quite possible to give such trifles to tools. This saves up to half the time on review, and allows you to focus on analyzing the algorithm, rather than looking for an incorrectly docked ampersand.
We also tried to introduce the practice of one-hope review a couple of times, when approval is reached with the author of the code regarding minor comments that can be corrected with a few keystrokes and uploaded without a secondary approval. But obvious errors, lack of tests and everything else that requires more serious changes should lead to rejection. But such a system did not take off, people used this loophole to quickly enter changes into the master, as a result, this led to a greater number of untested changes and an increase in the number of errors.
The result?
Developers are a strange bunch. Despite writing logical algorithms and complex logic, programming itself can be emotional. We take pride in writing Code with a capital letter, especially when we solve a complex problem beautifully, in a dozen lines and in an unusual way. Our code can be so beautiful that many begin to consider it art. Code reviews can be seen as a test of our ability to write Code, no matter how much we convince ourselves that it is just a process of checking it. We may believe that the project code is the property and responsibility of the team, and not a specific developer. But our code reviews are the moments when we pass on our own creations to the rest of the team. If it is rejected, it can leave an unpleasant feeling, especially with less experienced colleagues. And our petty grievances become the cause of unnecessary nitpicking in other commits.
The most important thing in a code review is to check the code, not the developer. You can find fault with the code, but not with the position; seniors sometimes screw up worse than juniors. A good way to not seem too harsh is to ask questions, rather than assert that the code sh…but bad, but if the code is real sh…but such is the case, then do not be afraid to say so, but with compelling arguments and an example of a good solution.
Good Coda and one-hope review to all.
