代码之丑（一）——让判断条件做真正的选择

    转载按：这是我在InfoQ上看到的一个关于代码书写规范的一系列的讨论，文后的评论，有些比较有意思的，我也一并摘抄下来了，大家有什么看法、想法也可以在这里继续讨论。

---

诸位看官，上代码：

if (0 == retCode) {
    SendMsg("000", "Process Success", outResult);
} else {
    SendMsg("000", "Process Failure", outResult);
}

乍一看，这段代码还算比较简短。那下面这段呢？

if(!strcmp(pRec->GetType(), RECTYPE::INSTALL)) {
    CommDM.ChangGroupInfo(const_cast(CommDM.GetAttr("IPAddress", &(pGroup->m_Attr))), true);
} else {
    CommDM.ChangGroupInfo(const_cast(CommDM.GetAttr("IPAddress", &(pGroup->m_Attr))), false);
}

看出来问题了吗？经过仔细的对比，我们发现，如此华丽的代码，if/else的执行语句真正的差异只在于一个参数。第一段代码，二者的差异只是发送的消息，第二段代码，差异在于最后那个参数。

看破这个差异之后，新的写法就呼之欲出了，以第一段代码为例：

const char* msg = (0 == retCode ? "Process Success" : "Process Failure");
SendMsg("000", msg, outResult);

为了节省篇幅，我选择了条件表达式。我知道，很多人不是那么喜欢它。如果if/else依旧是你的大爱，勇敢追求去吧！

由这段代码调整过程，我们得出一个简单的规则：

• 让判断条件做真正的选择。

对于前面调整的代码，判断条件真正判断的内容是消息的内容，而不是消息发送的过程。经过我们的调整，获取消息内容和发送消息的过程严格分离开来。

消除了代码中的冗余，代码也更容易理解，同时，给未来留出了可扩展性。如果将来retCode还有更多的情形，我们只要调整消息获取的部分进行调整就好了。当然，封装成函数是一个更好的选择，这样代码就变成了：

SendMsg("000", msgFromRetCode(retCode),outResult);

至于第二段代码的调整，留给你练手了。

这样丑陋的代码是如何从众多代码中脱颖而出的呢？很简单，只要看到，if/else执行块里面的内容相差无几，需要我们人工比字符寻找差异，恭喜你，你找到它了。

---

以下是原出处的评论（摘抄）：

2010年11月18日 上午3时45分 发表人 Yetao Chen

bool flag = !strcmp(pRec->GetType(), RECTYPE::INSTALL);
CommDM.ChangGroupInfo(const_cast(CommDM.GetAttr("IPAddress", &(pGroup->m_Attr))), flag);

当 然，非要写成一句也可以，但是我觉得，写成一句太过简练，不清楚。做一个变量也是okay的。不过，我不知道那个bool的变量的context是什么， 所以就用flag作为变量代替了。其实，我觉得，ChangeGroupInfo的第一个参数应该拿出来做成变量，再带进去，方法套方法多了的话就太乱 了。

bool flag = !strcmp(pRec->GetType(), RECTYPE::INSTALL);
var something = const_cast(CommDM.GetAttr("IPAddress", &(pGroup->m_Attr)));
CommDM.ChangGroupInfo(something, flag);

或者也可以把CommDM.GetAttr("IPAddress", &(pGroup->m_Attr))作为ChangeGroupInfo的内部调用，然后只将
&(pGroup->m_Attr)作为参数传递给ChangeGroupInfo。具体问题具体分析。

还 是上次我回复的时候说的那个现象。我所看过的我们公司代码里面，就能有比这还夸张的，一个事件响应的handler里面2个if/else,每个里面再套 3个if/else,然后这最内一层的，总共6个if/else里面的6段20行以上的代码，有80%是一样的。有的时候我就心想“写个方法调用能死 啊！”

说到底，还是态度问题。所以相比来说，这两段code还算okay了....

－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－

2010年11月18日 下午9时55分 发表人 Jinian Xie

这样的代码一般人还是很容易看懂的，个人以为代码容易懂，没有bug基本上就能算不错的代码了，当然以优雅的角度来看，是需要提炼加工的

－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－

2010年11月19日 上午1时54分 发表人 Java longhorn

这种代码还有看不懂的

这种代码的代价在于维护，如果这行代码有问题，你就知道改动的代价可能比直接写的代价还要高
所以我觉得
一开始可能会写出作者说的那种
但经过重构就可以进化到下面的那种

－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－

2010年11月19日 上午3时20分 发表人 Yetao Chen

你说的没错。

但问题是，并没有很多人认识到重构的重要性，于是乎，维护代价很高的代码就一直留了下来。

－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－

2010年11月19日 上午5时9分 发表人 Shichao Liu

我认为有一点需要补充一下， 对于仅仅参数不同分支， 也要从分支的业务意义上加以区分：
1.是恰巧逻辑相同，实则业务意义上完全独立的分支。
2.或是业务意义上就应该有完全相同的逻辑操作，而仅是数据有差异。

在需求发生演变时会有所不同。

举个例子，网游， 一次攻击可能是“普通伤害”或者“致命一击”。可能在第一个版本中仅仅是伤害数值有所差异， 而在第二个版本中需要对致命一击后追击一些额外的操作（统计次数，累积点数等等策划能想得到的点子）。

－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－

2010年11月20日 上午11时29分 发表人 李 炎

if (0 == retCode) {
SendMsg("000", "Process Success", outResult);
} else {
SendMsg("000", "Process Failure", outResult);
}
初看其实代码是差不多...有共同点.
很多情况下的代码写就写了
很少有再次整理或是优化的感觉...
其实代码也好.整体架构也好.在完成后总是能或多或少发现共同点或者说是相似点
抽离出共同的地方,在有差异的地方做处理..个人觉得这样结构上更具有美感
经常会听见强调模块化和低耦合的声音
其实哪怕一个if/else就能体现出代码是不是真的低耦合或者说模块化了..
如同砌墙一样...一块砖放的方式做出改变..往往会导致整体结构出现不必要的变化

以上都是个人愚见

－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－

2010年11月25日 上午12时3分 发表人 高 翌翔

作者所做的主要工作是消除重复，重复代码绝对是代码的坏味道！

但代码优化的脚步止于此实在可惜，俺进一步改进，以提高代码的可读性。

const char* msg = (0 == retCode ? "Process Success" : "Process Failure");
SendMsg("000", msg, outResult);

改进为：

const char* msg = GetProcessResultMessage(retCode);
SendProcessResultMessage(msg, outResult);

private char* GetProcessResultMessage(int value)
{
return (0 == value ? "Process Success" : "Process Failure");
}

private void SendProcessResultMessage(char* message, object outResult)
{
SendMsg("000", message, outResult); // 方法的首个参数语义不明确，仍有改进余地。
}

修改后的代码中，三元操作符被封装为私有方法，语义更加明确；
至于第二句由于没有源码，因此难以进一步优化，只能简单封装，将语义不明确的首个参数隐藏起来。

更详细的说明在 Bob 大叔的《Clean Code》有更详细的讲解，

俺之前只读过一遍，最近准备认真重读，需要此书 pdf 英文版的朋友可发邮件给我，邮件标题以【from InfoQ】结尾，来信必复！
My Email: [e-mail]

－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－

2010年11月25日 上午3时21分 发表人 Lu Luke

代码首先是人阅读，其次是机器阅读，基于此，我认为问题不在于if..else结构, 而在于判断条件是否可读性比较强，例如
a) if(YEAR%4==0 && YEAR%100！=0 || YEAR%400==0)..
b) if(isLeapYear(year))..
b难道不比a更好吗？

－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－

2010年11月25日 上午3时24分 发表人 Lu Luke

补充一句： 个人感觉楼主提到的代码问题，其实是重复代码的一种形式。