18 Catch-Catches for Code Review, Collect It!

preface

After we develop requirements, we generally need code review before testing. Guys, do you know what are the military rules for code review? Bringing you the 18 Catch-Catches for code review today.

1. Add the necessary comments

In fact, when writing code, there is no need to write too many comments, because good method names and variable names are the best comments. Here are some annotation specifications summarized by the author:

• All classes must have a creator and creation date, as well as a simple comment description
• Complex business logic or algorithms inside a method need to be clearly annotated
• In general, comments describe the role of classes, methods, and variables
• Any warnings or TODOs that need to be reminded should also be clearly commented
• If commenting on a line of code, use //; If you comment out a code block or interface method, there are multiple lines /* **/
• A piece of code logic If you look at it from the perspective of a stranger and can't understand it the first time, you need to add comments

Here are some annotated demos:

/**
 * @author 田螺
 * @date 2023/04/22 5:20 PM
 * @desc 田螺的实现类，捡田螺、卖田螺 （更多干货，关注公众号：捡田螺的小男孩）
 */
public class TianLuoClass {
 
    /**
     * 这是卖田螺的两法，它将两个田螺的价格整数相加并返回结果。
     * 
     * @param x 第一个整数
     * @param y 第二个整数
     * @return 两个整数的和
     */
    public int sellTianLuo(int x, int y) {
        return x + y;
    }
}           

2. Log printing specifications

The log is a good helper for quickly locating problems, and a powerful tool for fighting and throwing pots! It is very important to print a good log. If these logging specifications are not adhered to during the code review, they need to be modified:

• The log level is selected incorrectly. Common log levels are error, warn, info, and debug, don't backhand is info
• The log does not print the input parameters and response results of the call method, especially when calling across systems.
• The business log does not contain key parameters, such as userId, bizSeq, etc., which is not convenient for troubleshooting
• If the log contains key information, such as mobile phone number and ID card, you need to desensitize it
• For some unsatisfactory situations, such as some unknown exceptions (database data exceptions, etc.), or special scenarios that do not meet business expectations, you need to print relevant logs

For the log printing specification, I sorted out an article before, you can take a look at it, it's quite useful:

Summary! 15 recommendations for log printing

3. Naming conventions

Java code should be named clearly, concisely, and easy to understand. When we review code, we should pay attention to whether there is code that is not standardized and unclear in naming. Here are some naming convention recommendations:

• Classes and interfaces should use camel nomenclature with an initial capitalization
• Methods and variables should use lowercase camel nomenclature
• Constants should use all uppercase letters and underscores
• Developers choose easy-to-understand names for variables, classes, and methods

4. Parameter verification

When we review the code, we should pay attention to whether the parameters are checked, such as userId non-null check, amount range check, userName length check, and so on. Generally, when we deal with business logic, we must follow the principle of checking first and processing later.

If your database field userName is set to varchar(16), the other party passes a 32-bit string over, you do not validate the parameter, and the insertion into the database is directly exceptional.

Many bugs are caused by not doing parameter validation, and this military rule is the focus of code review:

5. Null Judgment Processing

• When obtaining the properties of an object, it must be nulled. Otherwise, there will be a null pointer exception many times.

if(object!=null){
   String name = object.getName();
}           

If you want to iterate through the list, you also need to judge the blank

if (CollectionUtils.isNotEmpty(tianLuolist)) {
        for (TianLuo temp : tianLuolist) {
            //do something
        }
    }           

6. Exception Handling Specifications

Good exception handling ensures the reliability and maintainability of your code. Therefore, exception handling is also an important specification for code reviews. Here are some suggestions for exception handling:

• Instead of catching generic exceptions, you should catch specific exceptions whenever possible
• When you catch an exception, you should log the exception information for ease of debugging
• Inner exceptions should confirm the final handling to avoid unknown exceptions being treated as failures.
• Release resources in finally blocks, or use try-with-resource
• Instead of using e.printStackTrace(), use log printing.
• If the exception is caught, the specific exception must be printed, otherwise the problem cannot be better located
• The catch exception and throw exception must match exactly, or the catch exception is the parent class of the thrown exception
• If you catch an exception, you cannot ignore it, and the corresponding log must be printed
• Watch out for exceptions that infect your code hierarchy (early detection, early handling)
• Customize the wrapper exception and do not discard the original exception information Throwable cause
• Pay attention to the order in which exceptions are matched, and catch specific exceptions first
• When providing APi to the outside world, provide the corresponding error code
• The system should throw custom exceptions with business meaning, rather than throwing RuntimeException, or Exception\Throwable.

If you are interested, you can read my previous article ha: Ten suggestions for Java exception handling

7. Modularity, scalability

When reviewing code, pay attention to whether the code writing design meets the module language and whether the interface is extensible

For example, your demand is sauce purple: when users add or modify employees, they need to brush their faces. So are you backhanding an employee-managed submission swipe information interface? Or think first: is submitting a face brush a universal process? For example, if you need to access the face brush for transfer or one-click discounting, do you need to re-implement an interface? At present, it is better to divide modules according to service types, and it is good to reuse this interface to preserve the scalability of the interface.

If divided by module, if other scenarios such as one-click discounting access to brush face in the future, there is no need to engage in a new set of interfaces, only need to add enumerations, and then reuse the brush face through the process interface to achieve the differentiation of one-click discount brush face.

8. Concurrency Control Specification

• When using concurrent collections, you should pay attention to their thread safety and concurrency performance, such as ConcurrentHashMap is linearly safe, HashMap is nonlinear safe
• Optimistic locking, pessimistic locking prevent database concurrency. Optimistic locks are generally controlled by version number, pessimistic locks are generally controlled by select ... for update
• If it is a single-instance multi-threaded concurrent processing, it is generally through Java lock mechanisms, such as sychronized and reentrantlock
• If it is multi-threaded concurrent processing in the same cluster, you can use Redis distributed lock or zookeeper
• In the case of multithreaded concurrent processing across clusters, consider the distributed locks implemented by the database.
• When using distributed locks, pay attention to what pits, such as redis some classic pits.

As for distributed locks, you can take a look at my previous articles

• 10 pits of Redis distributed locks
• Interview essentials: Talk about the multiple implementations of distributed locks!

9. Unit Test Specification

• The name of the test class is generally based on the test class + Test, such as: CalculatorTest.
• The naming of test methods, generally starting with test + test method, such as testAdd.
• The coverage of a single test line is generally required to be greater than 75%.
• The general requirements of single testing include verification use cases such as main process use cases and parameter boundary values
• Single test generally requires use cases that contain exceptions such as middleware access timeouts, return nulls, and other exceptions, such as accessing databases or Redis exceptions.
• Single test use cases are required to include concurrency, anti-weight, idempotent, and so on.

10. Code Format Specification

Good code formatting, which can make the code easier to read and understand. Here are some common code formatting suggestions:

• Indentation uses four spaces
• Code blocks are separated by curly braces
• No more than 80 characters per line
• Each method should be in a specific order, for example: class variables, instance variables, constructors, public methods, private methods, and so on.

11. Interface Compatibility

When reviewing code, focus on whether the compatibility of the interface is taken into account. Because many bugs are caused by modifying the old interface, but not making it compatible. Most of the key problems are more serious, which may directly lead to the failure of the system release. Novice programmers are easy to make this mistake~

Therefore, if your requirements are modified on the original interface, especially if this interface is an external service, you must consider that the interface is compatible. For example, for example, the dubbo interface, originally only received A and B parameters, but now you add a parameter C, you can consider this treatment:

//老接口
void oldService(A,B){
  //兼容新接口，传个null代替C
  newService(A,B,null);
}

//新接口，暂时不能删掉老接口，需要做兼容。
void newService(A,B,C){
  ...
}           

12. Whether the program logic is clear and whether the priorities are clear enough

When reviewing code, pay attention to whether the program logic is clear. For example, one of your registration interfaces has functions such as parameter verification, determining whether the user has been registered, inserting user records, and sending registration success notifications. If you cram all the functional code into one method, the program logic is not clear, the priority is not clear enough, for example:

public Response registerUser(String userName, String password, String email) {

        if (userName == null || StringUtils.isEmpty(userName)) {
          log.info("用户名不能为空！");
            throw new BizException();
        }

        if (password == null || password.length() < 6) {
            log.info("密码长度不能少于6位！");
            throw new BizException();
        }

        if (email == null || StringUtils.isEmpty(email) || !email.contains("@")) {
            log.info("邮箱格式不正确！");
            throw new BizException();
        }

        Response response = new Response();
        UserInfo userInfo = userService.queryUserInfoByUsername();
        if (Objects.nonNull(userInfo)) {
            response.setCode(0);
            response.setMsg("注册成功");
            return response;
        }


        UserInfo addUserInfo = new UserInfo();
        addUserInfo.setUserName(userName);
        addUserInfo.setPassword(password);
        addUserInfo.setEmail(email);
        userService.addUserInfo(addUserInfo);

        MessageDo messageDo = new MessageDo();
        messageDo.setUserName(userName);
        messageDo.setEmail(email);
        messageDo.setContent("注册成功");
        messageService.sendMsg(messageDo);

        response.setCode(0);
        response.setMsg("注册成功");
        return response;
    }           

In fact, the above piece of code, the primary and secondary points are not clear enough: parameter validation accounts for a large part of the registerUser method. The positive example can be divided into primary and secondary functions, and the small functions are pumped as follows:

public Response registerUser(String userName, String password, String email) {

        //检查参数
        checkRegisterParam(userName, password, email);
        //检查用户是否已经存在
        if (checkUserInfoExist(userName)) {
            Response response = new Response();
            response.setCode(0);
            response.setMsg("注册成功");
            return response;
        }

        //插入用户
        addUser(userName, password, email);
        sendMsgOfRegister(userName, email);

        //构造注册成功报文
        Response response = new Response();
        response.setCode(0);
        response.setMsg("注册成功");
        return response;
    }

    private void sendMsgOfRegister(String userName, String email) {
        MessageDo messageDo = new MessageDo();
        messageDo.setUserName(userName);
        messageDo.setEmail(email);
        messageDo.setContent("注册成功");
        messageService.sendMsg(messageDo);
    }

    private void addUser(String userName, String password, String email) {
        UserInfo addUserInfo = new UserInfo();
        addUserInfo.setUserName(userName);
        addUserInfo.setPassword(password);
        addUserInfo.setEmail(email);
        userService.addUserInfo(addUserInfo);
    }

    private boolean checkUserInfoExist(String userName) {
        UserInfo userInfo = userService.queryUserInfoByUsername();
        if (Objects.nonNull(userInfo)) {
            return true;
        }
        return false;
    }

    private void checkRegisterParam(String userName, String password, String email) {
        if (userName == null || StringUtils.isEmpty(userName)) {
            log.info("用户名不能为空！");
            throw new BizException();
        }

        if (password == null || password.length() < 6) {
            log.info("密码长度不能少于6位！");
            throw new BizException();
        }

        if (email == null || StringUtils.isEmpty(email) || !email.contains("@")) {
            log.info("邮箱格式不正确！");
            throw new BizException();
        } 
    }           

13. Safety Regulations

Code review, it is also very necessary to review the code for security issues. Like what:

• Input validation: Any input data from the outside should always be validated to ensure that it is as expected and does not cause harm to the system. Validation should include checking the type, size, and format of the data.
• Prevent SQL injection attacks: When using SQL queries, you should always use parameterized queries or prepared statements to prevent SQL injection attacks.
• Protection against cross-site scripting (XSS): In web applications, input HTML, JavaScript, and CSS should always be validated and special characters escaped to prevent XSS attacks.
• Avoid leakage of sensitive information: Sensitive information such as passwords, keys, session IDs, etc. should be encrypted during transmission and storage to prevent access by unauthorized persons. At the same time, you should avoid leaking sensitive information in logs, debug information, or error messages.
• Protection against Cross-Site Request Forgery (CSRF): CSRF tokens should be added to all sensitive operations such as changing passwords, deleting data, etc. to prevent unauthorized persons from performing these actions.
• Prevent security vulnerabilities: Highly secure algorithms and protocols (such as HTTPS, TLS) should be used to protect the transmission and storage of sensitive data, and regular vulnerability scanning and security audits should be conducted on the system.

In fact, I have written an article before, 10 solutions to ensure data security, you can take a look at Ha: 10 solutions to ensure interface data security

14. Transaction Control Specification

• It is generally recommended to use a programmatic transaction rather than a declarative transaction with an annotation @Transactional. Because there are many scenarios in which @Transactional can cause a transaction to not take effect. You can read my article ha: Meituan Two Side: 15 Scenarios in which Spring Transactions Do Not Take Effect
• The scope of the transaction should be clear, the database operation must be within the scope of the transaction, and if it is a non-database operation, try not to include it in the transaction.
• Do not make remote calls within a transaction (it may cause data inconsistencies, such as local success, but remote method failure, in which case a distributed transaction solution is required)
• Avoid processing too much data in the transaction, and some query-related operations should be put out of the transaction as much as possible (avoid large transaction problems)

15. Idempotent Processing Specifications

What is idempotency?

In computer science, idempotency means that one and multiple requests for a resource should have the same side effects, or that multiple requests have the same effect as the execution of a single request.

When reviewing code, pay attention to whether the interface considers idempotency. For example, when the account opening interface is requested many times, you need to check whether the customer has opened an account, and if the account has been successfully opened, directly return to the message of the successful account opening. If you have not opened an account, open an account first, and then return the message that the account was successfully opened. This is called idempotent treatment.

In general, there are several idempotent processing schemes:

• select+insert+primary key/unique index conflicts
• Direct insert + primary key/unique index violation
• State machine idempotent
• Extract the anti-weight table
• Token token
• Pessimistic lock
• Optimistic lock
• Distributed locks

Idempotent requires a unique token, such as a business-unique key for a database anti-weight table. It was also emphasized that multiple requests and one request had the same impact.

If you are interested in interface idempotency, you can read my previous article: Talk about idempotent design

16. Middleware Considerations (Database, Redis)

When reviewing code, if you use middleware such as databases, Redis, RocketMq, etc., we need to pay attention to some precautions for these middleware.

For example, the database:

• Pay attention to whether the database connection pool parameter settings and timeout parameters are set reasonably
• Avoid cyclic calls to database operations
• If you do not paging and query an SQL query, if the number of entries is not clear, whether the limit limit is added
• Whether the return of the database is null-vermiated
• Whether the database is slow SQL is monitored
• Whether the table structure update is compatible, and whether the existing table data involves compatibility issues
• Whether the index addition is reasonable
• Whether there are too many connected tables and so on

For example, Redis:

• Whether Redis key usage is standardized
• Redis exception capture and whether the processing logic is reasonable
• Whether the Redis connection pool and timeout parameters are set reasonably
• Whether Redis uses commands that have pits, such as hgetall, smember?
• Whether there may be problems such as cache penetration, cache snow rush, cache breakdown, etc.

Before I wrote an article about the points to note when using Redis, you can take a look at Ha: 21 points to note when using Redis

17. Watch out for bad code taste

To understand several common bad tastes of code, when you review code, you need to pay attention to some ha:

• Lots of repetitive code (common methods, design patterns)
• Too many method parameters (can be encapsulated into a DTO object)
• Method is too long (pumping function)
• Too many judgment conditions (optimization if... else）
• Do not deal with useless code (useless import)
• Avoid over-designing

18. Remote Invocation

Remote calls are a key focus of code reviews, such as:

• Don't treat timeouts as failures: remote calls can fail, such as network outages, timeouts, and so on. Developers need to pay attention to the error code returned by the remote call, unless it is an explicit failure, if it is only a timeout and other issues, it cannot be treated as a failure! Instead, you should initiate a query to confirm success and then process it.
• Exception handling: Remote calls may throw exceptions, such as due to server-side errors or incorrect request format. Therefore, developers need to ensure that these exceptions can be caught and handled to avoid system crashes or data loss.
• Network security: Since remote calls involve network communication, developers need to consider cybersecurity issues such as data encryption, authentication, access control, and more. Use secure protocols such as HTTPS or SSL/TLS whenever possible.
• Quality of Service: Remote calls can affect the performance and availability of the system. Therefore, developers need to ensure the quality of services, such as avoiding excessive use of remote calls, optimizing data transfer, implementing load balancing, etc.
• Version compatibility: Because remote calls involve communication between different processes or computers, developers need to pay attention to version compatibility between the server and the client. Use the same interfaces and data formats whenever possible to avoid incompatibilities.
• Try to avoid for loop remote calls: Try to avoid for loop remote calls, and consider interfaces that implement batch functions.

Link: https://juejin.cn/post/7228977496515379258