Somethings about code review you need to know
The technology and practice of software engineering is divided into two parts, one part is consistent with the book knowledge, about half, this part can be learned basically in the university, self-study as long as the method is appropriate, hard work can also be the way; but the second part comes from In actual teams, experience, content is usually not available from books, and it is difficult to say right or wrong, different people and different experiences have created different understandings. The code review is a typical part of the second part, which can be discussed in a big way.
Code review is a way to show personality and personality
I personally object to a rather common view that “a well-functioning project, different people should write the same code.” I understand the original intention of this point of view. In a team with good normative constraints, the code written by different people should be consistent. But in fact, the team that can really do this, I have never seen it before. The so-called consistent code, carefully tasted, found that different engineers wrote it is inconsistent. Therefore, the word “consistent” must be a general description to a certain extent, and it is not the more consistent. In fact, the creation of the code is unique. There is no doubt that we should comply with the statute and should conform to the habit, but once we try to overuse them to constrain the code, it is not only difficult to implement, but also prone to boring, inefficient and contradictory atmosphere.
Go back to the code review. The code review itself, as well as back and forth communication based on the review comments, is more difficult and less consistent than the code itself. I have seen many styles of code review. Some people like to pick up small problems. Some people like to talk about their opinions. Some people like to give practical examples to prove their opinions… No matter which style, I don’t think there is any big problem. But, as far as I am concerned, I can talk about my own code review style:
I will focus on three issues, requirements and business issues, code structure issues, and code style formatting issues.
The first two may have “serious” problems that hinder my ship code (saying “ship” means that the approved code has the conditions for pushing to the mainline branch). I can’t remember how many times the code author argued because of such a problem. Controversy is an art. Sometimes it is not always possible to reach an agreement. Some people are more likely to be persuaded, while others are more assertive. I don’t want to say which one is better, but it’s true that this is the fact that the style of the code review shows up – it’s all about things, but when someone is afraid or doesn’t like offending people, they will appear push over; others don’t care so much, believe in themselves. The idea is more appropriate and will appear defensive. I may belong to the latter. It seems that there are instances of conflicts with code reviews at various stages of my career, but in some cases I also choose disagree and commit (disagree but the opinions of the executive team). I believe that some teams will like my backbone, and some teams will hate my style. The following content is also mostly based on the expression of its own style.
Resolutely express their opinions, but euphemistically express
In this regard, I am also working hard to improve. There are many points that can be mentioned and a lot of skills, but to be honest, conflicts are inevitable. In almost all the teams I have experienced, there are sometimes good old people, but basically all the good guys lack the insistence on principles. Communication is the art of the door, and it is also reflected in the code review.
- The most important one is only for code, not for people. This article is very simple, knowing the importance of not being right, but be very careful not to violate it.
- For most of the code style formatting issues, I will mark this question as a picky or nit question (“a picky question”, which is what I learned when I was at Amazon). The advantage of this is that I clearly told the other party that although I raised this question, it is not a big deal. If you insist on not changing, I am not going to convince you. Or, I have a different opinion on this issue, but I am not convinced that my proposal is better.
- Use a modal particle that may, perhaps, possibly, and seems to be indeterminate (including the use of a subjunctive mood). The advantage of this is to ease the tone of your own opinions. For example: “Refactoring this place, it is better to remove this loop.”
- Interrogative expression of doubt. For example, seeing that the other party used a parameter of 3, but you feel wrong, but not very sure, you can say: “I have a question, why should I use 3 instead of other values here?” The other party may react to this The selection of the value is not appropriate enough.
- Put examples, links to discussions, and other supporting materials to prove your point of view, but don’t express your opinions directly and let the other party confirm this point. For example: “The following discussion is about another way of implementing this logic. I wonder if you think it will be a bit more concise?”
- First, then deny. I think a lot of people have been using it all the time. Let’s first say some consent and positive words, then use the revolution to say the opposite. This will compare the pros and cons in the speech, which means that this is A conclusion drawn by trade-off.
Some very common questions that can be mentioned in the code review
There are actually a lot of such problems, most of which have nothing to do with the technology of implementation, but it is easy to accidentally skip it. They are problems most of the time, and of course they are not.
- For example, one of the things I hate the most, a class or module with too broad responsibilities or unclear responsibilities. I have seen this class countless times: Helper, or Utils (note that they have no model or module prefix). Considering the size of the project, in most cases, such a class can easily become a balloon that is blowing more and more, and it seems that everything can be left in the air. It is a terrible design that violates the principle of single responsibility.
- For example, in thread usage, container usage, connection management, etc., there is a lack of cap-and-control design, which in some cases leads to excessive expansion of resource usage. The queue design of producers and consumers, once the consumer hangs up or can’t keep up, the queue is more and more, there is no rejection mechanism.
- For example, lack of subcontracting, layering, all things are stacked together, dozens, even dozens of class files are listed under the same folder.
- For example, the lack of design, flow account structure, noodle-type code, or a simple process of several procedural methods, the cost is not too small, the implementation rate of natural modification is low, so the people who ask the problem are quite For a headache.
Avoid mentioning too many questions in one review
In a few cases, getting a copy of the code that is really problematic is a time when you need to control your own impulses. First grasp the trunk of the problem, don’t pick up the details of the details, because it is very likely that the code will be changed beyond recognition or rewrite. Writing a lot of reviews is easy to overwhelm the most important issues.
It’s easy to talk about, but this degree is sometimes not well mastered. My habit is that after clearing the problem to be solved by the code, I should quickly go through the code and judge whether I should roughly grasp the main contradiction, or should I rigorously pick it up. I have seen some other ways.
Different review requirements
I always feel that the requirements for code review should not be exactly the same. Don’t overly pursue fairness. For new project code, as well as code written by new employees, it should be more appropriate.
The code for the new project is a template that forms the style and quality of the follow-up. We may all have heard of the “breaking window principle”. In a clean code field, the newly cultivated code is easy to keep consistent, and it can avoid some “to be consistent with the existing code style” and lead to quality compromise. The situation arises.
The high quality requirements of new employee codes are more about being responsible for their good habits. The formation of good professional habits of software engineers, the code can be said to be the most important part, and the impact of the first three months is very important.
It’s better not to judge
In some cases, code review is a very time consuming and laborious task. Especially unfamiliar with the business background, unfamiliar with the implementation technology, or simply a lot of changes submitted by the other party, reading is very laborious. I don’t know which one is the hardest, but if it is difficult to do a good review for these reasons, I will explain it in the review, or give up some review work to ensure the quality of the review.
Code review is a good way to build influence in your team. On the one hand, the business logic, on the other hand, the code structure. I object to a bunch of minor issues in style when it is difficult to give sufficient clear review opinions in both aspects. Otherwise it is easy to achieve a negative impact.
Let me talk about it here, I think there are still a lot of things that can be involved, and they are hard to learn in class and books. Controversy is interesting, and it’s true that practice is true. Do you have a consistent view, or rebut, welcome to discuss with me.