What exactly does Code Review focus on?

Code Review It is a very important part of the software development process.
Its main purpose is to find and fix bugs and improve software quality early in the project.

copy

This article mainly focuses on explaining which aspects of the code we care about when doing Code Review.

Everyone's focus is different. For me, my focus is generally on the following parts:

Basic articles - including coding standards, style, log specifications, memory leaks, etc.
Advanced articles - including whether there is a better abstraction, database change check, etc.
Advanced - includes contingency plans, failure considerations, etc.

Next, let's take a look at them one by one.

Basic

Basic Coding Standards

Whether the naming of classes and methods is standardized and clear
Whether there are too many method parameters (such as more than 6), whether the method body is too large
Duplicate code
Is there a "magic number", like what logic does == 1 handle? (It is recommended to have good annotations, define constants or enumerations instead)
... ...

log specification

Is the log printing meaningful? For example, the following types of logs are meaningless for troubleshooting, etc.

logger.info("method xxx implement");
... ...


logger.error("method xxx go wrong～");

copy

Log printing can be combined and printed together

#before the merger
logger.info("name={}", name);
logger.info("type={}", type);
logger.info("timeCosted={} ms", timeCosted);

#after the merger
logger.info("name={},type={},timeCosted={}", name,type,timeCosted);

copy

Whether the log is useful for MDC, if necessary, check whether the remove / clear method is called reasonably, to prevent log printing interference between threads and other issues.
... ...

Empirical check

Whether SimpleDateFormat is defined as a global variable, as in the following code, multi-threading will cause problems.

    private static final SimpleDateFormat DEFAULT_FOMAT
     = new SimpleDateFormat("yyyy-MM-dd"); 

    public static Date formatDefaultDate(String date) {
        try {
            return DEFAULT_FOMAT.parse(date);
        } catch (ParseException e) {
            return null;
        }
    }

copy

For more information on SimpleDateFormat, please refer to " SimpleDateFormat thread unsafe example and its solution>

Whether to use BigDecimal type for amount processing, float and doube cannot be used.

In addition, BigDecimal object creation may also cause problems if it is not used well. Like the following example:

    //0.299999999999999988897769753748434595763683319091796875
    BigDecimal v1 = new BigDecimal(0.3d);
    System.out.println(v1);

    //0.3
    BigDecimal v2 = new BigDecimal("0.3");
    System.out.println(v2);

    //0.3
    BigDecimal v3 = BigDecimal.valueOf(0.3d);
    System.out.println(v3);

copy

Whether the file stream is gracefully closed after use. We need to finally close the stream to prevent memory leaks.
... ...

timeout problem

Newly added services, such as Dubbo services, need timeout settings
Distributed locks need timeout processing
Calling downstream interfaces or calling various middleware requires a timeout mechanism
... ...

Code responsibility and calling relationship

Whether the code responsibilities are clear, for example, UserService has written a lot of logic processing methods related to permission resources, which is not good.
Whether the hierarchical call is correct, generally Controller -> Service -> DAO. Not good if Controller -> DAO
Dubbo services do not call their own services. How to understand this? For example, there is a UserRpcService, which has update and query interfaces. Assume that the update needs to perform user query operations to make judgments, and the query can be satisfied.

public class UserRpcServiceImpl implements UserRpcService {

  public int update(xxx) {

      //Call query, and query is also an rpc interface 
      query(xxx);
  }

  public int query(xxx) {


  }

}

copy

It is not recommended to call its own rpc interface in the rpc interface, such as update and query in this example.

... ...

Thread pool creates threads

For some multi-threaded logic, use the thread pool to create threads instead of creating them through new Thread.
... ...

Advanced

database check

To add fields to the table Alter table xxx add column xxxx ... , you need to check the number of records in the current table. If the amount of data is large, this cannot be done. Need to communicate with the DBA. Or by building a new table (one more field than the original table) and replacing the old table with the new table. Here you need to complete the original table data migration and other content.
Is it appropriate to add the index
Is there any dangerous SQL, such as whether the variables in the update/delete statement can guarantee the necessary value in the business, and there cannot be many values, which will cause the if test to be unsatisfactory and cause the scope of the update to expand.

  <if test="xxx !=null ">

copy

Check to see if there is a single record inserted in a loop, and change it to batch insert.
... ...

Security Penetration Check

Does the file upload only judge the file suffix? Only by judging the suffix, the attacker can disguise a jsp or other file as a jpg or other format file, thereby successfully uploading to the service, resulting in server information leakage.
SMS verification code For a mobile phone number, can calling the interface send a new SMS verification code to the user, thereby causing SMS bombing? We need to ensure that the SMS verification code will not be regenerated within the validity period of the SMS.
Are there risks such as unauthorized viewing of the interface? For example, A can view the device information belonging to B through the id?
... ...

Interface Protection Check

Does the list query have a pageSize limit (for example, a maximum of 100 items can be queried at one time). If there is no limit, then assuming that the pageSize can be 5000, it is really simple, right?
If the number of interface calls needs to be limited, we also need to consider whether there are measures for limited flows such as methods?
... ...

schema application

If D.C.L (Double-Check Lock) is used, then see if there is a volatile modification
Is the abstraction sufficient? For example, there are different types of service interface calls, and the main steps are as follows:

1,Parameter check
2,Check for traffic
3,Execute business logic
4,Record call log
5,flow deduction
6,... ...

copy

If each service is written by itself, it is not very suitable, and it is not easy to maintain and expand. In this case, different service calls are mainly step #1 and step #3 have personalized processing, which can be abstracted.

For example, to write a simple template, step #1 and step #3 use the abstract method, which is implemented by subclasses.

abstract class

public abstract class AbstractServiceHandler<T extends ServiceRequest> {

  /**
   * template method
   */
  public void handle(T param) {
    // 1. Verification
    this.doValidate(param);

    // 2. Check whether there is traffic

    // 3. Execute business logic
    doBusiness(param);

    // 4. Record the call log
    // 5. Traffic deduction
    // 6, ... ...
  }

  // subclass for validation
  protected abstract void doValidate(T param);

  // Subclasses do business logic
  protected abstract void doBusiness(T param);

}

copy

Service Request Parameters

import java.io.Serializable;

public class ServiceRequest implements Serializable {

  private static final long serialVersionUID = 4314529075012784996L;

  // attribute omitted

}

copy

Decision flow service processing class

public class DecisionFlowHandler extends AbstractServiceHandler<ServiceRequest> {

  @Override
  protected void doValidate(ServiceRequest param) {
    // TODO Auto-generated method stub

  }

  @Override
  protected void doBusiness(ServiceRequest param) {
    // TODO Auto-generated method stub

  }

}

copy

Decision tool service processing class

public class DecisionToolHandler extends AbstractServiceHandler<ServiceRequest> {

  @Override
  protected void doValidate(ServiceRequest param) {
    // TODO Auto-generated method stub

  }

  @Override
  protected void doBusiness(ServiceRequest param) {
    // TODO Auto-generated method stub

  }

}

copy

Some design principles can be considered, such as single responsibility/interface isolation, etc.

You can refer to " Principles of Design Patterns>

... ...

Advanced

In this part of the chapter, we have to consider some "failure" scenarios, solutions & contingencies.

Retries and idempotence

For data synchronization between systems, if it fails? Do we have a retry mechanism?
For scenarios such as billing, retry the call after failure, does our interface support idempotence?
If the file import task is interrupted, do we have a mechanism to restart the task and continue to complete?
... ...

Plans and contingencies

Add attributes to the cache object (for example, add a type to User), and check whether the cached Key has version adjustments when it is released online. If the Key remains unchanged after the upgrade, the value of Redis may be updated by the original service, and the original value (excluding type) may still be taken after the newly changed content is launched.
For example, in some scenarios, whether there is a switch for Redis cache addition (generally pushed by the configuration center), to prevent the use of the database to guarantee the bottom in the scenario where the cache is not very correct
For example, when it comes to data migration or Redis cluster upgrade (changed from 5.0 to 6.0), is the flow switching plan reasonable?
Sometimes in order to reduce RPC calls or less network travel, it will be used in combination with Redis (distributed) + Guava (local cache). What is the strategy for updating the local cache? (Under the cluster, message broadcasting is required to achieve the purpose of quickly updating the local cache of each machine)
Is the value stored in the cache a large object, and how large is the cache? What is the failure strategy? Are scenarios such as cache avalanche/concurrency considered, etc.
... ...

For more knowledge about caching, readers can read from the previous article " aha! cache » Learn more.

summary

This article mainly focuses on the code itself, makes a simple explanation of Code Review, and writes whatever comes to mind.

Code Review's focus on code is far more than that. This article is also a simple introduction. Interested readers can leave a message to discuss together.

Tags: Cache

Posted by lisa99 on Mon, 21 Nov 2022 23:57:34 +1030

Programs Wiki

What exactly does Code Review focus on?

Basic

Advanced

Advanced

summary

Popular Tags