The method of good coding habits does only one thing

1. Background

I recently took over a project. When I was familiar with the code, I found a lot of code that can be optimized. These codes violated a specification and asked the method to do only one thing

I think it's very meaningful to record, so let's write these problem codes, and I think the problem points of these codes, as well as optimization

2. Example 1

2.1 problem code

// Determine whether you have permission by user id
func (userAuth *UserAuth) CheckPermission(uid int64, needAdminPermission bool) (hasPermission bool) {
    user, err := userAuth.GetUserInfoById(uid)
    
    // Users have permissions when they exist
    if err != nil {
        hasPermission = true
    }

    // Administrator privileges are required
    if needAdminPermission {
        if user.IsAdmin() {
            hasPermission = true
        } else {
            hasPermission = false
        }
    }
    
    return
}

2.2 problem points

This CheckPermission method does two things:

Judge whether the user has general permissions
Judge whether the user has administrator permission

It uses the Boolean needAdminPermission to determine what needs to be done, which will cause some problems

Readability: the biggest disadvantage is poor readability. The actual code is not so simple. It does many things. Accordingly, the logic will become complex and the number of lines of code will increase, which is not conducive to troubleshooting and future generations. Worse, the method of doing many things can't even see the name and know the meaning
Flexibility and scalability: think about it. In case the requirements change, you need to add one more role permission. This method is easy to expand
Testability: Generally speaking, the methods with fewer input parameters and fewer functions have higher testability and easier to write a single test. If I am asked to write a single test of this function, I will decide which use cases there are according to the input parameters. Then there are six use cases of this function, including non-existent users + true/false, ordinary users + true/false, and administrators + true/false

2.3 optimization

Split CheckPermission into two methods, one for verifying ordinary users and the other for verifying administrators

// Determine whether you have permission by user id
func (userAuth *UserAuth) CheckUserPermission(uid int64) (hasPermission bool) {
    user, err := userAuth.GetUserInfoById(uid)
    // Users have permissions when they exist
    if err != nil { 
        hasPermission = true
    }   
    return
}

// Determine whether you have permission by user id
func (userAuth *UserAuth) CheckAdminPermission(uid int64) (hasPermission bool) {
    user, err := userAuth.GetUserInfoById(uid)
    // Judge whether it is an administrator
    if err != nil && user.isAdmin{
        hasPermission = true
    }   
    return
}

Benefits:

Even if I need to expand one more role, I only need one more checkxxpermission method, which has no impact on the original method and single test
Easy to understand, see the name know the meaning
Reduce the risk of change
It's also easy to write a single test. For each checkxxpermission method, I only need to pass it to non-existent users, ordinary users and administrators. Although the number of use cases has not changed, the combination conditions are less, and the complexity of single test is reduced at once

3. Example 2

This example is also a method that does many things, but its problems will be more masked, and it will be more troublesome to find problems when problems occur

3.1 problem code

// Verify SMS verification code
func (biz *SmsBiz) CheckMoileSmsCode(phone, smsCde string) (err error){
    
    // Need to detect graphic verification code
    // Judge the number of times the mobile phone number obtains the verification code. If it exceeds the specified number, an error requiring the graphic verification code will be returned
    // If the number of times is not exceeded, the number of times the mobile phone number obtains the verification code + 1
    if err := biz.CaptchaCodeRequire(phone); err == errorutils.ErrorNeedCaptchaCode() {
        return
    }
    
    // Get the SMS verification code from the cache and match it
    cacheSmsCode := biz.getSmsCodeFormCache(phone)
    if smsCode != cacheSmsCode {
        return errorutils.ErrorCodeSmsCodeNotMatch()
    }
    
    return nil
}

3.2 problem points

This method has side effects. What are side effects? In short, the method not only completes its own work, but also has an additional impact on the system or the callee

The checkmobilesmscode method seems to check whether the verification code of the mobile phone is correct, but it also has a side effect that it will record the number of times the mobile phone number obtains the verification code

Actual problems: in addition to a large string of verification logic, the login verification also calls the checkmoilesmcscode method. Therefore, after multiple logins, an error requiring to verify the graphics code was reported.

3.3 optimization

Whether this method needs to detect the graphic verification code CaptchaCodeRequire() is checked at the same level from the abstract level of the mobile phone short message verification code CheckMoileSmsCode(). It should not be called in CheckMoileSmsCode(), and should be removed.

Benefits: reduce the hidden harm caused by side effects

4. Example 3

4.1 problem code

// Verify the name and address of the application
public void checkAppNameAndUrl(String name, String url) {
    
    // Judge whether the application name is empty
    StringUtils.isBlank(name);
    
    // Determine whether the application address is empty
    StringUtils.isBlank(url);
    
}

4.2 problem points

It is not conducive to code reuse

When creating a new application, you need to verify the name and address of the application, but this method does many things, that is, it binds the verification of the name and link address together

Assuming that only the name can be updated when the requirement is updated, this method is not applicable

4.3 optimization

4.3.1 optimization I

// Verify the applied parameters
public void checkAppParam(AppModel appModel) {
    
    // Determine whether the application name already exists
    checkAppName(appModel.getName());
    
    // Determine whether the application address already exists
    checkAppUrl(appModel.getUrl());
    
}

// Inspection application name
public void checkAppName(String name) {
    StringUtils.isBlank(name);
}

// Inspection application address
public void checkAppUrl(String url) {
     StringUtils.isBlank(url);
}

The method of doXXX1AndXXX2 is not right at first sight. It's best to disassemble it into doXXX1() and doXXX2()

It is convenient for code reuse. If I only need to verify the name, I can directly use the checkAppName method

4.3.2 optimization II

There is also a congestion mode, which divides the responsibilities of these parameter verification into the AppModel class.

public class AppModel {

    private String name;
    
    private String url;
    
    // getter,setter
    
    // Verify the applied parameters
    public void checkParam() {
    
        // Determine whether the application name already exists
        checkName(this.getName());
    
        // Determine whether the application address already exists
        checkUrl(this.getUrl()));
    
    }

    // Inspection application name
    public void checkName() {
        StringUtils.isBlank(this.getName());
    }

    // Inspection application address
    public void checkUrl() {
         StringUtils.isBlank(this.getUrl());
    }
}

I prefer this congestion mode

This is the object-oriented writing method, instead of only getter s and setter s like anemia mode
The verification details are shielded from the outside. You only need to call it outside to verify, without understanding how you verify parameters( Demeter's law)
Reduce business layer logic

Of course, there are some disadvantages. For example, should the responsibility of detecting parameters be divided into AppModel? In fact, these are relatively vague, and different people have different views.

5. Thinking

For CAS operations, there are a large number of doxxandxxx () methods in the atomic package under Java and JUC, such as

public final boolean compareAndSet(int expect, int update) {
    return unsafe.compareAndSwapInt(this, valueOffset, expect, update);
}

So does the method here do many things? Should it be split into two methods

5.1 personal understanding

Judge whether the method has done many things. In addition to looking at the abstraction level as in example 2, you can also see whether another method can be removed as in example 3

Can we tear down these two methods here?

Well, I don't think so. Because we want to perform compare and set operations atomically, we can't split them into compare and set methods

In fact, it's not impossible to disassemble. Just lock it when you call compare and set. But this puts the cart before the horse. Generally speaking, the cost of locking is greater than CAS, so it is reasonable to combine compare and set in one method

6. Summary

Possible problems caused by different responsibilities
- Poor readability
- Poor scalability
- Poor testability
- May cause additional side effects
- It is not conducive to method reuse
How to judge methods? Responsibilities are not single
- Look at the name
- Look at the level of abstraction
- See if we can find another way

The code is constantly polished. Different people have different understandings and opinions. Specific scenarios are analyzed. If the bosses have any good optimization ideas, you can tell me. Finish writing

Keywords: Java Go

Added by Clerma on Sun, 05 Sep 2021 01:41:22 +0300

Programming VIP