What do front-end code reviews usually pay attention to

Code review is also known as (Code Review, CR for short). About CR, development students are not unfamiliar. It is an indispensable link in the project development process of most companies, and the benefits of CR are familiar. . Different companies have different standards for CR methods and quality requirements. This article mainly talks about which codes and directions will be reviewed during the code review process, for everyone to use as a reference in the next CR.

1. Review node

Many projects generally put CR's time node before going online. If the project is larger, the CR node will be as close to the front as possible. Development students are less willing to modify it because the tested code only needs to be changed. , There is a risk of problems, the test also needs to return, it may produce new defects, or bring the problem to production. The following are the CR nodes in our project process. Generally, a code review will be conducted before joint debugging, after testing, and before going online.

2. Review content

1. Design plan
The design plan is generally a plan determined in the early stage of development. In the ideal state of having a perfect design plan, in the CR stage, you only need to judge when reading the code and whether to follow the design plan for development.
A relatively complete design plan usually includes:

However, the reality is that many companies do not have a design review process, and they have not carried out program design before project development. There may be several reasons:
First: The project is too small and I think it is unnecessary.
Second: I don’t realize the benefits of the design plan, and feel that time is wasted. In the process of designing the plan, it is necessary to research, design, draw, and formulate the plan. In the tight development cycle, the project hopes that the sooner the development is completed, the sooner it goes online.
Third: Even if the development classmates have made a design plan, the design plan will not be reviewed. Because each project needs to bring the relevant personnel and the person in charge of the review together to conduct a half-hour or one-hour program evaluation. Therefore, for various reasons, the final design plan has become a variant of the requirements document.

When are general comments written?
I often hear this answer: Comments should be written before the code, clear the logic and ideas in advance, and write the comments well, so that there is no need to repeatedly modify the code, and the code can be cleaner and clearer. In fact, the design plan is also like a super big comment, so that you can sort out the development ideas in advance, so that you can get twice the result with half the effort. However, making a technical design plan in advance is not only for a smoother development process, but also a major assist in the code review process.

Many times we get the code that needs to be reviewed and read it line by line from top to bottom through the order of the files. In this way, the context of each file is often not connected. We understand the first file but switch to the second file. Still incomprehensible, it is often necessary to use annotations, dictation or self-checking the context to view the code to understand. But through a complete design plan, we can understand the general requirements, the developer's design ideas, and follow the structure and ideas of the design plan to read the main line of the code.

In the projects where the design plan has been written and the design plan has passed the review, the more emphasis in code design is whether the code is developed according to the design plan, and the rationality, feasibility, and scalability of the design plan are in the design plan review The stage has passed the discussion and reached a conclusion. For projects that have no design plan or no design plan review, while understanding the code, it is also necessary to evaluate the design of the overall plan. Even if the plan design is unreasonable, once the project is tested and it is about to go online, there is no time and Conditions to make adjustments.

In design, the following aspects are generally paid attention to:

• Reuse: Can functions and components extract public methods? Do you repeat the wheel? The same code should not be repeated more than twice.
• is extensible: When the requirements change, can we use a small amount of changes to achieve our goal and adapt to future changes?
• over-design: In many cases, in order to have better scalability in the early stage, a lot of abstraction and encapsulation were carried out, which made it complicated and caused over-design.
• 1616016af9fce0 Dependency Inversion dependency between modules reasonable? Once the module is modified, can the affected area be effectively controlled?

2, maintainability
The term maintainability actually means a lot. For example, it conforms to the team’s specifications, has good complexity, high readability, good scalability, and reasonable structure. Maintenance is foreshadowing.
Each team has its own set of coding standards. During the review process, you need to pay attention to whether it conforms to the team's coding standards. However, most coding standards can be constrained by tools, and the content that can be constrained by tools should not need to spend time to pay attention to when reviewing as much as possible. You can focus on specifications outside of tool constraints, such as:

• naming: First, the format (middle line, small hump, big hump, underscore) needs to conform to the specification. Whether it is file naming or variable naming, it should conform to its functional characteristics as much as possible, and its meaning can be known through naming without adding comments to deliberately illustrate.
• Comment: Whether to add a comment in the key code? Does it meet the correct specifications?
• Complexity: Whether the complexity is within a reasonable range threshold, recommended article Front-end code quality-cyclomatic complexity principle and practice
• Unreasonable code: Every project has some old code that is difficult to maintain. Adding code on this basis may solve the current problem quickly, but it will only make it more difficult to maintain in the future. Refactoring and optimizing unreasonable old code in time is particularly important.

3. Robustness
Whether the code is safe and robust is undoubtedly very important to any team.

• XSS : XSS attack details, recommended article Front-end Security Series (1): How to prevent XSS attacks?
• CSRF : CSRF attack details, recommended article Front-end security series (2): How to prevent CSRF attacks?
• Logical boundary processing: Is there any boundary logic that takes into account the code? Is the interaction logic comprehensive?
• Exception error handling: Once an exception or error is thrown, will the page or running code crash?
• Resource release: Is the timer cleared in time? Clean up the memory in time?
• Compatibility: Is there a browser version compatible? Is the phone model compatible? Compatible with historical data? Interface compatible? Is the applet version compatible?
• Data display: For the display of key data such as assets, shopping cart amount, etc., the back-end returned data should be displayed as much as possible, and the front-end will not do calculations.
• Data verification: verify and authenticate the transmitted/received data to ensure the source and correctness of the data. Verify valid bits, calculation accuracy, completeness, consistency, timeliness (whether the timing of acquisition is correct, and whether the cache is updated)
• Data conversion: The data conversion process must be fully tested and verified, and try to select the source data for transmission, not the converted data.

4. Functional range
Many people think that the scope of functional features should be guaranteed by testing, and there is no need to pay attention to what features have been developed during code review. But in fact, the current situation is that many of our online codes often belong to private goods. For example, there was a small bug with little impact in the last iteration. Before it was discovered, we secretly brought it online, or found the last time. The code I wrote was too stupid, and there was a better solution, so the hygienic fetish broke out, and I changed it silently, and I thought I was awesome. But the test only knows the functional characteristics of this iteration, and apart from returning to the main function, it does not know that there are other places that need to be retested. If the development students happen to be very confident in their code, they think it must be okay, and the test regression is not notified. According to Murphy's law, this kind of code that is often considered to be no problem, in the end... all can cause online failures.

• influence: Modification of the underlying architecture, components or methods, whether to confirm the scope of influence, and each affected dependency can be used normally.
• Modification scope: Whether it belongs to the functional scope of this iteration that is normally online, whether there is any change to this scope, and whether the test students are notified.

5. Monitoring/buried point coverage
The prerequisite for adding monitoring is that the company has a monitoring system, in addition to the defined abnormal monitoring scenarios. Usually some new key functions, pages, etc. also need to be monitored. Add the monitoring code in advance. You don't need to wait until you want to check the problem before remembering to forget to add monitoring.

• Monitoring: Monitoring is generally used to monitor the abnormal situation of the data, the rendering of the page, the consistency and correctness of the data. For example: In the logic of some key data, if the data returned by the interface is inconsistent with the original agreement, after monitoring is added, we can quickly respond and solve the problem. You don't have to wait until more errors are caused before you can see the problem.
• Buried points: Buried points are generally used to count user operation behavior data. In most scenarios, data product managers who need to bury points will provide them.

6. Compliance
The word compliance is very common in the financial industry, but because people pay more and more attention to privacy and security, laws and regulations are becoming more and more perfect, and the word compliance is no longer unfamiliar. If the application is not compliant, it will face the risk of being offline, and some non-compliant content may not pass the test. Therefore, the review of compliance issues in the CR process is particularly important. Any operation that causes the user's privacy to be disclosed must be prohibited.

• Sensitive information display: Whether the user's key sensitive information is directly displayed.
• Sensitive information is reported in plaintext: Whether the user's key sensitive information is directly transmitted to the background in plaintext, without encryption processing or other operations.
• Sensitive information storage: to ensure the safety of user information, sensitive user information is not stored in an insecure place, such as web storage .

7, performance
C-side applications have higher requirements for front-end performance, and you can also pay attention to more common problems during code review. Front-end performance optimization 24 recommendations (2020)

• Picture size: Whether the picture has been compressed, non-page-level pictures generally should not exceed 200kb.
• http request: Too many page initialization requests? The blank screen is too long? Is the initial loading data within a reasonable range?
• Lazy loading: Can it be optimized through lazy loading or on-demand loading?
• Cached data: When data needs to be loaded repeatedly, cache data can be used to reduce requests.

8、tips

• checklist : During the review process, you can find many TODO Lists, such as adding configuration, you need to release the configuration platform before going online, such as adding a piece, remember to release the CDN, such as the test environment added test code, need to re-test in production, etc. And so on, so every time you CR, you can generate a checklist before going online, check it and execute it before going online, so you can ensure that you won’t miss it.
• less and more meals: After many CRs, it can be felt that the more code is reviewed each time, the quality will be reduced, and the review time will be too long, and there will be a sense of fatigue, and some small details will be easier to overlook. Therefore, it is better to control the time of each review at about 30min~60min. You can initiate multiple reviews and eat less and more meals~
• Good code: Every time CR is like finding faults and finding unreasonable places. But in fact, finding good and reasonable code can also promote everyone's progress. Most project CRs are not attended by all staff, so the good code is sorted out and a best practice is generated, which can be used for your study and reference, and expand some new ideas.

Third, common problems

Some time ago, I listened to a lecture by a senior CR leader of our company, and listed a lot of common problems that usually occur in the CR process. On this basis, I have made some additions and extensions for your reference.

third-party library
To add, delete, and modify the version number of a third-party library (including the addition of a small arrow), you must confirm the scope of the modification to ensure that you understand the impact of the library upgrade. Do not modify the version number arbitrarily. Even a minor version upgrade of a third-party library cannot guarantee that it will not affect the current project or current dependent packages. In order to avoid online problems, it is best to lock the version number.

// package.json

// before
{
     "dependencies": {
        "eslint": "7.27.0",
        "eslint-plugin-vue": "7.15.1",
    },
}
// after
{
     "dependencies": {
        "eslint-plugin-vue": "^7.15.1",
        "typescript": "^4.3.2"
    },
}

named
The naming is not uniform, which leads to the high cost of reading. It also indicates the number of , but the a file is named quantity , the b file is named amount , and the c file is named count .

// a.js
const quantity = 1;

// b.js
const amount = 1;

// c.js
const count = 1;

circular reference
Cyclic loading of JavaScript modules
Mutual quoting of files may cause quotation error undefined . Try to avoid interdependence between files, you can use eslint or webpack to constrain.

// a.js
import b from b.js'

// b.js
import a from 'a.js'

Complicated judgment conditions
The general logic judgment is very complicated. Generally, I did not think it clearly in the early stage, or the maintenance in the later stage continued to iterate and continue to stack up. In this case, the logic will become more and more complicated. You can consider reorganizing it during development or CR. Be clear, focus on viewing, and optimize.

if (!somethingA || somethingB && (!somethingC || somethingD)) {
    ...
}
or
if (...) {
    if (...) {
        ...
    } else if (...) {
        ...
    } else {
        ...
    }
} else {
    ...
}

Logical range change
This problem generally occurs in incremental code, because the scope of the previous conditional judgment has become smaller, which causes the scope of the subsequent logic processing to become larger. At this time, pay attention to whether this change in scope really meets expectations, or whether you just want to modify one logic, but accidentally affects the following logic.

// before
if (type === 1) {} else {}
// after
if (type === 1 && isShow) {} else {}

Exception handling
Ensure that all boundary logic has been processed or does not need to be processed.

// else为空，确认无需处理
if (conditionA) {}

// 报错catch
UserService.getList().then()

// try...catch，未处理catch
try {...} catch() {}

probability is correct! == correct
The Murphy's law mentioned earlier (Murphy's law is really easy to use): As long as everything can go wrong, it will definitely go wrong. The following setTimeout is believed to be seen by many people. The use of 500 milliseconds delay makes occasional problems "no longer appear". But in fact, the probability of occurrence is reduced, and the problems that should arise will still appear. Treat the symptoms but not the root cause. Be sure to find the cause of the problem and really solve it.

setTimeout(() => {
    ...延时拉取接口 or do something
}, 500)
=> 正确 !== 小概率出错

object chain value
There will always be a lot of such errors xxx is undefined monitoring platform, most of the reason is mainly because we like to be direct when selecting the value of the object. We recommended lodash of get or ?.?? .

const a = productObj.something.a;
=> productObj.something是否一定有值？

// lodash
const a = get(productObj, 'something.a')
// 或者双问号操作符
?.??

implicit type conversion
Implicit type conversion is prone to many problems, == can be avoided by using eslint.

if ( amount  == '22' ) {}

+new Date()

css repeat pattern
In many cases, CSS will be ignored in CR and feel that it will not produce any waves, but in projects such as small programs that have strict requirements on the size of the code package, the streamlining of CSS is very important. In many cases, when writing styles, you will write them all in one go based on the visual draft, but in fact, many inheritable attributes do not need to be written repeatedly.

.page {
    font-size: 18px;
    font-family: sans-serif;
    line-height: 18px;
    color: #000;
    ...

    .box {
        font-size: 18px;
        font-family: sans-serif;
        line-height: 18px;
        color: #000;
        ...
    }
}

Each team has different requirements and specifications for CR, and everyone can choose the one that suits them. If you have more suggestions, please leave a message to add~