“代码之丑”系列文章转载

来源：互联网发布：淘宝订单提成编辑：程序博客网时间：2024/05/17 02:55

这几天按照惯例上InfoQ看一些新动态，拜读了其中的“代码之丑”的系列文章，颇有感触，特别将该系列的原文进行转载，在贴出原文之前，先了解一下作者的背景。

作者简介：郑晔，ThoughtWorks公司咨询师，拥有多年企业级软件开发经验，热衷于探索各种程序设计语言在真实软件开发中所能发挥的威力，致力于探寻合理的软件开发方式，加入ThoughtWorks公司后，投入到敏捷开发方法的实践之中，为其他公司提供敏捷开发方法方面的咨询服务。他的blog是梦想风暴。

代码之丑（零）

原文出处：http://dreamhead.blogbus.com/logs/80223223.html

看到下面这段代码，你会做何感想？
if(db.Next()) {
return true;
} else {
return false;
}

有的人会想，怎么写得这么笨啊！但是，请放心，绝对会有人这么想，挺好的，实现功能了。这并非我臆造出的代码，而是从一个真实的codebase上找到。

成为一个咨询师之后，我有机会在不同的项目中穿梭。同客户合作的过程中，我经常干的一件事是：codediff。也就是用源码管理工具的diff功能把当天全部修改拿出来，从编码的角度来分析代码写得怎么样。

因为这个工作，我看到了许多不同人编写的代码，我的编码底线不断受到挑战。许多东西，我以为是常识，但实际上不为许多人所知，比如上面那段代码。

我把在看到的问题总结成一个session，与客户的程序员分享。结束之后，有人建议，为什么不能把这些问题写下来，与更多人分享。于是，我产生了写下《代码之丑》念头，以此向《代码之美》致敬。

最后要说的是，开始那段代码可以写成这样：
return db.Next();

本文已经首发于InfoQ中文站，版权所有，原文为《专栏：代码之丑——开篇》，如需转载，请务必附带本声明，谢谢。
InfoQ中文站是一个面向中高端技术人员的在线独立社区，为Java、.NET、Ruby、SOA、敏捷、架构等领域提供及时而有深度的资讯、高端技术大会如QCon 、免费迷你书下载如《架构师》等。

代码之丑（一）

原文出处：http://dreamhead.blogbus.com/logs/80612297.html

诸位看官，上代码：
if (0 == iRetCode) {
    this->SendPeerMsg("000", "ProcessSuccess", outRSet);
} else {
    this->SendPeerMsg("000", "ProcessFailure", outRSet);
}

乍一看，这段代码还算比较简短。那下面这段呢？
if(!strcmp(pRec->GetRecType(), PUB_RECTYPE::G_INSTALL)) {
    CommDM.jkjtVPDNResOperChangGroupInfo(
      const_cast(CommDM.GetProdAttrVal("vpdnIPAddress",
      &(pGroupSubs->m_ProdAttr))),
      true);
} else {
    CommDM.jkjtVPDNResOperChangGroupInfo(
     const_cast(CommDM.GetProdAttrVal("vpdnIPAddress",
      &(pGroupSubs->m_ProdAttr))),
      false);
}

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

看破这个差异之后，新的写法就呼之欲出了，以第一段代码为例：
const char* msg = (0 == iRetCode ? "Process Success" :"Process Failure");
this->SendPeerMsg("000", msg, outRSet);

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

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

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

这里判断条件真正判断的内容是消息的内容，而不是消息发送的过程。经过我们的调整，得到消息内容和和发送消息的过程严格分离开来。

消除了代码中的冗余，代码也更容易理解，同时，给未来留出了可扩展性。如果将来iRetCode还有更多的情形，我们只要在消息获取的时候进行调整就好了。当然，封装成一个函数是一个更好的选择，这样代码就变成了：
this->SendPeerMsg("000", peerMsgFromRetCode(iRetCode),outRSet);

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

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

InfoQ中文站是一个面向中高端技术人员的在线独立社区，为Java、.NET、Ruby、SOA、敏捷、架构等领域提供及时而有深度的资讯、高端技术大会如QCon 、免费迷你书下载如《架构师》等。

代码之丑（二）

原文出处：http://dreamhead.blogbus.com/logs/81144620.html

这是一个长长的判断条件：
if ( strcmp(rec.type, "PreDropGroupSubs") == 0
    || strcmp(rec.type, "StopUserGroupSubsCancel") ==0
    || strcmp(rec.type, "QFStopUserGroupSubs") == 0
    || strcmp(rec.type, "QFStopUserGroupSubsCancel")== 0
    || strcmp(rec.type, "QZStopUserGroupSubs") == 0
    || strcmp(rec.type, "QZStopUserGroupSubsCancel")== 0
    || strcmp(rec.type, "SQStopUserGroupSubs") == 0
    || strcmp(rec.type, "SQStopUserGroupSubsCancel")== 0
    || strcmp(rec.type, "StopUseGroupSubs") == 0
    || strcmp(rec.type, "PreDropGroupSubsCancel") ==0)

之所以注意到它，因为最后两个条件是最新修改里面加入的，换句话说，这不是一次写就的代码。单就这一次而言，只改了两行，这是可以接受的。但这是遗留代码。每次可能只改了一两行，通常我们会不只一次踏入这片土地。经年累月，代码成了这个样子。

这并非我接触过的最长的判断条件，这种代码极大的开拓了我的视野。现在的我，即便面对的是一屏无法容纳的条件，也可以坦然面对了，虽然显示器越来越大。

其实，如果这个判断条件是这个函数里仅有的东西，我也就忍了。遗憾的是，大多数情况下，这只不过是一个更大函数中的一小段而已。

为了让这段代码可以接受一些，我们不妨稍做封装：
bool shouldExecute(Record& rec) {
    return (strcmp(rec.type, "PreDropGroupSubs") == 0
      || strcmp(rec.type,"StopUserGroupSubsCancel") == 0
      || strcmp(rec.type,"QFStopUserGroupSubs") == 0
      || strcmp(rec.type,"QFStopUserGroupSubsCancel") == 0
      || strcmp(rec.type, "QZStopUserGroupSubs")== 0
      || strcmp(rec.type,"QZStopUserGroupSubsCancel") == 0
      || strcmp(rec.type,"SQStopUserGroupSubs") == 0
      || strcmp(rec.type,"SQStopUserGroupSubsCancel") == 0
      || strcmp(rec.type,"StopUseGroupSubs") == 0
      || strcmp(rec.type,"PreDropGroupSubsCancel") == 0);
}

if (shouldExecute(rec)) {
    ...
}

现在，虽然条件依然还是很多，但和原来庞大的函数相比，至少它已经被控制在一个相对较小的函数里了。更重要的是，通过函数名，我们终于有机会说出这段代码判断的是什么了。

提取函数把这段代码混乱的条件分离开来，它还是可以继续改进的。比如，我们把判断的条件进一步提取：
bool shouldExecute(Record& rec) {
static const char* execute_type[] = {
    "PreDropGroupSubs",
    "StopUserGroupSubsCancel",
    "QFStopUserGroupSubs",
    "QFStopUserGroupSubsCancel",
    "QZStopUserGroupSubs",
    "QZStopUserGroupSubsCancel",
    "SQStopUserGroupSubs",
    "SQStopUserGroupSubsCancel",
    "StopUseGroupSubs",
    "PreDropGroupSubsCancel"
};

int size = ARRAY_SIZE(execute_type);
for (int i = 0; i < size; i++) {
    if (strcmp(rec.type, execute_type[i]) == 0) {
      return true;
    }
}

return false;
}

这样的话，再加一个新的type，只要在数组中增加一个新的元素即可。如果我们有兴趣的话，还可以进一步对这段代码进行封装，把这个type列表变成声明式，进一步提高代码的可读性。

发现这种代码很容易，只要看到在长长的判断条件，就是它了。要限制这种代码的存在，我们只要以设定一个简单的规则：

· 判断条件里面不允许多个条件的组合

在实际的应用中，我们会把“3”定义为“多”，也就是如果有两个条件的组合，可以接受，如果是三个，还是改吧！

虽然通过不断调整，这段代码已经不同于之前，但它依然不是我们心目中的理想代码。出现这种代码，往往意味背后有更严重的设计问题。不过，它并不是这里讨论的内容，这里的讨论就到此为止吧！

代码之丑（二）（续）

原文出处：http://dreamhead.blogbus.com/logs/81550344.html

sinojelly在《代码之丑（二）》的评论里问了个问题，“把这个type列表变成声明式”，什么样的声明式？

好吧！我承认，我偷懒了，为了省事，一笔带过了。简单理解声明式的风格，就是把描述做什么，而不是怎么做。一个声明式编程的例子是Rails里面的数据关联，为人熟知的has_many和belongs_to。通过声明，模型类就会具备一些数据关联的能力。

具体到实际开发里，声明式编程需要有两个部分：一方面是一些基础的框架性代码，另一方面是应用层面如何使用。框架代码通常来说，都不像应用层面代码那么好理解，但有了这个基础，应用代码就会变得简单许多。

针对之前的那段代码，按照声明性编程风格，我改造了代码，下面是框架部分的代码：

#define BEGIN_STR_PREDICATE(predicate_name) /
bool predicate_name(const char* field) { /
static const char* predicate_true_fields[] = {

#define STR_PREDICATE_ITEM(item) #item ,

#define END_STR_PREDICATE /
};/
/
int size = ARRAY_SIZE(predicate_true_fields);/
for (int i = 0; i < size; i++) { /
    if (strcmp(field, predicate_true_fields[i]) == 0) {/
        return true;/
    }/
}/
/
return false;/
}

这里用到了C/C++常见的宏技巧，为的就是让应用层面的代码写起来更像声明。对比一下之前的函数，就会发现，实际上二者几乎是一样的。有了框架，就该应用了：

BEGIN_STR_PREDICATE(shouldExecute)
STR_PREDICATE_ITEM(PreDropGroupSubs)
STR_PREDICATE_ITEM(StopUserGroupSubsCancel)
STR_PREDICATE_ITEM(QFStopUserGroupSubs)
STR_PREDICATE_ITEM(QFStopUserGroupSubsCancel)
STR_PREDICATE_ITEM(QZStopUserGroupSubs)
STR_PREDICATE_ITEM(QZStopUserGroupSubsCancel)
STR_PREDICATE_ITEM(SQStopUserGroupSubs)
STR_PREDICATE_ITEM(SQStopUserGroupSubsCancel)
STR_PREDICATE_ITEM(StopUseGroupSubs)
STR_PREDICATE_ITEM(SQStopUserGroupSubsCancel)
STR_PREDICATE_ITEM(StopUseGroupSubs)
STR_PREDICATE_ITEM(PreDropGroupSubsCancel)
END_STR_PREDICATE

shouldExecute就此重现出来了。不过，这段代码已经不再像一个函数，而更像一段声明，这就是我们的目标。有了这个基础，实现一个新的函数，不过是做一段新的声明而已。

接下来就是如何使用了，与之前略有差异的是，这里为了更好的通用性，把字符串作为参数传了进去，而不是原来的整个类对象。
shouldExecute(r.type);

虽然应用代码变得简单了，但写出框架的结构是需要一定基础的。它不像应用代码那样来得平铺直叙，但其实也没那么难，只不过很多人从没有考虑把代码写成这样。只要换个角度去思考，多多练习，也就可以驾轻就熟了。

代码之丑（三）

原文出处：http://dreamhead.blogbus.com/logs/82256362.html

又见switch：
switch(firstChar) {
case ‘N’:
    nextFirstChar = ‘O’;
    break;
case ‘O’:
    nextFirstChar = ‘P’;
    break;
case ‘P’:
    nextFirstChar = ‘Q’;
    break;
case ‘Q’:
    nextFirstChar = ‘R’;
    break;
case ‘R’:
    nextFirstChar = ‘S’;
    break;
case ‘S’:
    nextFirstChar = ‘T’;
    break;
case ‘T’:
    throw Ebusiness();
default:
}

出于多年编程养成的条件反射，我对于switch总会给予更多的关照。研习面向对象编程之后，看见switch就会想到多态，遗憾的是，这段代码和多态没什么关系。仔细阅读这段代码，我找出了其中的规律，nextFirstChar就是firstChar的下一个字符。于是，我改写了这段代码：
switch(firstChar) {
case ‘N’:
case ‘O’:
case ‘P’:
case ‘Q’:
case ‘R’:
    nextFirstChar = firstChar + 1;
    break;
case ‘T’:
    throw Ebusiness();
default:
}

现在，至少看起来，这段代码已经比原来短了不少。当然这么做基于一个前提，也就是这些字母编码的顺序确确实实连续的。从理论上说，开始那段代码适用性更强。但在实际开发中，我们碰到字母不连续编码的概率趋近于0。

但这段代码究竟是如何产生的呢？我开始研读上下文，原来这段代码是用当前ID产生下一个ID的，比如当前是N0000，下一个就是N0001。如果数字满了，就改变字母，比如当前ID是R9999，下一个就是T0000。在这里，字母也就相当于一位数字，根据情况进行进位，所以有了这段代码。

代码上的注释告诉我，字母的序列只有从N到T，根据这个提示，我再次改写了这段代码：
if (firstChar >= ‘N’ && firstChar <= ‘S”) {
    nextFirstChar = firstChar + 1;
} else {
    throw Ebusiness();
}

这里统一处理了字母为T和default的情形，严格说来，这和原有代码并不完全等价。但这是了解了需求后做出的决定，换句话说，原有代码在这里的处理中存在漏洞。

修改这段代码，只是运用了非常简单的编程技巧。遗憾的是，即便如此简单的编程技巧，也不是所有开发人员都驾轻就熟的，很多人更习惯于“平铺直叙”。这种直白造就了代码中的许多鸿篇巨制。我听过不少“编程是体力活”的抱怨，不过，能把写程序干成体力活，也着实不值得同情。写程序，不动脑子，不体力才怪。

无论何时何地，只要switch出现在眼前，请提高警惕，那里多半有坑。

代码之丑（四）

原文出处：http://dreamhead.blogbus.com/logs/82703006.html

这是一个找茬的游戏，下面三段代码的差别在哪：
if ( 1 == SignRunToInsert)    {
    RetList->Insert(i, NewCatalog);
} else {
    RetList->Add(NewCatalog);
}

if ( 1 == SignRunToInsert)    {
    RetList->Insert(m, NewCatalog);
} else {
    RetList->Add(NewCatalog);
}

if ( 1 == SignRunToInsert)    {
    RetList->Insert(j, NewPrivNode);
} else {
    RetList->Add(NewPrivNode);
}

答案时间：除了用到变量之外，完全相同。我想说的是，这是我从一个文件的一次diff中看到的。

不妨设想一下修改这些代码时的情形：费尽九牛二虎之力，我终于找到该在哪改动代码，改了。作为一个有职业操守的程序员，我知道别的地方也需要类似的修改。于是，趁人不备，把刚做修改拷贝了一份，放到另外需要修改的地方。修改了几个变量，编译通过了。世界应该就此清净，至少问题解决了。

好吧！虽然这个程序员有职业操守的程序员，却缺少了些职业技能，至少在挥舞“拷贝粘贴”时，他没有嗅到散发出的臭味。

只要意识到坏味道，修改是件很容易的事，提出一个新函数即可：
void AddNode(List& RetList, int SignRunToInsert, int Pos, Node&Node) {
    if ( 1 == SignRunToInsert)    {
      RetList->Insert(Pos, Node);
    } else {
      RetList->Add(Node);
    }
}

于是，原来那三段代码变成了三个调用：
AddNode(RetList, SignRunToInsert, i, NewCatalog);
AddNode(RetList, SignRunToInsert, m, NewCatalog);
AddNode(RetList, SignRunToInsert, j, NewPrivNode);

当然，这种修改只是一个局部的微调，如果有更多的上下文信息，我们可以做得更好。

重复，是最为常见的坏味道。上面这种重复实际上是非常容易发现的，也是很容易修改。但所有这一切的前提是，发现坏味道。

长时间生活在这种代码里面，我们会对坏味道失去嗅觉。更可怕的是，一个初来乍到的嗅觉尚灵敏的人意识到这个问题，那些失去嗅觉的人却告诫他，别乱动，这挺好。

趁嗅觉尚在，请坚持代码正义。

代码之丑（五）

原文出处：http://dreamhead.blogbus.com/logs/83450989.html

不知道为什么，初见它时，我想起了郭芙蓉的排山倒海：
ColdRule *newRule = new ColdRule();
newRule->SetOID(oldRule->GetOID());
newRule->SetRegion(oldRule->GetRegion());
newRule->SetRebateRuleID(oldRule->GetRebateRuleID());
newRule->SetBeginCycle(oldRule->GetBeginCycle() + 1);
newRule->SetEndCycle(oldRule->GetEndCycle());
newRule->SetMainAcctAmount(oldRule->GetMainAcctAmount());
newRule->SetGiftAcctAmount(oldRule->GetGiftAcctAmount());
newRule->SetValidDays(0);
newRule->SetGiftAcct(oldRule->GetGiftAcct());
rules->Add(newRule);

就在我以为这一片代码就是完成给一个变量设值的时候，突然，在那个不起眼的角落里，这个变量得到了应用：它被加到了rules里面。什么叫峰回路转，这就是。

既然它给了我们这么有趣的体验，必然先杀后快。下面重构了这个函数：
ColdRule* CreateNewRule(ColdRule& oldRule) {
    ColdRule *newRule = new ColdRule();
    newRule->SetOID(oldRule.GetOID());
    newRule->SetRegion(oldRule.GetRegion());
    newRule->SetRebateRuleID(oldRule.GetRebateRuleID());
    newRule->SetBeginCycle(oldRule.GetBeginCycle() + 1);
    newRule->SetEndCycle(oldRule.GetEndCycle());
    newRule->SetMainAcctAmount(oldRule.GetMainAcctAmount());
    newRule->SetGiftAcctAmount(oldRule.GetGiftAcctAmount());
    newRule->SetValidDays(0);
    newRule->SetGiftAcct(oldRule.GetGiftAcct());
    return newRule
}

rules->Add(CreateNewRule(*oldRule));

把这一堆设值操作提取了出来，整个函数看上去一下子就清爽了。不是因为代码变少了，而是因为代码按照它职责进行了划分：创建的归创建，加入的归加入。之前的代码之所以让我不安，多重职责是罪魁祸首。

谈论干净代码时，我们总会说，函数应该只做一件事。函数做的事越多，就会越冗长。也就越难发现不同函数内存在的相似之处。为了一个问题，要在不同的地方改来改去也就难以避免了。

即便大家都认同了函数应该只做一件事，但多大的一件事算是一件事呢！不同的人心里有不同的标准。有人甚至认为一个功能就是一件事。于是，代码会越来越刺激。

想写干净代码，就别怕事小。哪怕一个函数只有一行，只要它能完整的表达一件事。在干净代码的世界里，大心脏是不受喜欢的。

接下来，我需要用历经沧桑的口吻告诉你，这么跌宕起伏的代码也只不过是一个更大函数的一个部分。此刻，浮现在我脑海里的是层峦叠嶂的山峰。

说明：以上系列文章均转载自“梦想风暴”博客，所有权利归原文作者所有。