本文共 9710 字,大约阅读时间需要 32 分钟。
在infoQ上面看见这系列文章的第一部分,给我很大的触动,以及将它转载的冲动。突然觉得原来写代码也可以这么讲究,“写代码”其实其中应该包括“设计”成分。虽然代码的可读性以及代码的运行效率有时候是存在矛盾的,同时,过于“干净”的代码并不见得就有很好的可读性,但是过于“直白”,不加任何修饰和处理的代码,也是让人绝望的。长久的“直白”代码会消磨程序员写程序的兴致和憧憬。
以下转载这个系列的文章。转载时请以超链接形式标明文章原始出处和作者信息及
代码之丑(零)
看到下面这段代码,你会做何感想?
if(db.Next()) { return true; } else { return false; } 有的人会想,怎么写得这么笨啊!但是,请放心,绝对会有人这么想,挺好的,实现功能了。这并非我臆造出的代码,而是从一个真实的codebase上找到。 成为一个咨询师之后,我有机会在不同的项目中穿梭。同客户合作的过程中,我经常干的一件事是:code diff。也就是用源码管理工具的diff功能把当天全部修改拿出来,从编码的角度来分析代码写得怎么样。 因为这个工作,我看到了许多不同人编写的代码,我的编码底线不断受到挑战。许多东西,我以为是常识,但实际上不为许多人所知,比如上面那段代码。 我把在看到的问题总结成一个session,与客户的程序员分享。结束之后,有人建议,为什么不能把这些问题写下来,与更多人分享。于是,我产生了写下《代码之丑》念头,以此向《 》致敬。最后要说的是,开始那段代码可以写成这样:
return db.Next();
代码之丑(一)
诸位看官,上代码: if (0 == iRetCode) { this->SendPeerMsg("000", "Process Success", outRSet); } else { this->SendPeerMsg("000", "Process Failure", 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两个执行块里面的内容相差无几,需要我们人工比字符寻找差异,恭喜你,你找到它了。
代码之丑(二)
这是一个长长的判断条件:
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”定义为“多”,也就是如果有两个条件的组合,可以接受,如果是三个,还是改吧!
虽然通过不断调整,这段代码已经不同于之前,但它依然不是我们心目中的理想代码。出现这种代码,往往意味背后有更严重的设计问题。不过,它并不是这里讨论的内容,这里的讨论就到此为止吧!
代码之丑(二)(续)
本文已经首发于 ,版权所有,原文为《 》,如需转载,请务必附带本声明,谢谢。
是一个面向中高端技术人员的在线独立社区,为Java、.NET、Ruby、SOA、敏捷、架构等领域提供及时而有深度的资讯、高端技术大会如 、免费迷你书下载如《 》等。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); 虽然应用代码变得简单了,但写出框架的结构是需要一定基础的。它不像应用代码那样来得平铺直叙,但其实也没那么难,只不过很多人从没有考虑把代码写成这样。只要换个角度去思考,多多练习,也就可以驾轻就熟了。
代码之丑(三)
又见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出现在眼前,请提高警惕,那里多半有坑。
代码之丑(四)
这是一个找茬的游戏,下面三段代码的差别在哪:
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); 当然,这种修改只是一个局部的微调,如果有更多的上下文信息,我们可以做得更好。 重复,是最为常见的坏味道。上面这种重复实际上是非常容易发现的,也是很容易修改。但所有这一切的前提是,发现坏味道。 长时间生活在这种代码里面,我们会对坏味道失去嗅觉。更可怕的是,一个初来乍到的嗅觉尚灵敏的人意识到这个问题,那些失去嗅觉的人却告诫他,别乱动,这挺好。 趁嗅觉尚在,请坚持代码正义。
代码之丑(五)
不知道为什么,初见它时,我想起了郭芙蓉的排山倒海:
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)); 把这一堆设值操作提取了出来,整个函数看上去一下子就清爽了。不是因为代码变少了,而是因为代码按照它职责进行了划分:创建的归创建,加入的归加入。之前的代码之所以让我不安,多重职责是罪魁祸首。 谈论干净代码时,我们总会说,函数应该只做一件事。函数做的事越多,就会越冗长。也就越难发现不同函数内存在的相似之处。为了一个问题,要在不同的地方改来改去也就难以避免了。 即便大家都认同了函数应该只做一件事,但多大的一件事算是一件事呢!不同的人心里有不同的标准。有人甚至认为一个功能就是一件事。于是,代码会越来越刺激。想写干净代码,就别怕事小。哪怕一个函数只有一行,只要它能完整的表达一件事。在干净代码的世界里,大心脏是不受喜欢的。
接下来,我需要用历经沧桑的口吻告诉你,这么跌宕起伏的代码也只不过是一个更大函数的一个部分。此刻,浮现在我脑海里的是层峦叠嶂的山峰。