About code review
About code review
I want to talk a little bit about the practice of code review. On the one hand, the practice is well known, mandatory and widespread in different agile environments. But I would like to study its usefulness as well as possible ways of improving it.
I want to talk a little bit about the practice of code review. On the one hand, the practice is well known, mandatory and widespread in different agile environments. But I would like to study its usefulness as well as possible ways of improving it.
There are several team development stages and each one may implement a certain code review form. Some teams have only read about such a miracle in wise books but in other enlightened teams code review is a mandatory procedure preceding code check-in — without a review a big, bad bearded architect will come and architect the hell out of the slack coder:).
But if the practice is applied, it is not guaranteed to produce the benefits which are so vividly described in the wise books by flexibility gurus. So, what might be the problem?
In the second place, the style check is followed by the “I would not put it this way” syndrome. Because there are always differences in terms of creating code when it comes to programmers. The same goes for approaches to solving identical tasks. Here it is very important to understand in which way the current solution is better/worse than the offered one because otherwise no one will remake the current solution, there will be a lot of fighting and the result is likely to be negative.
In the third place, the reviewers may simply lack the context for a full-fledged review. If a hero pattern flourishes in the team when a separate code piece is owned by a separate person, the others will simply lack the understanding of the current issue that might allow them to provide useful remarks. If comments do follow, the author can use his experience to express that everything was fine and the reviewer simply finds faults.
In the fourth place, it's already too late. The problem of any review implies that it is conducted after the completion of the task, i.e. the main development investments have already been made and the heat of the moment is kind of gone, and other tasks are waiting in backlog, and there is a strong desire to check the code. This is why the reviewer is unwilling to divide the classes into simpler and smaller ones, to assign liabilities for methods and improve readability of tests and other code.
Also, a review may produce pathological cases of never-ending iterations. When one of the reviewers wants the solution to look the other way, he or she may begin a range of “iterations” where the author tries to accommodate his or her decision to the view of one of the reviewers (it may be the view of a leading developer or architect) but he or she fails, once again, due to the fact that his or her brain is located outside the analogous device of the reviewer. The end result is long and inefficient, both for the author and the commentators.
What should be done to a review so that it is more efficient?
In order for a review to be efficient, all the participants must want this and they have to treat the review as a tool and not as an end in itself. Below are some issues/pieces of advice aimed at making this process more efficient.
First of all, it is preferable to work in pairs for difficult tasks at least in some moments or on the key part of the task, e.g. design. It will imply a “shared” code/solution ownership, which will simplify the review in several ways: the author will not take the criticism to heart as the liability will be shared. The decision will be of higher quality as work on it will involve 2 pairs of eyes and a couple of brains. And a brave co-pilot will be able to provide backup during a review and protect the author's ideas making it clear that they are not random but quite carefully thought out.
Second of all, in difficult cases and with no partner it is reasonable to conduct a review design prior to solving some tasks. It will allow people to reduce the risks of solving the wrong task or offer the wrong solution for the correct task. Then again, the availability of context after the design solution will allow you to make valid comments during a code review as all the reviewers will be aware of the problem and, at least partially, know the solution.
Third of all, it is reasonable to automate the style issues so they do not arise during the review. It will remove the temptation to make off-topic comments, which is useful for keeping the dialogue in comments from turning into a flame.
In addition, one needs to establish formal or informal limits and rules of play. For example, different tools make it possible to express one's actions via comment types and status. If you continually close your comments by setting a Won't fix status, it will look rather aggressive. In the same way, if a review tool allows “vetoing” a review, it may totally sour relationships. Instead of doing this, it's better to simply write/call/approach a person and discuss the issues about his or her solution in person. Verbal communication is generally a good dispute-solving tool compared to endless comments.
And, finally, it is important to understand the purpose of the entire thing. The purpose of a review is not to demonstrate the superiority of an architect over coders but to make the world:) and the team better. Reviews help spread knowledge, patterns and approaches to different task solutions. This exchange of experience prevents lame solutions because other members of the team know which problems will follow if the current solution is made.
But, in general, as with any tool, code reviewing will be useful if it is used according to the designed purpose in a team with skills. It means that in case of difficult tasks one needs to work in pairs, even for a short time, apart from reviewing. If not implemented, one needs to conduct design review or at least describe one's insights and rationale when publishing the review. And it is important to prevent rot in the team and exchanging nasty comments, for nothing good will come of it.
Sergey Teplyakov
Expert in .Net, С++ and Application Architecture
There are several team development stages and each one may implement a certain code review form. Some teams have only read about such a miracle in wise books but in other enlightened teams code review is a mandatory procedure preceding code check-in — without a review a big, bad bearded architect will come and architect the hell out of the slack coder:).
But if the practice is applied, it is not guaranteed to produce the benefits which are so vividly described in the wise books by flexibility gurus. So, what might be the problem?
In the second place, the style check is followed by the “I would not put it this way” syndrome. Because there are always differences in terms of creating code when it comes to programmers. The same goes for approaches to solving identical tasks. Here it is very important to understand in which way the current solution is better/worse than the offered one because otherwise no one will remake the current solution, there will be a lot of fighting and the result is likely to be negative.
In the third place, the reviewers may simply lack the context for a full-fledged review. If a hero pattern flourishes in the team when a separate code piece is owned by a separate person, the others will simply lack the understanding of the current issue that might allow them to provide useful remarks. If comments do follow, the author can use his experience to express that everything was fine and the reviewer simply finds faults.
In the fourth place, it's already too late. The problem of any review implies that it is conducted after the completion of the task, i.e. the main development investments have already been made and the heat of the moment is kind of gone, and other tasks are waiting in backlog, and there is a strong desire to check the code. This is why the reviewer is unwilling to divide the classes into simpler and smaller ones, to assign liabilities for methods and improve readability of tests and other code.
Also, a review may produce pathological cases of never-ending iterations. When one of the reviewers wants the solution to look the other way, he or she may begin a range of “iterations” where the author tries to accommodate his or her decision to the view of one of the reviewers (it may be the view of a leading developer or architect) but he or she fails, once again, due to the fact that his or her brain is located outside the analogous device of the reviewer. The end result is long and inefficient, both for the author and the commentators.
What should be done to a review so that it is more efficient?
In order for a review to be efficient, all the participants must want this and they have to treat the review as a tool and not as an end in itself. Below are some issues/pieces of advice aimed at making this process more efficient.
First of all, it is preferable to work in pairs for difficult tasks at least in some moments or on the key part of the task, e.g. design. It will imply a “shared” code/solution ownership, which will simplify the review in several ways: the author will not take the criticism to heart as the liability will be shared. The decision will be of higher quality as work on it will involve 2 pairs of eyes and a couple of brains. And a brave co-pilot will be able to provide backup during a review and protect the author's ideas making it clear that they are not random but quite carefully thought out.
Second of all, in difficult cases and with no partner it is reasonable to conduct a review design prior to solving some tasks. It will allow people to reduce the risks of solving the wrong task or offer the wrong solution for the correct task. Then again, the availability of context after the design solution will allow you to make valid comments during a code review as all the reviewers will be aware of the problem and, at least partially, know the solution.
Third of all, it is reasonable to automate the style issues so they do not arise during the review. It will remove the temptation to make off-topic comments, which is useful for keeping the dialogue in comments from turning into a flame.
In addition, one needs to establish formal or informal limits and rules of play. For example, different tools make it possible to express one's actions via comment types and status. If you continually close your comments by setting a Won't fix status, it will look rather aggressive. In the same way, if a review tool allows “vetoing” a review, it may totally sour relationships. Instead of doing this, it's better to simply write/call/approach a person and discuss the issues about his or her solution in person. Verbal communication is generally a good dispute-solving tool compared to endless comments.
And, finally, it is important to understand the purpose of the entire thing. The purpose of a review is not to demonstrate the superiority of an architect over coders but to make the world:) and the team better. Reviews help spread knowledge, patterns and approaches to different task solutions. This exchange of experience prevents lame solutions because other members of the team know which problems will follow if the current solution is made.
But, in general, as with any tool, code reviewing will be useful if it is used according to the designed purpose in a team with skills. It means that in case of difficult tasks one needs to work in pairs, even for a short time, apart from reviewing. If not implemented, one needs to conduct design review or at least describe one's insights and rationale when publishing the review. And it is important to prevent rot in the team and exchanging nasty comments, for nothing good will come of it.
Sergey Teplyakov
Expert in .Net, С++ and Application Architecture