BT

如何利用碎片时间提升技术认知与能力? 点击获取答案

专栏:代码之丑(一)——让判断条件做真正的选择

| 作者 郑晔 关注 2 他的粉丝 发布于 2010年11月18日. 估计阅读时间: 4 分钟 | CNUTCon 了解国内外一线大厂50+智能运维最新实践案例。

诸位看官,上代码:

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执行块里面的内容相差无几,需要我们人工比字符寻找差异,恭喜你,你找到它了。

作者简介:

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

查看原文:代码之丑(一)

【编者按】:这是InfoQ中文站新推出的专栏栏目。专栏旨在邀请国内一线的技术领域专家,定期撰写发表连载专栏,从深度和质量上为国内的技术从业者提供专业性、持续性的知识分享。这个《代码之丑》专栏计划推出系列共十篇,敬请期待后续精彩内容。也欢迎向InfoQ中文站推荐专栏作者及内容

评价本文

专业度
风格

您好,朋友!

您需要 注册一个InfoQ账号 或者 才能进行评论。在您完成注册后还需要进行一些设置。

获得来自InfoQ的更多体验。

告诉我们您的想法

允许的HTML标签: a,b,br,blockquote,i,li,pre,u,ul,p

当有人回复此评论时请E-mail通知我
社区评论

这篇文章所提代码确实需要优化一下。 by jianming guojm

如题,确实需要进一步精练。

Re: 这篇文章所提代码确实需要优化一下。 by 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了....

If-else 更符合人类的思考习惯 by Liu Tiger

If-else 更符合人类的思考习惯, 举例的代码确实可以简化,不过不能算丑陋吧,我还见过这样写的:

for(;;)
{
if(...) break;
if(...) break;
}

应该不算丑,有点啰嗦而已 by Jinian Xie

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

Re: 应该不算丑,有点啰嗦而已 by Java 陈

这种代码还有看不懂的

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

Re: 应该不算丑,有点啰嗦而已 by Yetao Chen

你说的没错。

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

补充 by Shichao Liu

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

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

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

编程也是一门艺术 by 陈 冬梁

代码是丑是美,看我们是把它看成一门艺术,还是吃饭的工具!

编译器是什么的? by M. Jwo

十几年前我也作这种事情,我的团队现在可以到上百人我反而觉得不该做了。
但是现在有种东西叫编译器的(十几年前也有),会帮你把那些事情做好。
你可以反组译两个代码,如果都一样没有必要写成易读性差的,
这两个例子都是,新的写法易读性我看来更差,更别说开篇的那段也是。
现在流行的是把程式写得很容易给一般人看懂。
当然,如果你的工作没有一个好得编译器的状况下除外。
真的有差别的举例如下,要求1+2+3+4+5+6....n,或是复杂点1-2+3-4+5....n
这样的题目如果用for-loop等来计算和(1+n)*n/2速度差很多,
这样的代码编译器可能不会帮你最佳化,才需要挑出来,但是仅限于Embedded System,
因为for-loop的可读性较佳,一眼可以看出需求的长相,在不是TDD的环境下用for-loop更好,
在Embedded System中因为速度与代码长度需要很计较,所以才需要如此优化。
但是无论如何,你的例子的透过编译后的机器码都是一样的,代表你是作虚功,白领钱没作任何有意义的事情。

一钱不值 by 毕 成栋

个人觉得应该有丰富的实践经验,才可以去教诲别人如何去做。这才比较复合中国人的做事习惯。作为公司的领头人物,有点知识,更不应该浮躁,到处卖弄,夸夸其谈。
文章根本没提到if else的真正问题,给出的例子也不好。

是优化还是总结? by 李 炎

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

以上都是个人愚见

Re: 一钱不值 by 曹 云飞

期待您给出好的例子,提出if else的真正问题,这样更有建设性。

Re: 编译器是什么的? by 朱 敏

呵呵,不错!
有的时候,还是简单点好吧 :)

Re: 应该不算丑,有点啰嗦而已 by Jinian Xie

维护的问题在于重复,而不在于是否用if else,那是习惯问题。

Re: 一钱不值 by 霍 泰稳

这不应算是“教诲”吧,这是真正的分享。Elessar可以谈一谈自己认为的if else的真正问题,包括给出一些例子,和读者分享一下,这才更有建设性,谢谢。

好文章 by 施 建雄

好文章,转到我的博客了

www.ackarlix.com/

Re: 一钱不值 by 毕 成栋

Re: 一钱不值 by 毕 成栋

具体参阅:www.antiifcampaign.com/

Re: 一钱不值 by Yetao Chen

个人觉得应该有丰富的实践经验,才可以去教诲别人如何去做。这才比较复合中国人的做事习惯。作为公司的领头人物,有点知识,更不应该浮躁,到处卖弄,夸夸其谈。
文章根本没提到if else的真正问题,给出的例子也不好。

===========================

相比这篇文章本身,我实在看不出来您的这条评论能否“值得了一钱”。

何况,在您说这句话的同时,您不也是在随意教训别人么。

您说这边文章没有提到if else的真正问题,却没有给出详细的解读,这不也恰恰是在夸夸其谈么。

Re: 一钱不值 by 毕 成栋

我已经回复了,请看www.antiifcampaign.com/

代码冗余并不是if else造成的。把代码冗余和if混为一谈,本身就很奇怪。有哗众取宠之嫌。
另外,例子中的条件表达式也早就被遗弃了,真不是个好例子。
联盟中的好例子有很多了,请详细阅读。

ps.楼上是马甲吗?
个人觉得应该有丰富的实践经验,才可以去教诲别人如何去做。这才比较复合中国人的做事习惯。作为公司的领头人物,有点知识,更不应该浮躁,到处卖弄,夸夸其谈。
文章根本没提到if else的真正问题,给出的例子也不好。

===========================

相比这篇文章本身,我实在看不出来您的这条评论能否“值得了一钱”。

何况,在您说这句话的同时,您不也是在随意教训别人么。

您说这边文章没有提到if else的真正问题,却没有给出详细的解读,这不也恰恰是在夸夸其谈么。

Re: 一钱不值 by Yetao Chen

我已经回复了,请看www.antiifcampaign.com/

代码冗余并不是if else造成的。把代码冗余和if混为一谈,本身就很奇怪。有哗众取宠之嫌。
另外,例子中的条件表达式也早就被遗弃了,真不是个好例子。
联盟中的好例子有很多了,请详细阅读。

ps.楼上是马甲吗?


一个链接又能对一个open discussion提供多少贡献呢?

我想说的是,对于infoq这么一个纯净的技术讨论空间,我们完全可以形成非常好的社区讨论氛围。

好了,话不多说,多说无益。

ps.我不是马甲。

Re: 一钱不值 by 毕 成栋

给一个链接我是不想拷贝别人的东西,谢谢!

Re: 一钱不值 by Yetao Chen

不客气。

最起码,应该有简单地阐述和说明,最后如果需要有引用的话,可以写“更多的内容可以参考xxx链接“,在你的具有简单的说明的前提下,提供引用链接是不会被认为在拷贝别人的东西,相反,还会被认为是具有建设性的讨论。

去看看一些论坛上面比较正规的讨论,比如msdn forum, stackoverflow。有很多更好的方式可以学习。

作者不在团队吧 by 拾 叁

团队更需要的是逻辑清晰,而且易于扩展,当复杂了,你还是要把这一行的代码改成if/else
代码的丑和美,我觉得并不是执着于这些奇淫巧计。而且在整体上保持高度可读和易于扩展。
不要再写这些让人抓狂,自己觉得很牛鼻的代码。

Re: 应该不算丑,有点啰嗦而已 by 尧 思齐

维护代价很高的代码就一直留了下来。这是我过IT业的普遍现象。

Re: 一钱不值 by Peng Shawn

我对Elessar Aragorn的回复同样感到疑惑,我想说你真得没有看懂文章想说什么。文章其实没有针对if/else做任何评价,事实上在示例中作者并没有对if/else作任何处理,作者真正要强调的是调用SendMsg方法的时机,你得理解文章的主题。
在这段示例代码中,真正应该被if/else决定的是方法的第二个参数,而不是整个逻辑分支。当我们把这段程序中的if/else的职责真正弄清楚之后,我们可以把msg的赋值逻辑提取出来,然后把方法调用提到外面。

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

这段代码的第一行将可继续重构成为一个方法String resolveResult(int retCode);
最后这段代码将成为:
String msg=resolveResult(retCode);
sendMsg("000",msg, outResult);
这样已经很简洁了,但有人还会继续重构,消除变量
将变量msg inline之后,将变成:
sendMsg("000", resolveResult(retCode), outResult);

to Elessar Aragorn
当你打算回复时请尊重他人,不要把任何人当成是谁谁的马甲,谁都不会且不需要以假面示人。

Re: 一钱不值 by 毕 成栋

回答你的问题一:请您看文章的标题“代码之丑(一)——让判断条件做真正的选择”

回答你的问题二:我已经做出回复了,已经有人对if else做出了很充分的评价。并有专门的门户,建立了联盟。
至于为什么说别人是不是某人的马甲,我原先以为我的理由是很充分的。为什么会有人指责我的评价“一钱不值”。最后我明白是由于我没有贴上原文。并注明出处。

不知道是否能解答您的疑惑?

Re: 一钱不值 by 邹 超群

我支持你,这个例子确实很一般

Re: 一钱不值 by 毕 成栋

谢谢

俺更喜欢改成这样儿,欢迎大家拍砖~~~ by 高 翌翔

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

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

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: yixianggao@126.com

if..else的问题在哪? by Lu Luke

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

Re: if..else的问题在哪? by Lu Luke

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

我们不是吵的,而是追求的 by 孙 留成

我认为楼主的思想很好,第一点,楼主是通过if else 来引出编程艺术的思想;第二,不会因为简单,就认为没有必要与大家共享。感谢楼上,让我和大家一起分享看文章的5分钟。

Re: 我们不是吵的,而是追求的 by Shi Eric

蛮好的 关注

这样的怎么优化 by 丁 赛

if(user5.getUserIdEmail()!=null&&!user5.getUserIdEmail().equals("")){
if(!db.getOtherid(user5)){
if(db.insertOther(user5)){
if(!db.getOtherGuanXi(user5)){
if(!db.insertGuanXi(user5)){ test=false;}
}
}else{
test=false;
}
} else{
if(db.getOtherGuanXi(user5)){
if(!db.updateOther(user5)){ test=false; }
}else{
if(!db.insertGuanXi(user5)){ test=false;}
}
}
}
帮我优化一下

判断时候有用户(无),插入用户,插入关系
判断时候有用户(有),判断是否有关系 ,插入关系或者修改关系和用户

面向对象的改法 by HUANG liang

ProcessMessage msg = new ProcessMessage(retCode);
msg.Send();
msg.GetResult(outResult);

最后两句也可能合一:
msg.Send(outResult);

Re: 一钱不值 by Wong Peter

觉得把resolveResult改成queryResultMessage更清楚些

允许的HTML标签: a,b,br,blockquote,i,li,pre,u,ul,p

当有人回复此评论时请E-mail通知我

允许的HTML标签: a,b,br,blockquote,i,li,pre,u,ul,p

当有人回复此评论时请E-mail通知我

37 讨论

登陆InfoQ,与你最关心的话题互动。


找回密码....

Follow

关注你最喜爱的话题和作者

快速浏览网站内你所感兴趣话题的精选内容。

Like

内容自由定制

选择想要阅读的主题和喜爱的作者定制自己的新闻源。

Notifications

获取更新

设置通知机制以获取内容更新对您而言是否重要

BT