The art of review – talk about the code review in reality

I have written a little article about code review, such as this and this, now I feel that there is a lot of updates about it. The technical and practice of software engineering is divided into two parts. Part is consistent with books, about half, this part can basically learn in college, self-study as long as the method is proper, but the second part is from In actual team, experience, content is usually unable to get from books, and it is difficult to say that different people and different experience have made different understandings. The code review is the second part is quite slot, which can be a typical example.

Code review is a way to show personalities and character

I personally oppose a quite common point of view, “a good operational project, different people, the same code should be written.” I understand the original intention of this point of view, a team of different people, and the code written out should be consistent. But in fact, I haven’t seen it at all teams that can do this. The so-called consistent code, carefully taste, discovers that different engineers are written and are inconsistent. Therefore, the word “unanimous” must be a general description of a certain extent, and it is not a good agreement. In fact, the creativity of the code is personal. There is no doubt that we should comply with the statute should comply with habits, but once you try to use them to go to constraint, it is not only difficult to implement, but it is easy to generate non-fun, inefficient and contradictory atmosphere.

Go back to the code review. The code review itself, as well as the return communication based on the review, and the code itself is more difficult, and it should not be consistent. I have seen many code-reviewed style. Some people like to pick small problems. Some people like to express their views. Some people like to prove their views to the actual example … No matter which style, I don’t think there is any big problem. However, I personally say that I can talk about my own code review style:

I will pay attention to three problems, demand and business issues, code structure issues, code style format.

The first two may have a “serious” problem that hinders me SHIP code (say “Ship” means that the recognizable code has the conditions for PUSH to the main line branch). I can’t remember how many times and code authors argued because of such problems. The argument is an art, sometimes it is not necessarily possible, and some people are more likely to be convinced, and some people have more insisted. I don’t want to say which better, but it is true that this is the fact that the style of the code is – all the matter, but some people are afraid or don’t like the sinner. If someone doesn’t care so much, I don’t care so much, I firmly believe in myself The idea is more appropriate, and it will look defensive. I may belong to the latter, and it seems that there will be an example of a code review conflict with me in the career, but in some cases I will also choose Disagree and Commit (disagree but the opinion of the team reached). I believe that some teams will like my backbone, and there will be a team to hate my style. The following content is mostly based on its own style.

Strengthen your own opinion, but euphemistically expressed

I am also trying to improve this. There are many points that can be mentioned, there are many techniques, but honestly say that conflicts are still inevitable. In almost all teams I have experienced, sometimes there will be old good people, but all old good people are lacking for the principle. Communication is a door art, and it is also the same in the code review.

The most important one, only for the code, not for people. This is very simple, knowing that the importance of people is not right, but it is very careful not to violate.

For the problems in most code style formats, I will label this problem is a Picky or NIT problem (“picky problem”, this is what I learned when I am in Amazon). This kind of benefit is to clearly tell the other party, although I put forward this problem, but it’s not big, if you insist, I don’t want to convince you. Or, I have different opinions on this issue, but I don’t believe that my proposal is better.

Perhaps, perhaps, it is possible to indicate that uncertain tone words (including using virtual tone gas). Such benefits are to ease the tone of your own opinions. For example: “This place refactors, remove this loop, maybe better.”

Indirect expression doubts. For example, seeing the other party with a parameter 3, but you don’t think, but it is not very sure, you can say this: “I have a question, why do you want to use 3 instead of other values?” The other party may react this. The value is not appropriate enough.

Put an example, the links discussed, and other auxiliary materials prove their own point of view, but do not directly express the views, let the other party to confirm this view. For example: “The following discussion is another implementation of this logic. I don’t know if you think it will be slightly simple?”

Affirmation first, no longer negate. I think a lot of people have been using it, and the truth is sincerely say some consent and positive words, then use hoof to turn, say the opposite situation, this is also comparing PROS and CONS in the speech, which means this is The conclusions obtained after trade-off. ……

Some common problems can be mentioned in the code review

Such a problem is actually a lot, most and the technology of the technology, but it is easy to slightly. Most of them are problems, of course, sometimes it is not.

For example, one of my most hate, responsibilities are too broad or unclear class or module. I have seen such a class in many 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 is easy to become a more blown balloon, and it seems that something can be put, it is a poor design of a single responsibility principle.

For example, in thread usage, container use, connection management …, lack of upper-limit control design, in some cases, to excessively expand resource use. Producers and consumers’ queue design, once consumers hang or can’t keep up, the more the queues, the more, no mechanisms.

For example, lack of subcontracting, hierarchical, all things are stacked together, more than dozens of class files below the same folder.

Rethiore, the code lacks the design, streaming structure, noodle code, or simple laying of several processes. This modification is still not small, and the implementation rate of natural revision is low, and the people who make problems are also quite For a headache.


Avoid excessive problems mentioned in an assessment

In a small number of cases, I have a problem that I have more problematic, and I need to be able to swear the impulse. First capture the backbone of the problem, don’t pick the problems of those fine branches, because it is very likely that these code will be changed unrecognizable, or rewritten. Write a lot of review opinions, but it is easy to overunday the most important issue.

Talking about it, but this degree is sometimes not well mastered. My habit is that after the issue to be solved by the code, I will first read the code quickly. Judging that I should roughly arrest the main contradictions, or should be strictly picking up. I have also seen some other ways.

Different review requirements

I always think that the requirements for code review should not be entirely consistent. Don’t overactize fairness. For new project code, as well as the code written by new employees, it should be appropriate.

The code for the new project is a template that forms subsequent styles and quality. We may have heard that “the principle of breaking windows”, in a dry clean code field, new cultivated code is easy to maintain, can also avoid some “in order to maintain consistent with existing code style” and lead to quality compromise The situation appears.

The high quality requirements for new employee codes are more responsible for their good habits. Software engineers have a good career habit, and code can be said to be the most important ring, and the impact of the first three months is lifted.

Not as good as the review

In some cases, the code review is a very laborious work. Especially for business background, it is unfamiliar with the implementation of technology, or simply is a lot of modifications to the other party. I don’t know which one is the most difficult, but if it is difficult for these reasons, I will explain it in the review or give up some review work to ensure the quality of the review.

The code review is a good way to build in the team. On the one hand, business logic is a code structure. I oppose it in two aspects, it is difficult to give sufficient clear review of reviews. Otherwise it is easy to reach a negative impact.

Let me talk about it first, I think there are many things that can be involved, and they are difficult to learn in class and books. The debate is interesting, practicing realism is trying. What is the consistent view, or refute, welcome to discuss me.