• 2010-11-12

    代码之丑(五)

    版权声明:转载时请以超链接形式标明文章原始出处和作者信息及本声明
    http://www.blogbus.com/dreamhead-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));

    把这一堆设值操作提取了出来,整个函数看上去一下子就清爽了。不是因为代码变少了,而是因为代码按照它职责进行了划分:创建的归创建,加入的归加入。之前的代码之所以让我不安,多重职责是罪魁祸首。

    谈论干净代码时,我们总会说,函数应该只做一件事。函数做的事越多,就会越冗长。也就越难发现不同函数内存在的相似之处。为了一个问题,要在不同的地方改来改去也就难以避免了。

    即便大家都认同了函数应该只做一件事,但多大的一件事算是一件事呢!不同的人心里有不同的标准。有人甚至认为一个功能就是一件事。于是,代码会越来越刺激。

    想写干净代码,就别怕事小。哪怕一个函数只有一行,只要它能完整的表达一件事。在干净代码的世界里,大心脏是不受喜欢的。

    接下来,我需要用历经沧桑的口吻告诉你,这么跌宕起伏的代码也只不过是一个更大函数的一个部分。此刻,浮现在我脑海里的是层峦叠嶂的山峰。


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

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

    分享到:
    引用地址:

    评论

  • 感觉使用clone模式比较好吧,而且容易理解。
    直接写复制构造函数可能会影响比较大吧。
    比如,还要有赋值函数等。。。

    个人见解,不足见笑。呵呵。
  • newRule->SetBeginCycle(oldRule.GetBeginCycle() + 1);
    newRule->SetEndCycle(oldRule.GetEndCycle());

    如果用一个SetCycle(begin,end)的方法会更适合人类阅读
  • 我的话可能会这么做:
    1、实现ColdRule的复制构造函数(不喜欢用拷贝这个音译词,见谅);
    2、检查各种场合下这些set是不是都是配套出现的,是的话——统统干掉;
    3、用shared_ptr接住new出来的对象
  • 这样的处理方式就是重构中的提到的 Extract Method 操作吧.
  • newRule->SetEndCycle(RebateSchemaRul->GetEndCycle());
    郭芙蓉里面的这行代码中的RebateSchemaRul是咋处理的,如果RebateSchemaRul是另外一个ColdRule的话,是不是也得传递进去?
    回复roody说:
    嗯,这里是我处理的疏漏,多谢提醒。
    2010-11-15 22:01:05
  • 弱弱的问一下,CreateNewRule()函数 是不是属于ColdRule类的?
    回复jackaniu说:
    实际上,是的。这里只是为了演示基本的重构思路,把CreateNewRule写在了这里。
    2010-11-12 17:59:15