• 2010-10-24

    代码之丑(一)

    版权声明:转载时请以超链接形式标明文章原始出处和作者信息及本声明
    http://www.blogbus.com/dreamhead-logs/80612297.html

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


    本文已经首发于InfoQ中文站 ,版权所有,原文为《专栏:代码之丑(一) 》,如需转载,请务必附带本声明,谢谢。

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

    分享到:
    引用地址:

    评论

  • 关于代码之丑中的一,是否可以详细说说优化后的好处?可以让偶们了结得更透彻。

    我的理解:
    后面的代码遵循了单一职责原则。每一句话的职责都是独立的,逻辑判断就是判断,发消息就是发消息。因而可扩展性和维护性变好了。
    其它还有什么好处?比如说编译上的,运行速度上的。
  • 评论有点本末倒置
  • (它说我内容过长,我分段了。)

    (……)

    对于这么一个例子显得有点蛋疼,但是真有人复制几十行一样的进去的,对它们进行同样的改造就大不一样了。
  • (它说我内容过长,我分段了。)

    (……)

    [DEFINE msg]
    if (0 == iRetCode) {
    [ASSIGN "Process Success" -> msg]
    } else {
    [ASSIGN "Process Failure" -> msg]
    }
    this->SendPeerMsg("000", msg, outRSet);

    (……)
  • (它说我内容过长,我分段了。)

    文首的例子的根本问题在于把本与判断条件无关的代码复制了两份放进了条件语句的两个分支中,造成了代码重复,自然会造成所有代码重复会造成的问题。

    truncatei 所言也很有道理,选择表达式也不是处处受欢迎。
    所以一个更好的段落或许是([] 中的为伪代码):

    (……)
  • 一般这种问题除了特别简单的我倾向于把它写成Class里的一组函数,然后在运行时交给多态解决.
  • 老板,很喜欢你分享的经验,谢谢!
  • 也不能这么绝对的,很多公司会限制使用条件表达式,而强制使用if else.

    CheckStyle默认规则里就是不允许使用条件表达式的.
    回复2sin18说:
    我这里只是为了节省篇幅,用了条件表达式。这里的关注点在于判断的到底是为什么。
    2010-10-26 14:36:45