Some experience sharing about Code Review within the team seems to be a bit empty, but it is also carefully prepared. Each company may be different from each platform, but the overall logic can be used for reference, may be able to gain inspiration. The following is mainly the manuscript, if there are some strange places to understand a little. There are many pictures and texts, and the reading time is about 10 minutes.

A simple self-introduction: Eel, front-end development, has been in Baidu since graduation, now based in Shenzhen, if you want to know more, you can pay attention to or read the previous article. There is a disclaimer, this share content is only based on the current experience of personal feelings, if there is any idea of upgrading, later. If you have any questions, you can also communicate at any time.

This sharing will include three parts: what is CR, the attempts we have had, the difficulties we have encountered & solutions, etc. (There are also practical tips related to the platform for employees in the factory, which are not listed here).

PART 1 · What is CR

The purpose of Code Review is to find and correct errors that are not found in the early stage of software development, and to improve the quality of software and the skills of developers. Code reviews often take different forms, such as pair programming, informal reviews of entire code, or formal software reviews.

So down, is there a kind of, look very powerful but really want to say nothing like? Git commit => git merge

This process, of course, requires us to do more than just submit it. In general, we require the person who writes the code to consciously write code that is easy to read. For example, sometimes when we test, the variable is often defined as ABC, but in the formal review, we must choose some easy to read names.

For reviewers, they want a place to look at the code they’ve submitted, comment on it, ask questions, make suggestions, and like it, and the process may take several rounds. After a series of processes, it is expected that there will be a better platform to do some precipitation and sharing of valuable discussions

So, what we’re going to talk about here in CR is not just a process of code being pulled up and then merged by a button action.

Review may be extended to the whole process of r&d, such as the correctness of requirements, the design of codes, the real coding style, and other relevant contexts.

It sounds a little bit abstract, but let’s look at a couple of examples of what happens.

The first is a deceptively simple API change.

Some time back in CR, I saw a weird new API called forceUpdate, and I wrote a very serious comment about forcing updates. But as a curious reviewer, a point that needs to be confirmed, why?

So there was a series of conversations: Why was this added? Is there a performance problem with this complexity? Have you investigated alternative implementations? If there’s a question? How big will the impact be? Is there any possibility of negotiation on demand?

After a series of intermittent interlocutions, I found that it was actually an ellipsis requirement, and the logic was very complicated. Then there were some small bugs on the page, and a mandatory update could solve the bug. In fact, there is a certain amount of room for optimization of the entire requirement, so when cr, you can ask why according to the code diff, and then you will find:

  • There may be room for discussion when a small detail causes a large implementation cost
  • Maybe the design is not unreasonable, in fact, there is a more mature plan, but he is very hard to achieve all kinds of
  • The code may not change much, in fact, the interface design is not reasonable a certain layer of lazy, and finally all the burden is borne in this layer

At such times, as a reviewer, you can have the courage to work with your submitter to push requirements back and find a better balance, rather than going crazy for compatibility. This is an example of looking at the rationality of requirements through process extension.

In another example, there is a repeat string method that then adds compatible logic to determine that if it is not a string, it will throw an exception that must be a string. It seems that there is nothing wrong with it, and even want to praise this classmate is good, know how to deal with abnormalities.

But does this non-transparent adjustment have any effect on the caller? For example, he used to return string, now he returns string or exception. If the caller does not handle the exception properly, the whole page may crash, and then there may be an online bug: occasionally a blank screen appears, but it seems that nothing has been changed. If there is no exception log, it may take some time to find the problem.

The lesson of this story is that when it comes to code, especially method changes, it’s sometimes necessary to look ahead. And use good tools, like I can look at references in the editor and so on. This is actually an example of extending context through code diff.

These are two very simple examples, but as you can see, often a random code submission is more than a few lines of diff. The change you see may be a few tiny tweaks, but it may actually be working frantically under a huge, complex tree.

If we only look at Review one-sided, only look at the DIff produced in the CR process. So, our CR can’t go very far, and it’s very possible that one day, the code won’t be maintained, and one day, it will collapse.

PART 2 · Previous attempts

After the explanation of the basic concepts, let’s take a look at some of our attempts and efforts in the direction of FE.

I joined Baidu in 2017. At that time, I should have just migrated from Gitlab to the internal platform, which was basically unsupervised and I didn’t realize CR was a process. Most of the time, it was one person and one project, and the platform did not have strict means to require someone to help you cr to join, so it was easy to indulge in their own world and fly. It feels like CR is just a decoration.

In the next 18 years, when there was no topic, the weekly meeting would do CR, and the host would take turns to host, pick the code warehouse review and do some sharing. It is common to pull branches separately and then write comments in the form of comments. The advantage is that someone will finally look at someone else’s code, but there are some problems:

  1. There are only a few code libraries, and there will be a feeling that once you’ve seen it, you won’t be able to see it for more than a few rounds
  2. Easy to formalize, and often get caught up in details
  3. [bug Mc-10387] – the code is merged before CR, indicating that problems can occur for various known reasons and do not change

Then, in nineteen nineteen, I tried to make a basic CR specification, and then I created a CR group. After submitting, I threw links in the group with the help of at people. Advantages: The atmosphere of CR is basically formed, and we can discuss the problems together, but most of the problems are still in the details stage. Existing problems:

  1. Too much group information, too few people will take the initiative to seriously CR, the project is tight, often brainless join in
  2. New students are generally embarrassed, or easy to directly send their familiar people, and there is no effective means to prevent
  3. Similar problems, lack of precipitation

By the time of 20 years, the whole department began to follow this idea and set up a CR group for each direction. The leaders of each direction took the lead in CR every day, and the core members of different teams were basically present in the group of each direction. Therefore, from time to time, the code you wrote would be submitted to CR by a “big man”. However, in this way, the news is too explosive, and it will be found that different groups have similar problems, so it is a waste of time to discuss and modify it again and again.

Therefore, in the second half of last year, all CR with comments were required to do a certain summary, and then similar topics were deposited for similar problems, such as if and else nesting too much should be done. In the weekly meeting, a weekly quick report was added to explain some interesting CR by the host. In addition, some teams developed various efficient robots to facilitate CR.

After this set, last year should be the front direction, at least each person in charge is aware of the importance of CR, but also brought some problems. There are too many people, and they’re all in different projects, so if you have two people working on a project, even if there’s a group, they’ll happily +2 each other and not pay much attention to details, they’ll be very people related. Therefore, a team is trying a form of group CR, in which all codes are directly +2, and then reset at the end of each week to the nodes without review, and a NEW CR is proposed, which is discussed and reviewed by everyone (similar to the PR of Github). This will not consume too much time and energy, but the group needs an experienced student to take the lead, and to push forward with strong operation.

(Background: CR in the factory is different from PR in Github, which is mainly reviewed based on COMMIT. That is, after each code is submitted, a review link will be generated separately. When everyone thinks that the review is ok, there will be a scoring link, that is, +2 will be allowed to be added.)

The above is a situation at present, the screenshots are some of the things we do, and the precipitation of the document. For example, CR robot just said, some of the knowledge of precipitation, as well as a summary of CR problems like comments and so on (desensitization code is thicker)

All the way, I feel that the current situation is not completely successful, but I can see that everyone is making continuous efforts and making progress slowly.

Here is mainly to express a point of view, some things, the truth we all understand. But, for a variety of reasons, it’s not that easy to do. If you are trying to push something that looks a little different, you may all feel it. Here is a quote from Romain Rolland:

PART 3 · Problems & solutions

Next, I will share some of the difficulties I encountered in this process and some of the solutions I think are reasonable.

3.1 Service Pressure

First, the biggest problem, business pressure. Imagine getting up every day, turning on the computer at work, saying it was a wonderful day and getting ready to help with cr. And then you look at all the demand emails for the product, and the PMO’s in the group and you ask about the schedule, when is it going to be available, and we’re like lambs waiting to be slaughtered.

At this point, there is usually a quiet reduction in CR time, which leads to the following real conversation: come and go, deploy QA and wait for validation; Time is so tight, don’t break off the naming of these standard problems, slowly after the whole; I will change it later. I will pass it first. I submit too many things for fear of conflict. And so on. At this time, as a serious CR people, I am also very helpless ah.

This usually has several problems and solutions. First of all, for the scenarios that are pushed by the project schedule, think about it. At present, most of our schedule is development + visual interaction restoration + testing time, and sometimes we leave some buffer. But CR is actually a process of coding, as a process, he is bound to consume time, so we can not as a standard process, counted in the development time?

The actual scenario that happened some time ago: the project time was relatively tight. Although problems were raised, the CR was directly and quickly passed without further investigation. It took two or three days to solve the problem, and the whole module was almost rewritten.

Generally speaking: development + CR discussion time < no CR, modify after encountering problems, design unreasonable maintenance costs. So one of the points here, the prophase accepts his presence and counts it in the schedule.

The second problem is also easy to arise. In the case of tight business time, we see some unreasonable things that do not affect major functions in fact, such as code specifications, and naming. If the developer works hard and works late to write the code, he or she is tested with a knife and asks you to deploy one. And then, while you’re leisurely eating melon seeds, you tell him that the name here should be more semantic, because if you think about it, it’s likely that everyone would want to chop you.

In fact, this situation is a team running-in problem. There is no clear concept of importance classification, and the priorities are not accurate, so it may spend too much energy on details. The suggestion is to follow the principle that small details and differences in CR do not block business, and to include them for discussion, unless there is a big design problem. The life cycle of CR does not end with +2 integration.

And the third point, you can look at a cartoon on the right. You are writing code seriously, and then xiaoming beside happily calls you to help me look at a CR. As a serious and responsible you, of course, is full of promise, ready to click in to have a look, after a second to open the web page, found 326 file changes, at this time ask your heart shadow area.

This scenario, it is a one-off, according to the code too much, many students feel that I should put the development of a fully functional, lift up again, you don’t know what I am writing, but in fact, coding is a gradual process, suggest students daily development control size, commit as much as possible to ensure to submit every day, As well as writing a commit message as well as associating the commit with the card, it will also help us understand your intention better.

3.2 Mental awareness

You can look directly at the picture, some common mental activity: you called my code back? Are you out of love with me? / MY hard work was beaten -1. Does the boss think I am a chicken? / You said don’t write this, then how to write you tell me, repeatedly knocked back too uncomfortable…

These problems good solution is very good to solve, say difficult to solve after all, this is a psychological problem, sometimes not so easy. The main causes may be:

  • To the judge, he might relate the CR in technology to the innate human fear of rejection
  • For the judges: on the other hand, he may be considerate of others’ feelings, dare not give negative marks, and only +1 with some comments if there are questions

In essence, as long as you understand the technical “beat back” and “do not recognize” are two different things, it is easy to handle, in addition, it is also suggested to standardize the standard of different scores, we cognitive unity. For example, our team will have a reference score (corresponding to the company platform).

Here, we set up some cr-related etiquette for both students who do CR and those who submit code. For reviewers:

  • I hope he is serious and responsible for every line of code. In addition to code specifications, he needs to see the logic of the business itself
  • I hope you can make more comments. The comments are not only about what you think is not good, but also about what you have done well
  • Go ahead if time is tight, but be sure to review it
  • Precipitate common questions and share summaries regularly

There are also several expectations for code submitters: one topic at a time as much as possible, small grained submission, and looking first to avoid conflicts. Nice, will fix it/feature& explain /not a bug explain/thanks… Again, often referred to as document precipitation.

In addition, there is another point, the perceptive comment. We once had a heated discussion on this side. Let me briefly make a conclusion. First of all, I suggest to minimize perceptive comments, such as: feel bad here can be optimized, do not nest so many layers, can simplify, here is too messy change it…

Instead, it should be: this doesn’t work, change it, this doesn’t work, how to change it, this doesn’t work, how to change it, why.

In fact, this process is not a low requirement for reviewers, but also a process of mutual learning. In reality, due to people’s limited energy, it may take time to write every item in detail, and it may be easy for the code submission person to lose the ability to think about the problem. There are certain conventions in the team. For different types of problems, THE CR format can be different. It can be analyzed on a case-by-case basis.

3.3 Unfamiliar with the project

Third, the project is unfamiliar, especially in the case of one person following a project. I do, but there are so many things, and it feels like a magic attack when faced with all kinds of business logic. I think it’s weird, but I can’t tell what’s wrong with it, so I can only be a human code checker.

Here’s a quick story, for those of you who like to play games, GTA5 online is incredibly slow to load, usually taking more than 10 minutes. Then some hacker decompiles the file and finds that the open machine reads a 10-megabyte, 60,000-line, character-by-character JSON file and executes the if command 1.9 billion times, and it still does

Think about it, if you have to do this kind of code review, you will be confused, even though it works.

In fact, most of the time, we feel that the very complex things, it is likely to be some code design problems, such as logic is not clear enough abstract degree, confusion of naming, too much duplicate code, code files too large and so on. The most intuitive way is not to mention that I have to guess what you are doing carefully. It will save a lot of time to directly bring the student in the team to explain offline and then make comments (teleconference is fine if it is remote).

Here to borrow a word from a big man in our factory: if I can’t understand it within five minutes, there must be something wrong with this code.

Therefore, in the CR process, reviewers can be bold and don’t think about whether I can’t understand the code or not. If they can’t understand the code, they can ask directly and boldly. Don’t be confused by all kinds of strange logic on the surface. In addition, when we usually write code, also try to think about, you write code to be seen by people, not just for the machine to see.

A more effective way to do this is when you want to let yourself go and have a fling. Think of that pathetic coworker who’s ready to take over your code, like hiring a new classmate to take along on a project. Or, six months later, the product will tell you to add something to this module and feel sorry for yourself.

SOMETIMES, It’s “not the code is RIGHT” that matters. INSTEAD, It’s “the code is written RIGHT” is matters.

Sometimes, it’s not enough that the code is correct, it works, it’s more that it’s written correctly and that it’s written in a way that makes it readable and maintainable.

3.4 Repeat low-quality CR

For example, we often say that a function should not take too many arguments, and if it does, try to write it as an object. But for new students, or students who are not familiar with the business, he may really just add something, and then add it and suddenly it becomes more. As a judge, at the beginning, you might have reminded him seriously and found some supporting documents corresponding to the theory. Then when you met the second student, you pointed out the problem and suggested how to write the plan, and thought you did a good job.

But as you get more and more CRS, you might find that there’s a third, a fourth, a fifth problem. You also start to lose patience slowly, maybe CR will become a one-sentence comment, here the parameters are concise, the new student will be confused. So you try to explain again, and then sigh, feeling like you’re doing this every day, your life burning for nothing, wanting to have brainwave transmission.

So the solution? It’s not that hard. First of all, one of life’s golden rules: the holes we step in, we step in again. I don’t know who said that.

So how do you minimize the number of potholes? One way is to let everyone step on it to make a better impression, but as people move around, the number of people in the team increases. Another is the need to actively do a good document precipitation, summary, sharing and so on.

For example, after two years, we may include CR that we think is valuable, and there are corresponding items on the list that students in charge will review every week. If problems encountered in a project are similar to those in another project, relevant students can be brought together to have a discussion. Or there could be some sort of reward and punishment but it’s just an idea and it’s not yet tested.

3.5 Differences of Opinion

Most of the time, there is no absolute right or wrong way to write code, but a certain tradeoff in different scenarios. But often, because people have different perspectives and different habits, it comes up I think what I’m doing is good, you think what you’re doing is good. Radish green vegetables, not careful dispute. The indented jihad of history, and whether PHP is the best language and all sorts of stuff that went on for a decade or two.

First of all, it’s a good thing to have this problem. At least that we look at the problem, have their own thinking inside. So what do you do when it comes to this problem? As long as team members can embrace that things can’t be black and white, they can have it both ways.

The usual recommended approach to non-blocking business principles continues to be discussed. For example, in your spare time, you can launch a debate contest, make some voting decisions within the team or find a technical master to help you decide. Of course, you need documentation at the end, or someone else will have another discussion and you’ll have to start all over again, back to repeating the question.

Above, and much more, in a nutshell

If Code is a work of art, then CR is the creation process in Review. Here is a vivid example

This is a Picasso painting, called the bull, the specific market price has not been carefully investigated, but according to his average painting, at least tens of millions of dollars is indispensable. A lot of people, when they look at it, they think, this is a stick figure for children, and I can do it, too.

But in fact, when you look at the process diagram, it’s actually a step-by-step abstraction, similar to how we write code. Sometimes, you really can’t just look at the result without looking at the process. Of course, you can’t focus too much on the process without looking at the result. You need to find a balance.

In the final analysis, to do a good CR, we need to spend some effort on consciousness, mentality, norms, experience and precipitation, rather than just stay on the +1 +2 button. It is a process of mutual cooperation and growth of a team, such as the recognition degree of CR, whether there are good specifications to support you to do CR, whether everyone is willing to establish specifications together, where to deposit documents, when to share, whether there are useful tools and so on.

Therefore, if I want to make a good CR, IT does not mean that I feel inspired by this article and decide to do it well. All of these need to be explored together. Of course, it may save some detours.

PART 4 · THE END

There should be some tips here, but I’ll leave them out because they’re platform specific.

In addition to the above experience, there is another important point. To do a good job in CR, a Leader needs to take the lead and take the initiative to participate. Maybe one day, maybe the team will slowly start to get better, will start to grow. After all, you have to have a dream. What if?

Appendix, recommend some small tools used in this sharing, if necessary, you can baidu search:

  • The code highlights the Carbon
  • Hand-painted style flow chart tool excalidraw
  • Dribbble, an awesome illustration site
  • Personal hand-drawing tool iPad + Apple Pencil

And finally, thank you for slipping on the bottom, and there may or may not be some videos to share. The whole is a little scattered, or the expected technical dry goods may not be said. We will continue to refuel later. You have all kinds of topics you want to know, you can also leave a message to praise and encourage, and you can write series slowly when you are free