良好的编码习惯之方法只干一件事

1. 背景

最近接手了个项目，在熟悉代码的时候，发现了好多可以优化的代码，这些代码都违反了一个规范，叫 方法只做一件事

感觉挺有记录意义的，所以来写一下这些问题代码，以及我觉得这些代码的问题点，还有就是优化

2. 示例一

2.1 问题代码

// 通过用户id判断有没有权限
func (userAuth *UserAuth) CheckPermission(uid int64, needAdminPermission bool) (hasPermission bool) {
    user, err := userAuth.GetUserInfoById(uid)
    
    // 用户存在即有权限
    if err != nil {
        hasPermission = true
    }

    // 需要管理员权限
    if needAdminPermission {
        if user.IsAdmin() {
            hasPermission = true
        } else {
            hasPermission = false
        }
    }
    
    return
}

2.2 问题点

这个CheckPermission方法做了两件事：

判断用户是否有一般的权限
判断用户是否有管理员的权限

它通过needAdminPermission这个布尔值来决定需要做哪件事，这样会带来一些问题

可读性：最大的坏处是可读性变差。实际的代码并没有那么简单，它做了多件事，相应地，逻辑会变复杂，代码行数会增加，这些不利于问题的排查、后人接手。更坏的情况下，做了多件事情的方法甚至都不能做到见名知意
灵活、可扩展性：想想，万一需求变了，需要加多一个角色权限，这个方法好扩展嘛
可测试性：一般来说，入参越少、功能越少的方法，可测试性越高，写单测越简单。如果让我写这个函数的单测，我会这样按照入参来决定用例有哪些，那么这个函数的用例就有六条，不存在的用户+true/false，普通用户+true/false，管理员+true/false

2.3 优化

将CheckPermission拆分成两个方法，一个用来校验普通用户的，一个用来校验管理员的

// 通过用户id判断有没有权限
func (userAuth *UserAuth) CheckUserPermission(uid int64) (hasPermission bool) {
    user, err := userAuth.GetUserInfoById(uid)
    // 用户存在即有权限
    if err != nil { 
        hasPermission = true
    }   
    return
}

// 通过用户id判断有没有权限
func (userAuth *UserAuth) CheckAdminPermission(uid int64) (hasPermission bool) {
    user, err := userAuth.GetUserInfoById(uid)
    // 判断是否为管理员
    if err != nil && user.isAdmin{
        hasPermission = true
    }   
    return
}

好处：

即使我需要扩展多一个角色，也只需要多一个CheckXXXPermission的方法即可，对原来的方法以及单测并无影响
简单易懂，见名知意
降低变更带来的风险
写单测也简单了。每个CheckXXXPermission方法我只需要传不存在的用户、普通用户和管理员即可，虽然用例数没变，但是组合条件是少了，单测的复杂度一下子就下来了

3. 示例二

这个例子的也是一个方法干了多件事，但是它的问题会比较掩蔽，出现问题时找问题会比较麻烦

3.1 问题代码

// 校验手机短信验证码
func (biz *SmsBiz) CheckMoileSmsCode(phone, smsCde string) (err error){
    
    // 是否需要检测图形验证码
    // 判断该手机号获取验证码的次数，超过指定次数则返回需要图形验证码的错误
    // 未超过次数，则该手机号获取验证码次数 + 1
    if err := biz.CaptchaCodeRequire(phone); err == errorutils.ErrorNeedCaptchaCode() {
        return
    }
    
    // 从缓存中获取短信验证码，然后匹配
    cacheSmsCode := biz.getSmsCodeFormCache(phone)
    if smsCode != cacheSmsCode {
        return errorutils.ErrorCodeSmsCodeNotMatch()
    }
    
    return nil
}

3.2 问题点

这个方法有副作用。什么是副作用呢？简单来讲就是方法除了完成了自己的工作之外，还对系统或者被调用者产生额外的影响

这个CheckMoileSmsCode方法看上去只是校验手机的验证码是否正确，但是它还有个副作用，就是会额外记录这个手机号获取验证码的次数

实际遇到的问题：登录校验除了一大串检验逻辑，还会调用到这个CheckMoileSmsCode方法，所以当时多次登录之后，就报了一个需要验证图形码的错误。

3.3 优化

这个 是否需要检测图形验证码CaptchaCodeRequire()的方法跟校验手机短信验证码CheckMoileSmsCode()从抽象层级来说是属于同一层级的，不太应该在CheckMoileSmsCode()中调用，应该移走。

好处：减少副作用带来的隐性危害

4. 例子三

4.1 问题代码

// 对应用的名称和地址进行校验
public void checkAppNameAndUrl(String name, String url) {
    
    // 判断应用名是否为空
    StringUtils.isBlank(name);
    
    // 判断应用地址是否为空
    StringUtils.isBlank(url);
    
}

4.2 问题点

不利于代码复用

在新建应用的时候，需要对应用的名称和地址进行校验，可是这个方法干了多件事，就是把名称和链接地址的校验捆绑了在一起

假设需求 更新应用时只能更新名称的时候，那么这个方法就不适用了

4.3 优化

4.3.1 优化一

// 对应用的参数进行校验
public void checkAppParam(AppModel appModel) {
    
    // 判断应用名是否已存在
    checkAppName(appModel.getName());
    
    // 判断应用地址是否已存在
    checkAppUrl(appModel.getUrl());
    
}

// 检验应用名称
public void checkAppName(String name) {
    StringUtils.isBlank(name);
}

// 检验应用地址
public void checkAppUrl(String url) {
     StringUtils.isBlank(url);
}

见名知意，doXXX1AndXXX2的方法一看就知道不对劲了，最好把它拆成doXXX1()和doXXX2()

利于代码复用，假如我只需要校验名称时，那么直接使用checkAppName方法即可

4.3.2 优化二

还有一种充血模式，就是把这些参数校验的工作职责也划分到AppModel这个类里面。

public class AppModel {

    private String name;
    
    private String url;
    
    // getter、setter
    
    // 对应用的参数进行校验
    public void checkParam() {
    
        // 判断应用名是否已存在
        checkName(this.getName());
    
        // 判断应用地址是否已存在
        checkUrl(this.getUrl()));
    
    }

    // 检验应用名称
    public void checkName() {
        StringUtils.isBlank(this.getName());
    }

    // 检验应用地址
    public void checkUrl() {
         StringUtils.isBlank(this.getUrl());
    }
}

我比较喜欢这种充血模式

这样才是面向对象的写法，而不像贫血模式那样只有getter、setter
对外屏蔽了校验细节，外面只需要调用一下即可校验，而不需要理解你对参数是怎么校验的。（迪米特法则）
减少业务层的逻辑

当然，这也有不好的地方，比如，检测参数这个职责应不应该划分到AppModel里呢？其实这些都是比较模糊的，不同人有不同看法。

5. 思考

CAS操作，在Java，JUC下面的atomic包里，有着大量的doXXXAndXXX()方法，例如

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

那么这里的方法算不算干了多件事情？应不应该拆分成两个方法

5.1 个人理解

判断方法是不是干了多件事，除了像示例二那样看抽象层级之外，还可以通过示例三那样看看能不能再拆出一个方法

那这里能拆成再拆出这两个方法嘛？
嗯，我觉得并不能，因为这里是要原子地执行compare和set操作，所以这里并不能拆成compare方法和set方法

其实拆也不是不行，再调用compare和set的时候加上锁不就行了。但是这样就本末倒置了，加锁的代价一般来说都会比CAS大，所以这里把compare和set合在一个方法里也挺合理的

6. 总结

职责不单一可能会带来的问题
- 可读性变差
- 可扩展性变差
- 可测试性变差
- 可能会带来额外的副作用
- 不利于方法复用
如何判断方法职责不单一
- 看名字
- 看抽象层级
- 看能不能再拆出一个方法

代码都是不断地打磨出来的，不同人有不同的理解和意见，具体场景具体分析嘛，假如大佬们有什么不错的优化想法也可以给我说说。写完收工