如何重构“箭头型”代码

如何重构“箭头型”代码

评论 57 条评论 116,937 人阅读

本文主要起因是，一次在微博上和朋友关于嵌套好几层的if-else语句的代码重构的讨论（微博原文），在微博上大家有各式各样的问题和想法。按道理来说这些都是编程的基本功，似乎不太值得写一篇文章，不过我觉得很多东西可以从一个简单的东西出发，到达本质，所以，我觉得有必要在这里写一篇的文章。不一定全对，只希望得到更多的讨论，因为有了更深入的讨论才能进步。

文章有点长，我在文章最后会给出相关的思考和总结陈词，你可以跳到结尾。

所谓箭头型代码，基本上来说就是下面这个图片所示的情况。

那么，这样“箭头型”的代码有什么问题呢？看上去也挺好看的，有对称美。但是……

关于箭头型代码的问题有如下几个：

1）我的显示器不够宽，箭头型代码缩进太狠了，需要我来回拉水平滚动条，这让我在读代码的时候，相当的不舒服。

2）除了宽度外还有长度，有的代码的if-else里的if-else里的if-else的代码太多，读到中间你都不知道中间的代码是经过了什么样的层层检查才来到这里的。

总而言之，“箭头型代码”如果嵌套太多，代码太长的话，会相当容易让维护代码的人（包括自己）迷失在代码中，因为看到最内层的代码时，你已经不知道前面的那一层一层的条件判断是什么样的，代码是怎么运行到这里的，所以，箭头型代码是非常难以维护和Debug的。

微博上的案例 与 Guard Clauses

OK，我们先来看一下微博上的那个示例，代码量如果再大一点，嵌套再多一点，你很容易会在条件中迷失掉（下面这个示例只是那个“大箭头”下的一个小箭头）

FOREACH(Ptr<WfExpression>, argument, node->arguments) {
    int index = manager->expressionResolvings.Keys().IndexOf(argument.Obj());
    if (index != -1) {
        auto type = manager->expressionResolvings.Values()[index].type;
        if (! types.Contains(type.Obj())) {
            types.Add(type.Obj());
            if (auto group = type->GetTypeDescriptor()->GetMethodGroupByName(L"CastResult", true)) {
                int count = group->GetMethodCount();
                for (int i = 0; i < count; i++) { auto method = group->GetMethod(i);
                    if (method->IsStatic()) {
                        if (method->GetParameterCount() == 1 &&
                            method->GetParameter(0)->GetType()->GetTypeDescriptor() == description::GetTypeDescriptor<DescriptableObject>() &&
                            method->GetReturn()->GetTypeDescriptor() != description::GetTypeDescriptor<void>() ) {
                            symbol->typeInfo = CopyTypeInfo(method->GetReturn());
                            break;
                        }
                    }
                }
            }
        }
    }
}

上面这段代码，可以把条件反过来写，然后就可以把箭头型的代码解掉了，重构的代码如下所示：

FOREACH(Ptr<WfExpression>, argument, node->arguments) {
    int index = manager->expressionResolvings.Keys().IndexOf(argument.Obj());
    if (index == -1)  continue;
    
    auto type = manager->expressionResolvings.Values()[index].type;
    if ( types.Contains(type.Obj()))  continue;
    
    types.Add(type.Obj());

    auto group = type->GetTypeDescriptor()->GetMethodGroupByName(L"CastResult", true);
    if  ( ! group ) continue;
 
    int count = group->GetMethodCount();
    for (int i = 0; i < count; i++) { auto method = group->GetMethod(i);
        if (! method->IsStatic()) continue;
       
        if ( method->GetParameterCount() == 1 &&
               method->GetParameter(0)->GetType()->GetTypeDescriptor() == description::GetTypeDescriptor<DescriptableObject>() &&
               method->GetReturn()->GetTypeDescriptor() != description::GetTypeDescriptor<void>() ) {
            symbol->typeInfo = CopyTypeInfo(method->GetReturn());
            break;
        }
    }
}

这种代码的重构方式叫 Guard Clauses

这里的思路其实就是，让出错的代码先返回，前面把所有的错误判断全判断掉，然后就剩下的就是正常的代码了。

抽取成函数

微博上有些人说，continue 语句破坏了阅读代码的通畅，我觉得他们一定没有好好读这里面的代码，其实，我们可以看到，所有的 if 语句都是在判断是否出错的情况，所以，在维护代码的时候，你可以完全不理会这些 if 语句，因为都是出错处理的，而剩下的代码都是正常的功能代码，反而更容易阅读了。当然，一定有不是上面代码里的这种情况，那么，不用continue ，我们还能不能重构呢？

当然可以，抽成函数：

bool CopyMethodTypeInfo(auto &method, auto &group, auto &symbol) 
{
    if (! method->IsStatic()) {
        return true;
    }
    if ( method->GetParameterCount() == 1 &&
           method->GetParameter(0)->GetType()->GetTypeDescriptor() == description::GetTypeDescriptor<DescriptableObject>() &&
           method->GetReturn()->GetTypeDescriptor() != description::GetTypeDescriptor<void>() ) {
        symbol->typeInfo = CopyTypeInfo(method->GetReturn());
        return false;
    }
    return true;
}

void ExpressionResolvings(auto &manager, auto &argument, auto &symbol) 
{
    int index = manager->expressionResolvings.Keys().IndexOf(argument.Obj());
    if (index == -1) return;
    
    auto type = manager->expressionResolvings.Values()[index].type;
    if ( types.Contains(type.Obj())) return;

    types.Add(type.Obj());
    auto group = type->GetTypeDescriptor()->GetMethodGroupByName(L"CastResult", true);
    if  ( ! group ) return;

    int count = group->GetMethodCount();
    for (int i = 0; i < count; i++) { auto method = group->GetMethod(i);
        if ( ! CopyMethodTypeInfo(method, group, symbol) ) break;
    }
}

...
...
FOREACH(Ptr<WfExpression>, argument, node->arguments) {
    ExpressionResolvings(manager, arguments, symbol)
}
...
...

你发出现，抽成函数后，代码比之前变得更容易读和更容易维护了。不是吗？

有人说：“如果代码不共享，就不要抽取成函数！”，持有这个观点的人太死读书了。函数是代码的封装或是抽象，并不一定用来作代码共享使用，函数用于屏蔽细节，让其它代码耦合于接口而不是细节实现，这会让我们的代码更为简单，简单的东西都能让人易读也易维护。这才是函数的作用。

嵌套的 if 外的代码

微博上还有人问，原来的代码如果在各个 if 语句后还有要执行的代码，那么应该如何重构。比如下面这样的代码。

//原版
for(....) {
    do_before_cond1()
    if (cond1) {
        do_before_cond2();
        if (cond2) {
            do_before_cond3();
            if (cond3) {
                do_something();
            }
            do_after_cond3();
        }
        do_after_cond2();
    }
    do_after_cond1();
}

上面这段代码中的那些 do_after_condX() 是无论条件成功与否都要执行的。所以，我们拉平后的代码如下所示：

//重构第一版
for(....) {
    do_before_cond1();
    if ( !cond1 ) {
        do_after_cond1();
        continue
    } 
    do_after_cond1();

    do_before_cond2();
    if ( !cond2 ) { 
        do_after_cond2();
        continue;
    }
    do_after_cond2();

    do_before_cond3();
    if ( !cond3 ) {
        do_after_cond3();
        continue;
    }
    do_after_cond3();

    do_something();  
}

你会发现，上面的 do_after_condX 出现了两份。如果 if 语句块中的代码改变了某些do_after_condX依赖的状态，那么这是最终版本。

但是，如果它们之前没有依赖关系的话，根据 DRY 原则，我们就可以只保留一份，那么直接掉到 if 条件前就好了，如下所示：

//重构第二版
for(....) {
    do_before_cond1();
    do_after_cond1();
    if ( !cond1 ) continue;
 
    do_before_cond2();
    do_after_cond2();
    if ( !cond2 ) continue;

    do_before_cond3();
    do_after_cond3();
    if ( !cond3 ) continue;

    do_something();  
}

此时，你会说，我靠，居然，改变了执行的顺序，把条件放到 do_after_condX() 后面去了。这会不会有问题啊？

其实，你再分析一下之前的代码，你会发现，本来，cond1 是判断 do_before_cond1() 是否出错的，如果有成功了，才会往下执行。而 do_after_cond1() 是无论如何都要执行的。从逻辑上来说，do_after_cond1()其实和do_before_cond1()的执行结果无关，而 cond1 却和是否去执行 do_before_cond2() 相关了。如果我把断行变成下面这样，反而代码逻辑更清楚了。

//重构第三版
for(....) {

    do_before_cond1();
    do_after_cond1();


    if ( !cond1 ) continue;  // <-- cond1 成了是否做第二个语句块的条件
    do_before_cond2();
    do_after_cond2();

    if ( !cond2 ) continue; // <-- cond2 成了是否做第三个语句块的条件
    do_before_cond3();
    do_after_cond3();

    if ( !cond3 ) continue; //<-- cond3 成了是否做第四个语句块的条件
    do_something(); 
 
}

于是乎，在未来维护代码的时候，维护人一眼看上去就明白，代码在什么时候会执行到哪里。 这个时候，你会发现，把这些语句块抽成函数，代码会干净的更多，再重构一版：

//重构第四版
bool do_func3() {
   do_before_cond2();
   do_after_cond2();
   return cond3;
}

bool do_func2() {
   do_before_cond2();
   do_after_cond2();
   return cond2;
}

bool do_func1() {
   do_before_cond1();
   do_after_cond1();
   return cond1;
}

// for-loop 你可以重构成这样
for (...) {
    bool cond = do_func1();
    if (cond) cond = do_func2();
    if (cond) cond = do_func3();
    if (cond) do_something();
}

// for-loop 也可以重构成这样
for (...) {
    if ( ! do_func1() ) continue;
    if ( ! do_func2() ) continue;
    if ( ! do_func3() ) continue;
    do_something();
}

上面，我给出了两个版本的for-loop，你喜欢哪个？我喜欢第二个。这个时候，因为for-loop里的代码非常简单，就算你不喜欢 continue ，这样的代码阅读成本已经很低了。

状态检查嵌套

接下来，我们再来看另一个示例。下面的代码的伪造了一个场景——把两个人拉到一个一对一的聊天室中，因为要检查双方的状态，所以，代码可能会写成了“箭头型”。

int ConnectPeer2Peer(Conn *pA, Conn* pB, Manager *manager)
{
    if ( pA->isConnected() ) {
        manager->Prepare(pA);
        if ( pB->isConnected() ) {
            manager->Prepare(pB);
            if ( manager->ConnectTogther(pA, pB) ) {
                pA->Write("connected");
                pB->Write("connected");
                return S_OK;
            }else{
                return S_ERROR;
            }

        }else {
            pA->Write("Peer is not Ready, waiting...");
            return S_RETRY;
        }
    }else{
        if ( pB->isConnected() ) {
            manager->Prepare();
            pB->Write("Peer is not Ready, waiting...");
            return S_RETRY;
        }else{
            pA->Close();
            pB->Close();
            return S_ERROR;
        }
    }
    //Shouldn't be here!
    return S_ERROR;
}

重构上面的代码，我们可以先分析一下上面的代码，说明了，上面的代码就是对 PeerA 和 PeerB 的两个状态 “连上”， “未连上” 做组合 “状态” （注：实际中的状态应该比这个还要复杂，可能还会有“断开”、“错误”……等等状态）， 于是，我们可以把代码写成下面这样，合并上面的嵌套条件，对于每一种组合都做出判断。这样一来，逻辑就会非常的干净和清楚。

int ConnectPeer2Peer(Conn *pA, Conn* pB, Manager *manager)
{
    if ( pA->isConnected() ) {
        manager->Prepare(pA);
    }

    if ( pB->isConnected() ) {
        manager->Prepare(pB);
    }

    // pA = YES && pB = NO
    if (pA->isConnected() && ! pB->isConnected()  ) {
        pA->Write("Peer is not Ready, waiting");
        return S_RETRY;
    // pA = NO && pB = YES
    }else if ( !pA->isConnected() && pB->isConnected() ) {
        pB->Write("Peer is not Ready, waiting");
        return S_RETRY;
    // pA = YES && pB = YES
    }else if (pA->isConnected() && pB->isConnected()  ) {
        if ( ! manager->ConnectTogther(pA, pB) ) {
            return S_ERROR;
        }
        pA->Write("connected");
        pB->Write("connected");
        return S_OK;
    }

    // pA = NO, pB = NO
    pA->Close();
    pB->Close();
    return S_ERROR;
}

延伸思考

对于 if-else 语句来说，一般来说，就是检查两件事：错误 和 状态。

检查错误

对于检查错误来说，使用 Guard Clauses 会是一种标准解，但我们还需要注意下面几件事：

1）当然，出现错误的时候，还会出现需要释放资源的情况。你可以使用 goto fail; 这样的方式，但是最优雅的方式应该是C++面向对象式的 RAII 方式。

2）以错误码返回是一种比较简单的方式，这种方式有很一些问题，比如，如果错误码太多，判断出错的代码会非常复杂，另外，正常的代码和错误的代码会混在一起，影响可读性。所以，在更为高组的语言中，使用 try-catch 异常捕捉的方式，会让代码更为易读一些。

检查状态

对于检查状态来说，实际中一定有更为复杂的情况，比如下面几种情况：

1）像TCP协议中的两端的状态变化。

2）像shell各个命令的命令选项的各种组合。

3）像游戏中的状态变化（一棵非常复杂的状态树）。

4）像语法分析那样的状态变化。

对于这些复杂的状态变化，其本上来说，你需要先定义一个状态机，或是一个子状态的组合状态的查询表，或是一个状态查询分析树。

写代码时，代码的运行中的控制状态或业务状态是会让你的代码流程变得混乱的一个重要原因，重构“箭头型”代码的一个很重要的工作就是重新梳理和描述这些状态的变迁关系。

总结

好了，下面总结一下，把“箭头型”代码重构掉的几个手段如下：

1）使用 Guard Clauses 。 尽可能的让出错的先返回， 这样后面就会得到干净的代码。

2）把条件中的语句块抽取成函数。 有人说：“如果代码不共享，就不要抽取成函数！”，持有这个观点的人太死读书了。函数是代码的封装或是抽象，并不一定用来作代码共享使用，函数用于屏蔽细节，让其它代码耦合于接口而不是细节实现，这会让我们的代码更为简单，简单的东西都能让人易读也易维护，写出让人易读易维护的代码才是重构代码的初衷！

3）对于出错处理，使用try-catch异常处理和RAII机制。返回码的出错处理有很多问题，比如：A) 返回码可以被忽略，B) 出错处理的代码和正常处理的代码混在一起，C) 造成函数接口污染，比如像atoi()这种错误码和返回值共用的糟糕的函数。

4）对于多个状态的判断和组合，如果复杂了，可以使用“组合状态表”，或是状态机加Observer的状态订阅的设计模式。这样的代码即解了耦，也干净简单，同样有很强的扩展性。

5） 重构“箭头型”代码其实是在帮你重新梳理所有的代码和逻辑，这个过程非常值得为之付出。重新整思路去想尽一切办法简化代码的过程本身就可以让人成长。

（全文完）

（转载本站文章请注明作者和出处 酷 壳 – CoolShell ，请勿用于任何商业用途）

相关文章

好烂啊有点差凑合看看还不错很精彩 (94 人打了分，平均分： 4.43 )
Loading...

《如何重构“箭头型”代码》的相关评论

    1. 那有什么问题，提前返回就可以了嘛。

      其实我从来没有写成过这么大的箭头，因为我的窗口宽度有限～

      回复

  5. “嵌套的 if 外的代码”一节，
    do_before_cond1()
    if (cond1) {…}
    do_after_cond1();
    重构成
    do_before_cond1()
    do_after_cond1();
    if (!cond1) continue;
    …
    有问题吧，如果if (condX) {…} 代码块里改变了某些状态，而do_after_condX()的逻辑依赖于这些状态的话。

    回复

    2. 如果 if 语句块中的代码改变了某些 do_after_condX() 依赖的状态，那么，原文中的重构第一版就是最终版本。

      回复

      1. do_before_cond2如果改变了do_after_cond1所依赖的状态，重构第一版也是不对的。
        不过这样的程序应该重新设计，各种before，after函数的模块化的意义几乎没有。

        回复

  6. 短路的方案也有不好的方面，说一个非短路的方案。
    嵌套可以理解为强依赖，整个大逻辑上是有状态的，要把子逻辑解耦，需要一个context（这个可以设计成函数参数传递来使函数变成无状态），context承载了每个子逻辑的条件，为了可读性和约束性，这些条件可以设计为context的属性和方法，然后每个子逻辑从context获取input，也可以output到context，这样在主逻辑线上就看不到if else了
    context = build_context(…)
    messages = {} # 逻辑清楚一点就不把messages放在context里面
    process1(context, messages)
    process2(context, messages)
    process3(context, messages)
    …
    同时，既然设计了context，还可以进一步考虑context的domain意义，可以抽象一些逻辑到context中完成，从另一个维度去重构代码。
    第一个例子感觉是为了举例而举例，重构的方案不只是解决if else的问题，而是功能职责的问题。

    回复

  8. 一般情况下，我都会用下列形式来处理多判断:

    do {
    if (! condition1) {
        break;
    }
    do_work1();
    if (! condition2) {
        break;
    }
    do_work2();
} while(0);
do_clean_up();

    这种形式可以保证一个函数只有一个入口和一个出口，可以保证所有的出错都会在while(0)的后面得到clean_up()

    回复

  9. 我个人的观点是这样的，愿意把代码写清晰写漂亮的人，只要碰到脏的代码就总会想办法把代码改好；不在乎的人，逼着他去写好，他也还是写不太好。

    回复

  10. 嵌套的 if 外的代码 那段的重构是得假设do_after_condX的执行没有顺序关系，如果像最后重构那版的话，原版代码cond1 && cond2 && cond3 == true的话，执行顺序是
    do_before_cond1()
    do_before_cond2()
    do_before_cond3()
    do_after_cond3()
    do_after_cond2()
    do_after_cond1()
    但重构后的哪一版都不是这个顺序，而且重构后都把do_before_condX和do_after_condX放在一起写了，为啥不直接重构成一个函数do_condX？
    这个重构是有问题的

    回复

  12. 之前有过类似的C++开发经历, 仅供大家参考,

    当时C11还不允许在组内使用.
    我将一部分实现抽成函数, 结果带来几个麻烦:
    1. 函数中有大量上下文需要传递. 比如do_after_cond1中需要输出
    do_after_cond2中需要的条件(而且不是主要条件)
    2. 大量的一次性函数, 以为不能使用匿名函数, 单单起一个合适的名字就成问题.一旦名字不合适就导致这个函数的误用.
    3.很多单一函数方便不能解决上下文绑定的问题,
    因为不能使用std::bind,也不能擅自引入boost库, 写了大量的伪函数类. 以至于声明的部分长度甚至超过原来函数的尺寸.
    4. Code Review的不必要麻烦, 原本一个复杂逻辑很容易被Reviewer直接忽略, 而多了很多静态声明后,Reviewer会不停的检查一下函数的基本问题, 比如输入输出检查, 函数名是否合适等等.

    最后那个项目写的很痛苦, 知道我后来开始转到一个前端项目, 用了一段时间JS和Type Script才慢慢好转.
    最近有一个项目开始用C++03了, 唉

    回复

  13. 写得很好，清晰易懂，真心不错，我做了四年的游戏开发，最终复杂的程序基本用的状态机实现的。

    回复

  14. 经常出现这样的箭头形代码往往是程序员缺乏对自己所要写的代码的思考，思考多了，考虑的角度多了自然就不会出现这样难维护可读性差的代码了。

    回复

  17. 感谢分享！已推荐到《开发者头条》：https://toutiao.io/posts/hqoj3c 欢迎点赞支持！
    欢迎订阅《后端攻城狮》https://toutiao.io/subject/57935

    回复

  18. 多用Haskell的方式来写代码，自然就会想到这些更好的形式。这些重构都是属于抽象数据类型应用的基本形式。
    如下这个就是Maybe Monad的典型应用。
    // for-loop 也可以重构成这样
    for (…) {
    if ( ! do_func1() ) continue;
    if ( ! do_func2() ) continue;
    if ( ! do_func3() ) continue;
    do_something();
    }

    ConnectPeer2Peer这个函数的最后形式就是对应Haskell的guard形式。

    函数式编程的思维在这些方面能帮助我们编写风格良好的代码。

    回复

  19. 一直使用“出错退出”的方式，goto fail 也是在动态释放的时候屡试不爽。if…else结构多了确实很头疼。

    回复

  20. “嵌套的 if 外的代码”一节，示例代码含义不够清晰吧。
    /////////////////////////////////////////////////////
    do_before_cond1()
    if (cond1) {
    ..}
    do_after_cond1();
    }
    重构第一版
    for(….) {
    do_before_cond1();
    if ( !cond1 ) {
    do_after_cond1();
    continue
    }
    do_after_cond1();
    /////////////////////////////////////////////////////
    如果 cond1 == true ，原代码是要先执行 do_before_cond2();
    而改后的代码是先执行do_after_cond1();
    所以，这种修改，只有函数调用顺序独立，没有关联性才可以。

    回复

  21. 最后的总结还是很到位的，尤其是第2条，函数和共享本质上还是两回事的，封装实现细节对外提供功能才是函数的本质。

    回复

  25. >> 函数是代码的封装或是抽象，并不一定用来作代码共享使用，函数用于屏蔽细节，让其它代码耦合于接口而不是细节实现，这会让我们的代码更为简单，简单的东西都能让人易读也易维护。这才是函数的作用。
    这句话太赞同了，函数的存在就是为了代码更易读

    回复

  28. 写这么好的代码不仅仅是技术问题，里面包含了美学，道德，职业等诸多因素的，我不做开发，会写些perl，python，改改PHP,jscript。 最近做ansible，看到旁边人写的烂代码，不忍心用，一切自己来

    回复

  34. 修正：

    原版

    for(....) {
    do_before_cond1()
    if (cond1) {
        do_before_cond2();
        if (cond2) {
            do_before_cond3();
            if (cond3) {
                do_something();
            }
            do_after_cond3();
        }
        do_after_cond2();
    }
    do_after_cond1();
}

    重构第一版

    for(....) {
    do_before_cond1();
    if ( !cond1 ) {
        do_after_cond1();
        continue
    } 

    do_before_cond2();
    if ( !cond2 ) { 
        do_after_cond2();
        continue;
    }
   
 
    do_before_cond3();
    if ( !cond3 ) {
        do_after_cond3();
        continue;
    }
    do_something();  
   do_after_cond3();
   do_after_cond2();
   do_after_cond1();
}
    回复

  36. 文中的建议用try {}catch(){}方式，

    $msg = "";
try {
      if($a){throw new Exception("aaaa");}
      if($b){throw new Exception("bbbb");}
      if($c){throw new Exception("cccc");}
} catch (\Exception $e) {
      $msg = $e->getMessage();
}
if(!empty($msg)){
  // 报错
} else {
 // 执行业务
}
    回复
  37. Pingback： 前端周刊第51期：1000个包的男人 + Vue.js + React Native + 奇技淫巧 – 1
  38. Pingback： C++ Tips – 前端开发，JQUERY特效，全栈开发，vue开发
  41. Pingback： C++ Tips _ 脚本宝典

  44. 卫语句使用比较多,使用策略模式结合工厂模式，对if else 优化也是常用的一种常用的方式。

    回复
  45. Pingback： 前端周刊第51期：1000个包的男人 + Vue.js + React Native + 奇技淫巧 – FIXBBS
  46. Pingback： 优化你的应用结构和实现Redis缓存 | 编程实战
  47. Pingback： Skybyte News - 如何重构“箭头型”代码

发表回复

您的电子邮箱地址不会被公开。 必填项已用*标注