Translation: I hate you: your code is trash!
I want to submit my translation of the article “Your Code Sucks and I Hate You: The Social Dynamics of Code Reviews” .
I hate you: your code is trash!
Jonathan Lange, 09/15/2008
Code revision is a really useful, but at the same time, incredibly frightening procedure. This article will tell you how to avoid "fist fights" during audits.
')
We briefly consider why the code should be audited, and focus on the question of how the relationships between the process participants are formed, especially in open source projects. Indeed, in part, open source attracts (and sometimes, on the contrary, scares away!) People precisely because your code will be viewed by experts from around the globe. We will also look at the impact that some existing technologies have on the code revision culture, consider what can be achieved with their help, and how revisions are conducted in other areas of activity. We will also denote some of the “pitfalls” of revisions that are easily overlooked.
Code revisions are known for a long time. They are important companions of the free software movement from the very beginning in the form of, at a minimum, the right to check a patch before accepting it.
In recent years, code revisions have gained new development. For example, supporters of extreme programming promote programming ideas in pairs (that is, in fact, programming with continuous verification of the code), and many open projects require that all proposed patches, regardless of the reliability of the author, go through an inspection process.
Each project has its own approach to revisions: different tools, different procedures are used, different goals are set. And therefore the impact on the developer community, they have the most diverse.
In this article, we will look at the decisions made by some open projects, as well as the difficulties created in these relationships in the relationship and the additional incentives they provide to developers. We will also identify potential problems and give some recommendations for fixing them.
This article does not pretend to be severely academic. This is only a set of theses, designed to stimulate further research and experimentation.
In short, code revision is what happens when someone scans your code and comments on it.
Highlight some types of revisions.
First of all, it is worth mentioning the preliminary reviews , i.e. revisions of patches before they are added to the main branch of the project code. A classic example of such revisions in the free software community is the placement of a patch by a new developer on a mailing list. In some projects, preliminary reviews even apply to code experienced developers (for example, Twisted, Bazaar, Linux).
Note also the post-factum code revision . This often happens in free software projects when someone enters a non-trivial code into a project. Usually, the reviewer, having found an error in the patch, notifies about it by publishing (reply) to the message on the commit message of the corresponding change.
Audits can also be conducted in the form of a random check . With this approach, the senior programmer inspects some arbitrary sections of code in the repository and leaves comments on its structure and quality.
Finally, revisions may be unplanned . This usually happens late at night, when a programmer is messing with code in some far corner of the project, possibly catching errors there. Such revisions are usually accompanied by obscene exclamations and angry comments. Often they are completely useless for the author of the code, since he is unlikely to be lucky to hear them.
Some code revisions seem to be a bit like writing documentation, designing through testing or eating bran: it's good if you manage to do these boring things less often than required.
However, unlike fiber-rich cereals, various code review techniques provide sometimes conflicting benefits, and you need to clearly understand which of these benefits are really important to the project.
Audits are indispensable when it is necessary that the entire source code looks consistent. This includes naming conventions, spell checking in comments, and consistency when using internal APIs.
A preliminary review of the code makes it possible to detect errors before they start to bother users. The auditor is more emotionally removed from the code than its author, and therefore more able to subject this code to criticism and, therefore, can detect more errors.
If a piece of code is viewed by someone other than its author, it means that there are at least two people in the world who are able to understand it.
Again, the auditor is intellectually more distant from the code and therefore can see the hidden assumptions and controversial decisions laid down by the author.
A clear and understandable code allows beginners to quickly begin to make their own additions to it, which is extremely important for open source projects, since they completely rely on the contribution of voluntary participants.
If all of the code being entered passes through a preliminary review, then each such procedure gives the reviewer the opportunity to study that part of the project code that he may not have been able to look into before. Thus, the verification procedure contributes to the dissemination of knowledge about the project code among all its participants.
However, reviewing the code with a “fresh look” of the reviewer does not always have a positive effect: sometimes it can really bring additional clarity, and sometimes it will be too superficial.
Since practically every developer has his own set of priorities, then the reviewer's priorities will differ from those of the author. For example, if the author proposes changes that increase the performance of the basic API functions, then it is likely that he will have to prove that this is more important, say, than backward compatibility. Thanks to preliminary reviews, these disputes will arise before the changes fall into the main repository, and not after.
Checking your own code by a programmer is an insight into the essence of your thinking, and prolonged immersion into the thought of another programmer expands your own thinking abilities. Reviewers may suggest techniques that the author has not heard of. They will ask questions that the author did not think about. They will indicate a simpler path that he did not notice.
On the other hand, revisions also encourage the reviewer to explain exactly what he understands by good code.
The code revision process changes depending on which of the listed goals are most important. If revisions are aimed primarily at ensuring the quality of the code, then they should be lengthy and thorough, since reviewers will try to detect errors, performance failures, and the like. If revisions are intended to increase the clarity of the code, they will be shorter, and reviewers will assume that the code is already sufficiently tested and justified.
When implementing the code revision procedure, some questions arise that require answers. Below we will look at such questions and see how they were answered in the projects Bazaar, Launchpad and Twisted.
Perhaps the most important question that every project should answer is who will perform the code revisions. In most open source projects, reviewers are chosen from among the main developers, although the mechanisms of this choice may vary from project to project.
In the Bazaar project, patches must be approved by two main developers (i.e., those who are explicitly given the right to place the code in the repository). This option ensures that all patches will be viewed by someone with sufficient experience, and also provides the necessary level of communication between developers.
The Launchpad has a dedicated review team in the development team. The group aims to include as many developers as possible, however, to get a full review right, you must go through the training procedure with a mentor. This procedure ensures that, in case of doubt, the new reviewer will be able to contact his mentor, and that the project’s priorities (code clarity and review speed) will be supported by all reviewers.
In Twisted, anyone can do a revision, except for those people who participated in the creation of the patch. This ingenious idea stimulates the group thinking of the author and reviewers. On the other hand, this leads to confusion in the reviews of the reviewers, and the reviews of the code themselves, being a “common cause”, can quickly turn into a “nobody's business”.
In Twisted, revisions are based on bug reports. Bazaar conducts audits on the general mailing list. Launchpad uses a separate mailing list for this.
The Twisted approach is a typical example of the UQDS (the Ultimate Quality Development System, universal quality assurance system). Inspections are performed on the basis of requests, and the web page with the request becomes the focal point of all information on correcting errors or adding a new function. In addition to centralized storage, this method allows to exclude from participation in the discussion of those who are not involved in solving the problem. However, there is a big drawback: reviewers' reviews are rarely read by someone other than the author of the patch, which leads to the application of different coding standards in each particular case. When a really important question arises in a review, it can be very difficult to attract everyone’s attention. Besides, the unsuccessful system of mailings in Trac complicates tracking a discussion course.
In Bazaar, reviews are sent directly to the general mailing list and are taken into account in the Bundle patch management system. Although reviews and patches are sent directly to developers by e-mail, discussions are usually much more open than Twisted. However, a large number of patches make it difficult to read the mailing list, and review discussions can be long and florid.
Launchpad has a separate mailing list for conducting audits, which makes it easy to separate their discussion from general development issues, but still allow outsiders to view this mailing list. In practice, the project receives the advantages and disadvantages of both approaches.
Conducting "asynchronous" revisions is very different from real-time revisions.
When conducting an off-line audit, for example, via e-mail, the reviewer has time to formulate his comments diplomatically, and the author can respond to them in any manner convenient to him.
The “real-time” revisions repeat all the advantages and disadvantages of a regular conversation: misunderstandings are easily detected, and there is no long wait for a response. However, passions can quickly heat up, which makes it difficult to separate facts from emotions. It is also more difficult to determine the duration of such revisions. Having received the letter, you can always see its size in the mail client, and real-time communication can continue indefinitely.
It happens that the author and the reviewer have opposite views on the code and can not come to an agreement. For example, the author believes that the following code
is an obvious way to get a single value from a list containing a single item. The reviewer considered this unobvious and suggested the following:
Despite mutual arguments, the parties did not come to an agreement. What to do? Who will solve the contradiction?
In Bazaar, the author of the code has the right to say the last word, in Twisted, on the contrary, the last word belongs to the reviewer, while in Launchpad, a specially appointed main reviewer has the decisive voice in such matters.
If the “last word” is provided to the author, then the programming process will go faster due to losses in uniformity and, possibly, quality. If the last word is for the reviewer, then the additions will be implemented more slowly (the reviewer is not so obsessed with the desire to make a patch as its author), but will be thoroughly studied.
There is practically nothing to add: inspect the program , not the programmer . Remarks about his personality only complicate his perception of criticism.
Advice: if you write negative comments, write “patch” or “code” instead of “you”. For example, instead of “You have a glitch in
If all you can say is “There is a
The author should be able to unambiguously identify and understand every single thought in your remark, as well as the entire note as a whole. Reviews with fuzzy results turn into endless discussions about code, which are more likely to give pleasure to the reviewer, rather than improving the quality of the code.
This is a problem of observers in any field: film critic, literary editor, scientific reviewer. Not necessarily really good is what I like. And even more so it’s not really bad that I don’t like. One of the aspects of the formation of an outstanding programmer is the modeling of his own preferences in accordance with real, objective advantages.
If you are reviewing a patch that perfectly fixes a problem using the paradigm of imperative programming, then you shouldn’t criticize it just because it doesn’t follow the functional paradigm. So you discredit the idea of revisions, turn it into a game of guessing the preferences of the reviewer, and it should help create a really good code.
The formulation of comments in the form of questions will help the verifier to avoid such an oversight, for example: “Did you think to use the functional style?”, “Why do you not use regular expressions to solve this problem?”, Etc.
See also “A clear answer is a good answer.”
When reviewing the code, there is always the temptation to ask the author to fix some other problems at the same time. Such an approach can quickly develop into a full-scale glossing operation over a large part of the code, although initially the author and the reviewer had to approve only a small improvement.
The best advice in this situation: to cherish a phased improvement. See "The best is the enemy of the good."
Filibustering is an American political term for delaying the debate on a bill to prevent its adoption. Sometimes this is done in free software projects to sabotage the adoption of a particular patch.
"Filibuster" may also arise unintentionally. The person conducting the revision does not accept the patch due to the absence of block tests, the author asks how to write the block tests for this code, and the reviewer does not bother to answer. But in addition to these troubles, the author also has his own affairs that prevent him from continuing to send new patches.
See “Foggy review summary” and “Quick response - good response.”
Specify specific lines of code that can be improved. Describe what's wrong with them. Suggest a way to do better. Help the author to define the limits of improvement.
Once the author has submitted his patches for revision, he can no longer work on them. He must wait for the auditor’s response before continuing. Write your comments to the review as soon as possible so that the author’s attention does not have time to dissipate.
You can not make a perfect patch that would solve all your problems at once. Do not pursue the ideal, work on a phased improvement.
Someone tried to improve your product. They have spent their strength and imagination to help you, and now they have submitted their work to the public: thank them so much.
In the Divmod project, the principle of including at least one praise in every review of the code is practiced. You can always find what to praise the author for, even if only because he undertook to work on this part of the code.
The reviewer should ask questions. You can not be mistaken by asking questions. In the worst case, the author will give a trivial answer. At best, both the author and the reviewer will discover something new.
Code revisions give a person a unique opportunity to grow as a programmer, while improving the product he creates. Each project can choose from many options for conducting audits and the opportunities they provide. Thoroughly thinking about technologies and processes of interaction of people in conducting surveys will allow open source projects to avoid the difficulties that frighten beginners and tire long-standing participants, and focus on developing the best software.
PS Moved to the "Revision Code" blog.
I hate you: your code is trash!
Relationships of Code Revision Participants
Jonathan Lange, 09/15/2008Overview
Code revision is a really useful, but at the same time, incredibly frightening procedure. This article will tell you how to avoid "fist fights" during audits.
')
We briefly consider why the code should be audited, and focus on the question of how the relationships between the process participants are formed, especially in open source projects. Indeed, in part, open source attracts (and sometimes, on the contrary, scares away!) People precisely because your code will be viewed by experts from around the globe. We will also look at the impact that some existing technologies have on the code revision culture, consider what can be achieved with their help, and how revisions are conducted in other areas of activity. We will also denote some of the “pitfalls” of revisions that are easily overlooked.
Introduction
Code revisions are known for a long time. They are important companions of the free software movement from the very beginning in the form of, at a minimum, the right to check a patch before accepting it.
In recent years, code revisions have gained new development. For example, supporters of extreme programming promote programming ideas in pairs (that is, in fact, programming with continuous verification of the code), and many open projects require that all proposed patches, regardless of the reliability of the author, go through an inspection process.
Each project has its own approach to revisions: different tools, different procedures are used, different goals are set. And therefore the impact on the developer community, they have the most diverse.
In this article, we will look at the decisions made by some open projects, as well as the difficulties created in these relationships in the relationship and the additional incentives they provide to developers. We will also identify potential problems and give some recommendations for fixing them.
This article does not pretend to be severely academic. This is only a set of theses, designed to stimulate further research and experimentation.
What is code revision?
In short, code revision is what happens when someone scans your code and comments on it.
Highlight some types of revisions.
First of all, it is worth mentioning the preliminary reviews , i.e. revisions of patches before they are added to the main branch of the project code. A classic example of such revisions in the free software community is the placement of a patch by a new developer on a mailing list. In some projects, preliminary reviews even apply to code experienced developers (for example, Twisted, Bazaar, Linux).
Note also the post-factum code revision . This often happens in free software projects when someone enters a non-trivial code into a project. Usually, the reviewer, having found an error in the patch, notifies about it by publishing (reply) to the message on the commit message of the corresponding change.
Audits can also be conducted in the form of a random check . With this approach, the senior programmer inspects some arbitrary sections of code in the repository and leaves comments on its structure and quality.
Finally, revisions may be unplanned . This usually happens late at night, when a programmer is messing with code in some far corner of the project, possibly catching errors there. Such revisions are usually accompanied by obscene exclamations and angry comments. Often they are completely useless for the author of the code, since he is unlikely to be lucky to hear them.
What are code revisions for?
Some code revisions seem to be a bit like writing documentation, designing through testing or eating bran: it's good if you manage to do these boring things less often than required.
However, unlike fiber-rich cereals, various code review techniques provide sometimes conflicting benefits, and you need to clearly understand which of these benefits are really important to the project.
Increased coherence
Audits are indispensable when it is necessary that the entire source code looks consistent. This includes naming conventions, spell checking in comments, and consistency when using internal APIs.
Quality assurance
A preliminary review of the code makes it possible to detect errors before they start to bother users. The auditor is more emotionally removed from the code than its author, and therefore more able to subject this code to criticism and, therefore, can detect more errors.
Increase clarity
If a piece of code is viewed by someone other than its author, it means that there are at least two people in the world who are able to understand it.
Again, the auditor is intellectually more distant from the code and therefore can see the hidden assumptions and controversial decisions laid down by the author.
A clear and understandable code allows beginners to quickly begin to make their own additions to it, which is extremely important for open source projects, since they completely rely on the contribution of voluntary participants.
Reviewer Training
If all of the code being entered passes through a preliminary review, then each such procedure gives the reviewer the opportunity to study that part of the project code that he may not have been able to look into before. Thus, the verification procedure contributes to the dissemination of knowledge about the project code among all its participants.
However, reviewing the code with a “fresh look” of the reviewer does not always have a positive effect: sometimes it can really bring additional clarity, and sometimes it will be too superficial.
Alignment priorities
Since practically every developer has his own set of priorities, then the reviewer's priorities will differ from those of the author. For example, if the author proposes changes that increase the performance of the basic API functions, then it is likely that he will have to prove that this is more important, say, than backward compatibility. Thanks to preliminary reviews, these disputes will arise before the changes fall into the main repository, and not after.
Programming Training
Checking your own code by a programmer is an insight into the essence of your thinking, and prolonged immersion into the thought of another programmer expands your own thinking abilities. Reviewers may suggest techniques that the author has not heard of. They will ask questions that the author did not think about. They will indicate a simpler path that he did not notice.
On the other hand, revisions also encourage the reviewer to explain exactly what he understands by good code.
findings
The code revision process changes depending on which of the listed goals are most important. If revisions are aimed primarily at ensuring the quality of the code, then they should be lengthy and thorough, since reviewers will try to detect errors, performance failures, and the like. If revisions are intended to increase the clarity of the code, they will be shorter, and reviewers will assume that the code is already sufficiently tested and justified.
Solutions
When implementing the code revision procedure, some questions arise that require answers. Below we will look at such questions and see how they were answered in the projects Bazaar, Launchpad and Twisted.
Who should be trusted to review?
Perhaps the most important question that every project should answer is who will perform the code revisions. In most open source projects, reviewers are chosen from among the main developers, although the mechanisms of this choice may vary from project to project.
In the Bazaar project, patches must be approved by two main developers (i.e., those who are explicitly given the right to place the code in the repository). This option ensures that all patches will be viewed by someone with sufficient experience, and also provides the necessary level of communication between developers.
The Launchpad has a dedicated review team in the development team. The group aims to include as many developers as possible, however, to get a full review right, you must go through the training procedure with a mentor. This procedure ensures that, in case of doubt, the new reviewer will be able to contact his mentor, and that the project’s priorities (code clarity and review speed) will be supported by all reviewers.
In Twisted, anyone can do a revision, except for those people who participated in the creation of the patch. This ingenious idea stimulates the group thinking of the author and reviewers. On the other hand, this leads to confusion in the reviews of the reviewers, and the reviews of the code themselves, being a “common cause”, can quickly turn into a “nobody's business”.
How to organize a forum?
In Twisted, revisions are based on bug reports. Bazaar conducts audits on the general mailing list. Launchpad uses a separate mailing list for this.
The Twisted approach is a typical example of the UQDS (the Ultimate Quality Development System, universal quality assurance system). Inspections are performed on the basis of requests, and the web page with the request becomes the focal point of all information on correcting errors or adding a new function. In addition to centralized storage, this method allows to exclude from participation in the discussion of those who are not involved in solving the problem. However, there is a big drawback: reviewers' reviews are rarely read by someone other than the author of the patch, which leads to the application of different coding standards in each particular case. When a really important question arises in a review, it can be very difficult to attract everyone’s attention. Besides, the unsuccessful system of mailings in Trac complicates tracking a discussion course.
In Bazaar, reviews are sent directly to the general mailing list and are taken into account in the Bundle patch management system. Although reviews and patches are sent directly to developers by e-mail, discussions are usually much more open than Twisted. However, a large number of patches make it difficult to read the mailing list, and review discussions can be long and florid.
Launchpad has a separate mailing list for conducting audits, which makes it easy to separate their discussion from general development issues, but still allow outsiders to view this mailing list. In practice, the project receives the advantages and disadvantages of both approaches.
In real time or off-line?
Conducting "asynchronous" revisions is very different from real-time revisions.
When conducting an off-line audit, for example, via e-mail, the reviewer has time to formulate his comments diplomatically, and the author can respond to them in any manner convenient to him.
The “real-time” revisions repeat all the advantages and disadvantages of a regular conversation: misunderstandings are easily detected, and there is no long wait for a response. However, passions can quickly heat up, which makes it difficult to separate facts from emotions. It is also more difficult to determine the duration of such revisions. Having received the letter, you can always see its size in the mail client, and real-time communication can continue indefinitely.
Who will judge the disputants?
It happens that the author and the reviewer have opposite views on the code and can not come to an agreement. For example, the author believes that the following code
[item] = singleton_listis an obvious way to get a single value from a list containing a single item. The reviewer considered this unobvious and suggested the following:
assert len(singleton_list) == 1, \
"List should have only one value: %r" % (singleton_list,)
item = singleton_list[0]Despite mutual arguments, the parties did not come to an agreement. What to do? Who will solve the contradiction?
In Bazaar, the author of the code has the right to say the last word, in Twisted, on the contrary, the last word belongs to the reviewer, while in Launchpad, a specially appointed main reviewer has the decisive voice in such matters.
If the “last word” is provided to the author, then the programming process will go faster due to losses in uniformity and, possibly, quality. If the last word is for the reviewer, then the additions will be implemented more slowly (the reviewer is not so obsessed with the desire to make a patch as its author), but will be thoroughly studied.
What is bad
Ad hominem (go to the individual - approx. Transl.)
There is practically nothing to add: inspect the program , not the programmer . Remarks about his personality only complicate his perception of criticism.
Advice: if you write negative comments, write “patch” or “code” instead of “you”. For example, instead of “You have a glitch in
get_message ”, write “After applying this patch, a glitch appeared in get_message ”.Misty audit results
If all you can say is “There is a
get_message bug in this patch,” then you are a bad reviewer. The purpose of the revision is to improve the code, and not to guess the riddle to the author.The author should be able to unambiguously identify and understand every single thought in your remark, as well as the entire note as a whole. Reviews with fuzzy results turn into endless discussions about code, which are more likely to give pleasure to the reviewer, rather than improving the quality of the code.
Do not confuse personal preferences with objective merits.
This is a problem of observers in any field: film critic, literary editor, scientific reviewer. Not necessarily really good is what I like. And even more so it’s not really bad that I don’t like. One of the aspects of the formation of an outstanding programmer is the modeling of his own preferences in accordance with real, objective advantages.
If you are reviewing a patch that perfectly fixes a problem using the paradigm of imperative programming, then you shouldn’t criticize it just because it doesn’t follow the functional paradigm. So you discredit the idea of revisions, turn it into a game of guessing the preferences of the reviewer, and it should help create a really good code.
The formulation of comments in the form of questions will help the verifier to avoid such an oversight, for example: “Did you think to use the functional style?”, “Why do you not use regular expressions to solve this problem?”, Etc.
See also “A clear answer is a good answer.”
"While you're still here ..."
When reviewing the code, there is always the temptation to ask the author to fix some other problems at the same time. Such an approach can quickly develop into a full-scale glossing operation over a large part of the code, although initially the author and the reviewer had to approve only a small improvement.
The best advice in this situation: to cherish a phased improvement. See "The best is the enemy of the good."
"Freebreeding"
Filibustering is an American political term for delaying the debate on a bill to prevent its adoption. Sometimes this is done in free software projects to sabotage the adoption of a particular patch.
"Filibuster" may also arise unintentionally. The person conducting the revision does not accept the patch due to the absence of block tests, the author asks how to write the block tests for this code, and the reviewer does not bother to answer. But in addition to these troubles, the author also has his own affairs that prevent him from continuing to send new patches.
See “Foggy review summary” and “Quick response - good response.”
What is good
A clear answer is a good answer.
Specify specific lines of code that can be improved. Describe what's wrong with them. Suggest a way to do better. Help the author to define the limits of improvement.
A quick answer is a good answer.
Once the author has submitted his patches for revision, he can no longer work on them. He must wait for the auditor’s response before continuing. Write your comments to the review as soon as possible so that the author’s attention does not have time to dissipate.
Best the enemy of the good
You can not make a perfect patch that would solve all your problems at once. Do not pursue the ideal, work on a phased improvement.
Be thankful
Someone tried to improve your product. They have spent their strength and imagination to help you, and now they have submitted their work to the public: thank them so much.
In the Divmod project, the principle of including at least one praise in every review of the code is practiced. You can always find what to praise the author for, even if only because he undertook to work on this part of the code.
Ask questions
The reviewer should ask questions. You can not be mistaken by asking questions. In the worst case, the author will give a trivial answer. At best, both the author and the reviewer will discover something new.
Conclusion
Code revisions give a person a unique opportunity to grow as a programmer, while improving the product he creates. Each project can choose from many options for conducting audits and the opportunities they provide. Thoroughly thinking about technologies and processes of interaction of people in conducting surveys will allow open source projects to avoid the difficulties that frighten beginners and tire long-standing participants, and focus on developing the best software.
What to read on the topic
- Guido Van Rossum on Mondrian , Google's own code review tool
- The Ultimate Quality Development System
- The bazaar hacking guide
- Beyond copy-editing: the editor-writer relationship
PS Moved to the "Revision Code" blog.
Source: https://habr.com/ru/post/64086/
All Articles