This post first appeared on my personal blog.

My previous blog post also wrote two articles on Code Review from different starting points:

  • Does QA testing look at the code — the need for QA to read the production code

  • Thinking about Code Review — The need for Code Review

  • Code Reading Tips – Some methodology for reading code

It seems that topics related to Code Review are particularly easy to find in the article 😅. In fact, there are many introductions of Code Review process on the Internet, including both Chinese and English materials, but there is no more systematic article to describe a complete and feasible Code Review scheme, specific to each link of the process.

Compared with the mediocre “How to Do Code Review”, it seems more interesting and meaningful to sort out a more systematic implementation plan for Code Review and provide reference for students who need it. With this idea in mind, I tried to sort out some experiences and ideas from the perspective of R&D efficiency from the perspective of QA. Many of the inspirations come from the Code Review plan being implemented by the R&D team, which is referred to as CR.

Code Review background

It is well known that the earlier bugs are found, the lower the cost of fixing them, and automated checks are designed to move tests as far left as possible, finding bugs early in the development process. According to this idea, from the perspective of the r&d process, RD self-test is the first, then the unit test before and after RD code submission, and then the automatic test triggered by code combination with the main trunk branch and CR. Let alone the self-test and single test of RD, these two points are often difficult to do well and need a strong driving force. So from this point of view, the point at which the code joins the main trunk branch is basically the earliest stage of outside involvement in testing, and CR is an early Bug detection tool that can be incorporated naturally.

On the other hand, the landing of CR reflects the engineering atmosphere of a RESEARCH and development team. It is easy for newcomers to obtain technical improvement through CR. At the same time, CR improves everyone’s familiarity with the same module, avoids a single module being understood by only one student, and prevents personnel flow from leading to a vacuum period in code development and maintenance. For example, a distributed system does not want a single point, and the master node should be replaced at any time, otherwise the probability of system failure will be higher.

Furthermore, there are frequent online accidents, and after analyzing a series of online accidents, our QA and RD team found that many of the bugs that caused the accidents were likely to be found during the CR phase (not 100% of course).

To sum up, the necessity of Code Review has been elaborated.

How does QA do Code Review

Why is IT difficult for QA to participate in CR

A lot of QA may feel that CR is RD’s job and has nothing to do with QA, which is one-sided. CR is rD-driven, but it’s a good place for QA to implement the principle of moving to the left of testing, as well as look at the production code and familiarize yourself with the details of what you’re testing.

In my opinion, one of the most critical points of QA’s difficulty in presenting a sense of participation in CR is that many QA’s lack of code experience or white-box testing thinking leads to low participation in CR, in other words, lack of experience and methodology in doing CR, which can be accumulated through practice. The older QA staff will not value CR, and the new QA students will probably not value CR either. On the other hand, it is true that the daily requirements test is quite heavy, especially for the students on the client side (mobile and Web side). It is an objective phenomenon that the business pressure is heavy, which will not be discussed here. In my opinion, a good QA should not be too far removed from the RD, except as a gatekeeper for process specifications.

The QA team I work for has been having problems with CR as follows:

  • Most of the business requirements (generally referring to the functions with normal interaction factors) are initiated after the requirements are tested. The time node of CR is too late, which leads to the failure of testing after CR finds problems and changes the code. The correct practice is to complete CR before or during testing, or retest CR after modification

  • Technical requirements (generally referring to functions poorly reflected by code optimization or UI), QA did not participate in the early stage of requirements, and was pulled in by RD without knowing the context to carry out CR on unfamiliar functional modules, which had no effect

  • In view of the above two points, IT is difficult for QA to play the role of real quality assurance of code bayon, and it is also impossible to establish RD’s technical recognition of QA, so QA’s CR becomes a mere formality (even RD’s CR).

CR goals from QA perspective

The ultimate goal is quality assurance. CR is a means of testing left shift. It is intended to find bugs in the early stage of testing and reduce the cost of Bug repair. The QA perspective CR is actually a complement to the RD perspective CR, there will be a difference in focus. The primary goal of QA is to find quality defects in the code, that is, to find bugs, not whether the code is elegant or better implemented.

QA CR criteria

To understand the role that QA plays in CR, CR can focus on many aspects, such as code design, maintainability, scalability, security, performance, etc. However, QA is not required to cover all aspects. As a role of quality control, QA should focus more on function implementation and code robustness.

1. No coding style

  • Ideally, coding style control should be left to specialized tools (various Lints) or the RD itself, and QA should focus on code logic rather than specification details such as naming style, function length, function return value, etc.

2. Clear Change List

  • The scope of change is the core information for QA to judge CR strategy. QA is certainly less familiar with the code itself and module design than RD, so it needs to obtain more auxiliary information to assist CR in CR.
  • Knowing the scope of the change allows you to identify the code risk first in terms of business knowledge, and then go through the code level one by one according to the risk points — which can be interpreted as QA designing according to the change firstCR cases, and then take the specific codeCR casesFor example, for a piece of loop logic, QA knows the end conditions of the loop based on business knowledge, and then looks for potential conditional scenarios that lead to an infinite loop in combination with the implementation.
  • Double Check comes with documentation in case you understand errors or miss information.

3. Where are the bugs

  • Repeat after me: under what circumstances will this code Bug. The key to QA CR is to switch your mind — try to deliberately traverse and overwrite scenarios where bugs can occur. Instead of following the code logic to see if there is any doubt, it may be assimilated by RD’s thinking. For example, if you see an if judgment, you need to think about the boundary value of the judgment variable, the trigger scenario, and so on.
  • It is necessary to understand the implementation idea of RD first. Maybe we should follow the idea of RD to go through it first, clarify the function details and call relationship, and then change our thinking to look again.

4. Ask RD to answer

  • Especially for technical requirements, QA’s early participation is not guaranteed in most cases, QA is not clear about the background of requirements and code design, and there is serious information asymmetry. For questions that cannot be solved by Google, don’t spend too much time pondering, and it is more efficient to ask RD directly.
  • Don’t be ashamed to ask implemenation-related questions. RDS need to know each other’s implementation details when reviewing code, let alone QA, for specific implementation techniques, advanced language features, and call relationships related to changes.
  • If you can’t understand the CR on a large scale and the CR is very important, you can talk to RD face to face or remotely.

5. Strictly control your time expenditure

  • Ensure CR speed, control the time cost, very important!! Especially in the case of a QA docking with multiple RD requirements, if the integration code receives 5 Merge Requests that day, CR for half an hour at a time, then almost half of the day is lost (including the time spent switching tasks). QA does CR mainly by day reading, the key code can be carefully read, do not have to understand every line.
  • Different requirements have different CR requirements. QA may be difficult to review such as platform strongly related advanced technology, large-scale multi-file changes, adjustment based on complex functions, UI requirements, etc. Coarse-grained CR is recommended. For cr-friendly requirements, such as adding a small standalone feature with fewer dependencies so that QA can get full context, more time should be spent on CR.
  • The time of CR coming is not suitable, so we should consider changing to a QA student with relevant ability to review.

6. Can give advice to RD

  • If you think there are missing scenarios or unknown risks to your code, you should point them out, either because you don’t know the context of your code, or because there is a real problem!

Some experience of QA doing CR

RD makes CR from various dimensions such as style, code design, annotation, function, performance, security, maintainability, etc. QA makes CR from functions, exception handling, performance and testability, with less but more concentrated concerns than RD. It is generally possible to follow the code Diff and go back and forth along the data flow link. Some empirical concerns are listed below.

RD can be used to explain the data flow process in person, which works wonders.

🚨 QA important test points

  • Correct initialization
  • Weak network, disconnection, failure
  • Cache clearing and cache invalidation
  • Persistent data is deleted
  • Variable to an empty
  • Crossing the line
  • End of loop condition
  • Unexpected data format and type (common server-side incidents)
  • Lock acquisition: starve to death, deadlock
  • Blocking vs. non-blocking, synchronous vs. asynchronous
  • Memory/resource leaks, especially exception logic
  • Implicit type conversion
  • The upstream and downstream interfaces are clear
  • The implementation is flawed
  • Security issues (e.g., cleartext persistence of critical information)
  • Ab experiment related code to understand the details of the open experiment

A simple but often overlooked secondary piece of information: the history Commit message.

Code testability

If you need QA to do specific testing, look at the testability of your code.

  • verifiability
    • Add the necessary logs
    • The point of change caused by the input is the output, and all the output can be viewed (positive example: falling disk of intermediate data).
    • Prompt message readable, clear and unique meaning (counterexample: ambiguous meaning of return code and multiple different errors using the same return code)
  • operability
    • Simplify test preparation and test cleaning (for example: the combination of several mark judgment conditions, can complete the mark setting, do not need manual operation scene to complete the setting)
    • The testing process has an easy-to-use companion tool
  • controllability
    • All factors affecting output are controllable (counterexample: a hard-to-construct deadlock scene; Internal anomalies that are difficult to construct)
    • Data in intermediate states can be directly controlled
  • decomposability
    • Small modules can be tested independently in large systems
  • understandability
    • Documentation is clear and up to date
    • Provides the modification background and impact scope

CR example

Here are some simple examples, which may not be exact, but just get the idea.

A variable is used when it is not initialized

// Note that Java primitives have initial values by default, but assigning them is a good habit
public class A{
    // ...
}

A a;
// ...
uploadData(a);
Copy the code


MainDic is already initialized, if null logic is always True, should be maindic.count

NSMutableDictionary *mainDic = [NSMutableDictionary dictionary];

if (mainDic){
    // ...
}
Copy the code


Such as network disconnection weak network retry, exception throw not catch, variable not null.

// The file resource is not close when an exception is thrown
try{
  OutputStream out = new FileOutputStream("");
  // ...
  out.close();
}catch(Exception e){
  e.printStackTrace();
}
Copy the code


I love to see you, you know. I don’t need an example.


int rulesForA(a){
// ...
}
int rulesForB(a){
// ...
}
A a = new A();
B b = new B()
rulesForA(b);   // Error writing variable name or function name
rulesForB(a);
Copy the code


class Test{
  static int a = 0;
  public void test(a){
    int a = 9;
    // ...
    a += 1;   // the local variable a is incremented}}Copy the code

For more on the methodology of doing CR, Google has also published a CR engineer’s practice with a link to the resources at the end of this article.

Code Review companion tools

Process platform

If you want to do CR well, you must rely on the support of the engineering platform. You cannot pull the branch code submitted by others to the local and diff it. Therefore, the basic requirements for the engineering platform are proposed for CR:

  1. Different aggregations to view diff:

    • Different warehouses involved in the change (the change involves multiple warehouses, see separately)

    • Changed file type (for example, unified view. Java file changes)

    • The directory tree of the modified file

  2. Diff codes are highlighted in color to allow you to view codes other than diff codes
  3. Support any code to add CR comments and show the resolution of the comments
  4. Show the history of the Source branch commit Message
  5. Show the source branch and the target branch to merge into
  6. Bind the document template so that RD can fill in relevant contents according to the template
  7. View all Merge requests that need to be CR

Accurately pick people

Believe there are involved in the process of CR students will face the same problem, is to cluster in demand on the date of the code, the day will receive a large number of CR invitation, don’t say work progress is at hand, receive CR invited may still isn’t relevant to own responsible modules, especially docking multiple RD, QA students probably would have been different RD invited CR. So the problem had to be solved: how to pick CR candidates more accurately.

Instead of randomly selecting CR candidates, there is a very simple solution: through background configuration, specify a code directory to match the relevant CR candidates. A code directory should have one or two principals, plus a number of candidates. When CR is selected, the principals are automatically selected, and the candidates are randomly selected. In this way, the problem of poor CR irrelevant effect caused by random selection of CR candidates is basically solved.

But with more modules, larger teams, and frequent staff turnover and code changes, is the configuration list still easy to maintain? The answer is definitely No, every time a new person comes in, you have to add that new person to the configuration, too much trouble! A more accurate way to automatically recommend CR candidates is also needed. Here is my idea:

Process overview

  1. The RD completes code development and submits the code to initiate Merge Request
  2. Obtain other files related to the modified file according to the file modified by RD and the association between the file and other files. There are x layers at the upper and lower levels (configurable)
  3. Iterate through the associated files to find the RD of each file module (if it is a child REPO, get the Repo owner; If it is a child file, git blame or git command to get the latest or most frequent changes to the RD.
  4. Pick these RDS into the CR code, or recommend them to the RD who initiated the CR and let them choose who to pull

A concrete example

  1. RD Chen modified the code file A. Java and submitted the Merge Request
  2. Function belongs to class Example. Class Example is defined and declared in B. Java and is imported by A. Java. A. Java is also used in the files C. Java and d. Java, and classes defined in c. Java and d. Java are called by e. Java and f.java. Therefore, the associated upstream and downstream file set finally obtained is [A.java, B.java, C.java, D.java, E.java, F.java], and a simple graph is attached below to express the upstream and downstream association relationship
  3. Get the CR candidates of [A.java, B.java, C.java, D.java, E.java, F.java, g.Java] code files.
  4. These candidate RDS are recommended to the CR sponsor to choose which RDS to pull into the CR

The core problem

How do I extract associations between code files? What static code analysis techniques are used?

I have been in contact with similar projects before. I used regular expressions to match different Java syntax, including: Class object creation declarations, package reference relationships, so that you know what classes a file will provide, and which classes it will use from what package. Furthermore, you can match two Java files to see if there is an association between them — you call me or I call you, which is the upstream and downstream association. For example, A. Java has class A, and B. Java introduces class A and uses it, then B. Java and A. Java have correlation.

The code analysis scheme based on regularization has many disadvantages. On the one hand, it is difficult to express the syntax through regularization, so it is hard to avoid the loss of correlation caused by omission of syntax. In addition, a large number of code format compatibility problems need to be dealt with. Slight changes in code format (for example, two more Spaces are typed and the code is displayed in the middle of a new line) may lead to miss matching of the regular and loss of association relations, so it is not recommended to use the regular for analysis.

The more common practice in the industry is to analyze file association based on the AST (Abstract Syntax Tree) obtained by Parser.

For mobile code, there are many callback functions provided by the system platform (such as the Android onCreate method, etc.), which need special treatment in relation to the relationship.

Other possible problems

  1. Can the analysis performance of association relationships be analyzed in real time? How accurate are the correlation results using offline analysis?
  2. Are extensive changes, such as code sinking and code migration, also applicable?
  3. Get through the code base submission history, have static code analysis ability to obtain the correlation between code files, according to the modification history

Code Review to the ground

Everyone should have a basic understanding of the Code Review process, so how CR should be implemented, and I will expand to tell you about it.

Ideas about

When CR is launched, the first thing we should do is to publicize the CONCEPT of CR and let people know that such a thing exists. The key is to explain why CR is being done, how CR is embedded in the current process, what people need to do more than usual, how CR is being done, and finally express the expected benefits of CR once it is officially implemented.

The process of pilot

The change of THE R&D process is a big deal. At least the addition of CR involves the change of RD and QA workflow, with dozens or even hundreds of people working up and down. Before it is 100% sure that enough benefits can be obtained, the process pilot is necessary.

It has to be mentioned that although CR itself is only a process, the effect of CR is directly related to the previous process, such as the thickness of requirements review, the quality of technical documents, the number of code comments, etc.

Before CR is fully promoted and popularized, CR can be implemented in a variety of pilot forms, such as:

  1. Make a list of people to match, several related RDS into a group, review each other’s code
  2. Part of the requirements are screened and put into CR pool, and RD selects the requirements to be reviewed and participates in CR by itself

Two types each have advantages and disadvantages, but the second form has given more freedom, are likely to be higher participation, but also to avoid demand cluster Review, especially the requirements on the technical is very tall, engineer, after all, do a lot of advanced technology is yearning, upper limit of each demand to set up a CR number, more than the number after the pool from the CR.

Rewards and punishment mechanism

Positive encouragement can accelerate the formation of CR atmosphere, and finally, to reward with material things (JD card?). , so some budget is needed 💰 and prizes are awarded according to everyone’s output. As for the punishment, of course, the responsibility for online problems will be shared proportionately.

Rewards are considered redeemable through points, so you can make a LEADERboard of CR points, which can be converted into the following indicators:

  • Take the initiative to evaluate
    • Reviewer Evaluation of the results of this CR (module familiarity, code skill improvement, etc.)
    • Evaluation of CR Effect (Reviewer will score after CR to evaluate the effect of this review)
  • Passive assessment
    • Number of problems found in CR preloading
    • Bugs per thousand lines of code (can be broken down by different dimensions: team, individual, requirements, etc.)
    • Number of bugs per requirement
    • Valid CR comments (marked as resolved)

Write in the last

According to various materials, it seems that Code Review is popular in large foreign factories, which may cause the attitude of Code Review in China to be indescribably praised or looked up to from time to time, mistakenly believing that Code Review can solve many problems. Code Review is only a supplementary means for quality assurance, and it is not a silver bullet. Don’t expect Code Review to find many key problems. Even if you invest unlimited manpower, you can only get limited benefits, and its effect is not as stable as testing.

This does not mean there is no need for Code Review. In the long run, a healthy technical team needs to practice Code Review. Especially for large-volume products, the larger the volume, the greater the impact of bugs will be, while the more cost-effective Code Review will be. Why?

If the daily activity of the product changes from 100W to 1000W, the number of users affected by online accidents with the same symptoms will differ by an order of magnitude, which means that a P2 online accident in 100W days will rise to P1 or even P0 in 1000W days.

The more complex the product Code is, the deeper the bugs will be hidden and the more constructed the problem scenes will be. Due to the tool support and the degree of construction, there are always some points that are difficult to cover in offline manual or automated testing. Code Review is just a means of fine cultivation to make up for the deficiencies of testing, especially the hidden bugs. For example, resource competition leads to deadlock starvation, abnormal processing branch error, performance problems under heavy traffic, technical debt (technical debt is also a problem, need to solve the cost in the future) and so on… These are the unique functions of Code Review.

As the product size increases, the value Code Review can provide will increase.

Saying that a Code Review is useless if it is not well done is like saying that a unit test should not be done if it is poorly written.

The resources

How to make technology from CODE REVIEW

Code Review plan

Google Engineering Practices Documentation

Code Review Guidelines for Humans