代码之丑.docx

代码之丑.docx

《代码之丑.docx》由会员分享，可在线阅读，更多相关《代码之丑.docx（16页珍藏版）》请在冰豆网上搜索。

代码之丑

专栏：

代码之丑——开篇

我是一个程序员，也是一个咨询师。

成为咨询师之后，我有机会在不同的项目中穿梭。

同客户合作的过程中，我经常干的一件事是：

codediff，也就是用源码管理工具的diff功能把当天全部修改拿出来，从编码的角度来分析代码写得怎么样。

因为这个工作，我看到了许多不同人编写的代码，我的编码底线不断受到挑战。

许多东西，我以为是常识，但实际上不为许多人所知。

比如，下面这段代码，你会做何感想？

if（db.Next（））{

returntrue;

}else{

returnfalse;

}

有的人会想，怎么写得这么笨啊！

但是，请放心，绝对会有人这么想：

挺好的，实现功能了。

这并非我臆造出的代码，而是源自一个真实的codebase。

这些代码的存在，给了我很多机会与人分享一些编码的心得。

其间，有人建议，为什么不能把你说的这些内容写下来，与更多人分享。

于是，有了这个即将看到的系列：

《代码之丑》，以此向《代码之美》致敬。

最后要说的是，上面那段代码可以写成这样：

returndb.Next（）;

专栏：

代码之丑

（一）——让判断条件做真正的选择

诸位看官，上代码：

if（0==retCode）{

SendMsg（"000","ProcessSuccess",outResult）;

}else{

SendMsg（"000","ProcessFailure",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的执行语句真正的差异只在于一个参数。

第一段代码，二者的差异只是发送的消息，第二段代码，差异在于最后那个参数。

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

constchar*msg=（0==retCode?

"ProcessSuccess":

"ProcessFailure"）;

SendMsg（"000",msg,outResult）;

为了节省篇幅，我选择了条件表达式。

我知道，很多人不是那么喜欢它。

如果if/else依旧是你的大爱，勇敢追求去吧！

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

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

对于前面调整的代码，判断条件真正判断的内容是消息的内容，而不是消息发送的过程。

经过我们的调整，获取消息内容和发送消息的过程严格分离开来。

消除了代码中的冗余，代码也更容易理解，同时，给未来留出了可扩展性。

如果将来retCode还有更多的情形，我们只要调整消息获取的部分进行调整就好了。

当然，封装成函数是一个更好的选择，这样代码就变成了：

SendMsg（"000",msgFromRetCode（retCode）,outResult）;

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

这样丑陋的代码是如何从众多代码中脱颖而出的呢？

很简单，只要看到，if/else执行块里面的内容相差无几，需要我们人工比字符寻找差异，恭喜你，你找到它了。

专栏：

代码之丑

（二）——长长的条件

这是一个长长的判断条件：

if（strcmp（type,“DropGroup"）==0

||strcmp（type,"CancelUserGroup"）==0

||strcmp（type,"QFUserGroup"）==0

||strcmp（type,"CancelQFUserGroup"）==0

||strcmp（type,"QZUserGroup"）==0

||strcmp（type,"CancelQZUserGroup"）==0

||strcmp（type,"SQUserGroup"）==0

||strcmp（type,"CancelSQUserGroup"）==0

||strcmp（type,“UseGroup"）==0

||strcmp（type,"CancelGroup"）==0）

之所以注意到它，因为最后两个条件是在最新修改里面加入的，换句话说，这不是一次写就的代码。

单就这一次而言，只改了两行，这是可以接受的。

但这是遗留代码，每次可能只改了一两行，通常我们会不只一次踏入这片土地。

经年累月，代码成了这个样子。

就我接触过的代码而言，这并不是最长的判断条件。

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

其实，如果这个判断条件是这个函数里仅有的东西，我也是可以接受的。

遗憾的是，大多数情况下，这只不过是一个更大函数中的一小段而已。

为了让这段代码可以接受一些，我们不妨稍做封装：

boolshouldExecute（constchar*type）{

return（strcmp（type,“DropGroup"）==0

||strcmp（type,"CancelUserGroup"）==0

||strcmp（type,"QFUserGroup"）==0

||strcmp（type,"CancelQFUserGroup"）==0

||strcmp（type,"QZUserGroup"）==0

||strcmp（type,"CancelQZUserGroup"）==0

||strcmp（type,"SQUserGroup"）==0

||strcmp（type,"CancelSQUserGroup"）==0

||strcmp（type,“UseGroup"）==0

||strcmp（type,"CancelGroup"）==0）;

}

if（shouldExecute（type））{

...

}

现在，虽然条件依然还是很多，但比起原来庞大的函数，至少它已经被控制在一个相对较小的函数里了。

更重要的是，通过函数名，我们终于有机会告诉世人这段代码判断的是什么了。

虽然提取函数把这段代码混乱的条件分离开来，它还是可以继续改进的。

比如，我们把判断的条件进一步提取：

boolshouldExecute（constchar*type）{

staticconstchar*execute_type[]={

"DropGroup",

"CancelUserGroup",

"QFUserGroup",

"CancelQFUserGroup",

"QZUserGroup",

"CancelQZUserGroup",

"SQUserGroup",

"CancelSQUserGroup",

"UseGroup",

"CancelGroup"

};

intsize=ARRAY_SIZE（execute_type）;

for（inti=0;i

if（strcmp（type,execute_type[i]）==0）{

returntrue;

}

}

returnfalse;

}

这样的话，如果以后要加一个新的type，只要在数组中增加一个新的元素即可。

这段代码还可以进一步封装，把这个type列表变成声明式，进一步提高代码的可读性。

简单的理解声明式的风格，就是描述做什么，而不是怎么做。

一个声明式编程的例子是Rails里面的数据关联，为人熟知的has_many和belongs_to。

通过这个声明，模型类就会具备一些数据关联的能力。

具体到实际开发里，声明式编程需要有两个部分：

一方面是一些基础的框架性代码，另一方面是应用层面如何使用。

通常，框架代码不像应用层面代码那么好理解，但有了这个基础，应用代码就会变得简单许多。

针对上面那段代码，按照这种风格，我改造了代码，下面是框架部分的代码：

#defineBEGIN_STR_PREDICATE（predicate_name）\

boolpredicate_name（constchar*field）{\

staticconstchar*predicate_true_fields[]={

#defineSTR_PREDICATE_ITEM（item）#item,

#defineEND_STR_PREDICATE\

};\

\

intsize=ARRAY_SIZE（predicate_true_fields）;\

for（inti=0;i

if（strcmp（field,predicate_true_fields[i]）==0）{\

returntrue;\

}\

}\

\

returnfalse;\

}

这里用到了C/C++常见的宏技巧，为的就是让应用层面的代码写起来更像声明。

稍微对比一下，就会发现，实际上二者几乎是一样的。

有了框架，就该应用了：

BEGIN_STR_PREDICATE（shouldExecute）

STR_PREDICATE_ITEM（DropGroup）

STR_PREDICATE_ITEM（CancelUserGroup）

STR_PREDICATE_ITEM（QFUserGroup）

STR_PREDICATE_ITEM（CancelQFUserGroup）

STR_PREDICATE_ITEM（QZUserGroup）

STR_PREDICATE_ITEM（CancelQZUserGroup）

STR_PREDICATE_ITEM（SQUserGroup）

STR_PREDICATE_ITEM（CancelSQUserGroup）

STR_PREDICATE_ITEM（UseGroup）

STR_PREDICATE_ITEM（CancelGroup）

END_STR_PREDICATE

shouldExecute就此重现出来了。

不过，这段代码已经不再像一个函数，而更像一段声明，而这，恰恰就是我们的目标。

有了这个基础，实现一个新的函数，不过是做一个新的声明而已。

使用这个新函数的方法依然如故：

if（shouldExecute（type））{

...

}

虽然应用代码变得简单了，但写出框架的结构是需要一定基础的。

它不像应用代码那样来得平铺直叙，但其实也没那么难，只不过很多人从没有考虑把代码写成这样。

只要换个角度去思考，多多练习，也就可以驾轻就熟了。

发现这种代码很容易，只要看到在长长的判断条件，就是它了。

要限制这种代码的存在，只要以设定一个简单的规则：

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

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

虽然通过不断调整，这段代码已经不同于之前，但它依然不是我们心目中的理想代码。

出现这种代码，往往意味背后有更严重的设计问题。

不过，它并不是这里讨论的内容，这里的讨论就到此为止吧！

代码之丑

（二）（续）

∙sinojelly在《代码之丑

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

好吧！

我承认，我偷懒了，为了省事，一笔带过了。

简单理解声明式的风格，就是把描述做什么，而不是怎么做。

一个声明式编程的例子是Rails里面的数据关联，为人熟知的has_many和belongs_to。

通过声明，模型类就会具备一些数据关联的能力。

具体到实际开发里，声明式编程需要有两个部分：

一方面是一些基础的框架性代码，另一方面是应用层面如何使用。

框架代码通常来说，都不像应用层面代码那么好理解，但有了这个基础，应用代码就会变得简单许多。

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

#defineBEGIN_STR_PREDICATE（predicate_name）\

boolpredicate_name（constchar*field）{\

 staticconstchar*predicate_true_fields[]={

#defineSTR_PREDICATE_ITEM（item）#item,

#defineEND_STR_PREDICATE\

 };\

 \

 intsize=ARRAY_SIZE（predicate_true_fields）;\

 for（inti=0;i

   if（strcmp（field,predicate_true_fields[i]）==0）{\

      returntrue;\

   }\

 }\

\

 returnfalse;\

}

这里用到了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）;

虽然应用代码变得简单了，但写出框架的结构是需要一定基础的。

它不像应用代码那样来得平铺直叙，但其实也没那么难，只不过很多人从没有考虑把代码写成这样。

只要换个角度去思考，多多练习，也就可以驾轻就熟了。

代码之丑

（二）（续二）

∙一个叫夏勇杰的朋友看了《代码之丑》

（二）》和《续》之后，给我写了封邮件，就原来的问题，给出了自己的解决方案，这里分享一下。

他的思路是把所有判断条件转换成数字，然后，利用常见的位操作的技巧来处理。

上代码：

 enumType{

   PreDropGroupSubs                 =1,

   StopUserGroupSubsCancel       =1<<1,

   QFStopUserGroupSubs            =1<<2,

   QFStopUserGroupSubsCancel   =1<<3,

   QZStopUserGroupSubs            =1<<4,

   QZStopUserGroupSubsCancel   =1<<5,

   SQStopUserGroupSubs           =1<<6,

   SQStopUserGroupSubsCancel   =1<<7,

   StopUseGroupSubs                 =1<<8,

   PreDropGroupSubsCancel         =1<<9,  

   //...

 };

 intcanExecute=PreDropGroupSubs|StopUserGroupSubsCancel

                  |QFStopUserGroupSubs|QFStopUserGroupSubsCancel

                  |QZStopUserGroupSubs|QZStopUserGroupSubsCancel

                       |SQStopUserGroupSubs|SQStopUserGroupSubsCancel

                       |StopUseGroupSubs|PreDropGroupSubsCancel;

 boolshouldExecute（Record&record）{

   return（（rec.type&canExecute）!

=0）;

 }

当然，这么做的一个前提是，需要把type的类型转换成数字。

这不是太大的问题，只要在整个处理开始的部分做一次转换就可以了。

从执行效率上来说，这段代码会比原来的方案高得多：

一方面在于字符串比较，另一方面在于原来是循环判断。

写《代码之丑》

（二）的时候，我没想着写《续》，因为那些内容不是在讨论“丑”，而纯粹是编程技巧了。

写《续》的时候，我更没想到会有《续二》。

这两个续篇都是看过前面内容的朋友驱动出来的，而这就是分享的乐趣，不是吗？

专栏：

代码之丑（三）——switch陷阱

又见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’:

thrownewIllegalArgument（）;

default:

}

出于多年编程养成的条件反射，我对于switch总会给予更多的关照。

在那本大名鼎鼎《重构》里，MartinFowler专门把switch语句单独拿出来作为一种坏味道来讨论。

研习面向对象编程之后，看见switch，我就会联想到多态，遗憾的是，这段代码和多态没什么关系。

仔细阅读这段代码，我找出了其中的规律，nextFirstChar就是firstChar的下一个字符。

于是，我改写了这段代码：

switch（firstChar）{

case‘N’:

case‘O’:

case‘P’:

case‘Q’:

case‘R’:

nextFirstChar=firstChar+1;

break;

case‘T’:

thrownewIllegalArgument（）;

default:

}

现在，至少看起来，这段代码已经比原来短了不少。

当然这么做基于一个前提，就是这些字母编码的顺序确确实实是连续的。

从理论上说，开始那段代码适用性更强。

但在实际开发中，我们碰到字母不连续编码的概率趋近于0。

但这段代码究竟是如何产生的呢？

我开始研读上下文，原来这段代码是用当前ID产生下一个ID的，比如当前是N0000，下一个就是N0001。

如果数字满了，就改变字母，比如当前ID是R9999，下一个就是T0000。

在这里，字母也就相当于一位数字，根据情况进行进位，所以有了这段代码。

代码上的注释告诉我，字母的序列只有从N到T，根据这个提示，我再次改写了这段代码：

if（firstChar>=‘N’&&firstChar<=‘S”）{

nextFirstChar=firstChar+1;

}else{

thrownewIllegalArgument（）;

}

这里统一处理了字母为T和default的情形，严格说来，这和原有代码并不完全等价。

但这是了解了需求后做出的决定，换句话说，原有代码在这里的处理中存在漏洞。

修改这段代码，只是运用了非常简单的编程技巧。

遗憾的是，即便如此简单的编程技巧，也不是所有开发人员都驾轻就熟的，很多人更习惯于“平铺直叙”。

这种直白造就了代码中的许多鸿篇巨制。

我听过不少“编程是体力活”的抱怨，不过，能把写程序干成体力活，也着实不值得同情。

写程序，不动脑子，不体力才怪。

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

专栏：

代码之丑（四）——代码找茬游戏

这是一个找茬的游戏，下面三段代码的差别在哪：

if（1==insertFlag）{

retList.insert（i,newCatalog）;

}else{

retList.add（newCatalog）;

}

if（1==insertFlag）{

retList.insert（m,newCatalog）;

}else{

retList.add（newCatalog）;

}

if（1==insertFlag）{

retList.insert（j,newPrivNode）;

}else{

retList.add（newPrivNode）;

}

答案时间：

除了用到变量之外，完全相同。

我想说的是，这是我从一个文件的一次diff中看到的。

不妨设想一下修改这些代码时的情形：

费尽九牛二虎之力，我终于找到该在哪改动代码，然后改了。

作为一个有职业操守的程序员，我知道别的地方也需要类似的修改。

于是，趁人不备，把刚做修改拷贝了一份，放到另外需要修改的地方。

修改了几个变量，编译通过了。

世界应该就此清净，至