代码之丑学习转载与总结

代码之丑（二）Tag：代码之丑 （原文链接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列表变成声明式，进一步提高代码的可读性。
      发现这种代码很容易，只要看到在长长的判断条件，就是它了。要限制这种代码的存在，我们只要以设定一个简单的规则：
 1.判断条件里面不允许多个条件的组合
 2.在实际的应用中，我们会把“3”定义为“多”，也就是如果有两个条件的组合，可以接受，如果是三个，还是改吧！
      虽然通过不断调整，这段代码已经不同于之前，但它依然不是我们心目中的理想代码。出现这种代码，往往意味背后有更严重的设计问题。不过，它并不是这里讨论的内容，这里的讨论就到此为止吧

      总结：选择判断条件不能有太多的子项组合在一起，超过三个的话应该考虑上面提到的方法。其实上面不要定义成数组，放在一个专门的配置下面定义一个枚举似乎更合理些。

 

代码之丑（四）Tag：代码之丑 （原文链接： 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);
 

  

      当然，这种修改只是一个局部的微调，如果有更多的上下文信息，我们可以做得更好。 重复，是最为常见的坏味道。上面这种重复实际上是非常容易发现的，也是很容易修改。但所有这一切的前提是，发现坏味道。长时间生活在这种代码里面，我们会对坏味道失去嗅觉。更可怕的是，一个初来乍到的嗅觉尚灵敏的人意识到这个问题，那些失去嗅觉的人却告诫他，别乱动，这挺好。

代码之丑（五）Tag：代码之丑 （原文链接：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));
 

      把这一堆设值操作提取了出来，整个函数看上去一下子就清爽了。不是因为代码变少了，而是因为代码按照它职责进行了划分：创建的归创建，加入的归加入。之前的代码之所以让我不安，多重职责是罪魁祸首。
      谈论干净代码时，我们总会说，函数应该只做一件事。函数做的事越多，就会越冗长。也就越难发现不同函数内存在的相似之处。为了一个问题，要在不同的地方改来改去也就难以避免了。
      即便大家都认同了函数应该只做一件事，但多大的一件事算是一件事呢！不同的人心里有不同的标准。有人甚至认为一个功能就是一件事。于是，代码会越来越刺激。
 想写干净代码，就别怕事小。哪怕一个函数只有一行，只要它能完整的表达一件事。在干净代码的世界里，大心脏是不受喜欢的。
      总结：这两这两篇其实都是在阐明一个道理，就是函数与职责的关系。一个函数应该只是履行一个职责，当一个函数要执行某项职责的时候需要其他某些子功能的实现的帮助。就这样，我们往往会直接在这个函数里面来实现这个子功能。这样其实这个函数就是实现了多个功能的实现。应该把这个子功能实现的代码写在一个子函数里面，当然这个还是要视具体情况而定的，因为参数有可能会影响。