Shulou(Shulou.com)06/03 Report--

With regard to the importance of Code Review, I believe all good engineers can realize it. Refer to "make Code Review a habit" and "talk about how to do technology from Code Review".

At the same time, quote someone's description of Google Code Review:

The biggest thing that makes Google's code so good is simple: code review. At Google, no code, for any product, for any project, gets checked in until it gets a positive review.

What is the main Revivew of Code Review?

Architecture/Design

The principle of single responsibility.

This is a principle that is often violated. A class can only do one thing, and a method had better do only one thing. A more common violation is to deal with the logic of voucher verification when dealing with promotions.

Whether the behavior is unified

For example, whether the cache is unified, the error handling is unified, the log printing is unified, the method is unified, and so on.

Does the same logic / same behavior follow the same Code Path? Another feature of low-quality programs is that the same behavior / logic is very difficult to maintain because it appears in different places or is triggered in different ways, does not follow the same Code Path, or has an implementation of copy everywhere.

Code pollution

Use magic numbers, such as comparing numbers directly. Write code that is not easy to read and apply a lot of complex designs.

Duplicate code

It mainly depends on whether the common components, reusable code, and functions are extracted.

Open/Closed principle

It's just good to expand. Open for extension, closed for modification. (a good interface design will solve these problems very well.

Interface-oriented programming and not implementation-oriented programming

The main thing is to see if there is a proper abstraction, abstracting some behaviors into interfaces.

Robustness

Have you considered the completeness and logic of Corner case? Is there a potential bug?

Is there a memory leak? Is there a circular dependency (for a specific language, such as Objective-C)? Is there a wild pointer?

Do you consider thread safety and consistency of data access?

Error handling

Is there a good Error Handling? For example, there is an error in the Price interface.

There is no increase in the necessary monitoring, timely alarm.

Is the change an improvement to the code?

Are the new changes patched so that the code quality continues to deteriorate, or is the code quality fixed?

Efficiency / performance

The two-tier loop, the limitation of data requests, is more time-consuming than big data and other time-consuming operations are handled properly.

What is the time complexity of the key algorithm? Is there any possibility of a potential performance bottleneck.

Complex requirements, necessary design, predictable efficiency, and consistency of development patterns should be solved in the Design Review phase as soon as possible. If the Design phase is not resolved, at least it should be found in the Code Review phase.

Style

Readability

A good practice measure of readability is whether Reviewer can easily understand the code. If not, that means the readability of the code needs to be improved.

Naming is very important for readability, and I prefer that the length of the function name / method name doesn't matter at all, it must be self-explanatory.

Try to use English words accurately (even if you need to use Google Translate sometimes, it is worth it)

Function length / class length

The function is too long to read. The class is too long, such as more than 1000 lines, so you need to see if you violate the "single responsibility" principle.

Just the right note. But more I see that one of the features of relatively poor quality projects is the lack of notes.

Number of parameters (no more than 5 parameters)

Review Your Own Code First

As with the famous rubber duck debug (Rubber Duck Debugging), it is helpful to go through your code as a whole before each submission, especially to see if you have made any low-level mistakes.

How to carry out Code Review

Ask more questions. Ask "how does this work?" "if there is a XXX case, what do you do with this?"

Do not submit too much code at a time, preferably no more than 1000 lines, otherwise review will be very inefficient.

Face-to-face discussion instead of Comments. In most cases, colleagues in the group sit together, and face to face's code review is very effective.

Distinguish the key points and don't skimp on the main points. Priority should be given to key issues such as design, readability, robustness, etc.

The consciousness of Code Review

As a Developer, we need not only Deliver working code, but also Deliver maintainable code.

Refactoring if necessary, as the project iterates, while planning new features, develop work items that should be actively planned for refactoring.

Keep an open mind and accept your Review Comments with an open mind.

Auditor and assignment of responsibilities

The so-called attribution of responsibility refers to who is responsible for this Code.

Author

Auditor

Responsibility attribution

P2.1 and below

P2.3

Auditor

P2.2 and above

P2.3

Author & reviewer

P2.3 and above

P3.1/p3.2

Author & reviewer

Check for comments. Spot check at the weekly meeting.

Audit process

Start

RD completes development self-test

I can't check myself.

Self-examination passed

Review your

Own code

RD modified according to comment's opinion

Re-test and resubmit the code

Yes

No

Is it possible to Merge

Merge to primary branch

1. Only people with permission from the main branch can Merge

End

Yes

No

Do you need it?

Code walk-through

RD initiates Code Review

As with the famous rubber duck debug (Rubber Duck Debugging), it is helpful to go through your code as a whole before each submission, especially to see if you have made any low-level mistakes.

Whether to go to school or not

1. Modified the core link code

two。 Number of modified lines of code > 600 lines

3. Modify common business components or logic

4. The auditor put forward the suggestion of day reading.

1. Select the auditor as required

two。 Set PR completion time

3. Supplement the main range of changes in code

4. Follow up PR completion status

Sponsor organization

Code Review will

Synchronously record commits on stash

No

Review

Whether to pass or not

Merge condition:

1.approve > = 2

All 2.comment have been modified.

Remarks label

Process node

Branch judgment

Figure example

Common code development specifications

Generally speaking, thrift service needs to output the log of input parameters and return parameters at info level in finally.

Implement the toString method of Object instead of using the json serialization tool to output logs

The equivalent of Integer uses equals instead of = =

Use apache commons packages instead of building your own wheels

Organize the parameter extraction method to complete instead of writing in service, and the method naming specification is buildXXXXX.

The use of thread pools needs to be unified into one Utils class to avoid flooding of thread pool definitions.

All remote service invocations need to encapsulate the delegate class, and direct use of the octo client is prohibited. In the actual call, you usually need to output the input parameters and return values of the info log in finally.

Static variables need to be uniformly defined in one class.

Instead of passing the classes generated by OCTO to the service layer, construct a local class for use.

Don't write the configuration in the code, make full use of MCC and * .properties

Configure switches and frequently modified properties in MCC, and put them into * .properties if they are not modified for a long time.

The method is named so that you and others can understand it.

Do not use pinyin abbreviations for variable names and method names

Json serialization uniformly uses JsonUtils in travel-insurance-common table

Don't write business logic in ThriftService. Sinking service

Do not copy the code

When executing the update statement, you must pay attention to the index in the where condition, which means that put the where condition in a separate select and then explain it. If it is a full table scan, the SQL will lock the whole table.

Do not write if (xxxxx&xxxx) {return true} else {return false} in the method that returns boolean

Never rpc in a transaction

It is best to subclass an exception so that you can see the cause of the exception from the cat.

Logical nesting should not exceed 3 layers

@ Transactional (rollbackFor = Exception.class) is commonly used when using transactions.

Do not use the condition generated by mybatis to spell the query. The mybatis generation tool is only responsible for generating the po of the table and the most basic insert and update. If there is a sub-table, please write all the sql by hand with the sub-table key.

Do not initialize in the static {} code block. Prohibit rpc in the static {} code block

Welcome to subscribe "Shulou Technology Information " to get latest news, interesting things and hot topics in the IT industry, and controls the hottest and latest Internet news, technology news and IT industry trends.