Code Kata 之读自己过去的代码
临近过年的日子，是个回顾的好机会。
正好前一段看到这篇翻译介绍 读自己以前代码的Kata，拿来练习练习。
其实Thomas在提出Kata概念的时候，涵盖的范围是远大于编码层面的。只是由于其它类型的Kata的判定标准比较“虚”，所以不常拿来练习。
这个Kata的关注点在于刻意练习阅读代码，而且是自己写的代码，来体会代码中各种细节的长期影响。通过主动的回顾来形成良好习惯，并增强review能力。
我自诩是个“代码考古学家”，常常修一个bug把80%的时间用在了翻历史记录，来琢磨它为啥能写成这个样子。同时也是个热衷于review别人代码的人，不惮于对别人的代码说三道四，想必很惹人厌了。
这次算是找个机会把自己的代码摆出来，自食其果吧。
程序员读到自己的代码
代码的选择
Kata的要求是写了超过一年，记忆有些模糊。代码量在500至1000行。
我选择了一个小项目，小到基本由我一个人开发。而且除了对口的产品经理，也再没其他人关心了。所以当初可以随心所欲的搞。
然而即便这么小的项目，做完以后客户却要求与最初商定的需求文档几乎完全不一样的功能。最终，所有的业务对象推倒重新做了一遍。所以也可以算是个典型的软件项目吧。
基本上是用outside-in 式TDD开发，虽然当时还不是特别自觉的采用这种流程。
从中选择了一个类来做Review。选择时参考了《修改代码的艺术》作者关于技术债分析的建议：一个类修改越频繁，说明它越可能将来被改动，越可能存在技术债。
我统计了所有的提交记录，选出一个提交次数最多的类，名叫WorkflowManager。和标准工作流没什么关系，是一个实现简单工作步骤流转的类。
第一遍读：假定这是大牛写的代码，记录惊艳的地方
- 短。类只有200行，每个方法的长度都少于30行。
虽说是个简单的流程控制，用语言来描述还真不是一两句话能说清的。相比起来代码算是相当简洁了。 - 一致。代码格式，命名规范。常用写法都很一致。看起来很整齐。
- 可读。方法和变量都起了有意义的名字。
- 测试覆盖。类共有15个方法，对应的测试有19个用例。测试代码量是实现代码的两倍。没有专门用工具统计覆盖率，按照从其它项目得到的经验，应该在90%以上。
- 每个测试都使用了统一的
Given/When/Then格式。 - 每个测试验证一个概念。对于相同方法不同使用情境的情况，分别写对应的测试用例。
- 在测试中使用有意义的变量命名和assert message来说明测试的意图，比如：
//Given
User original = new User("assignee before operation");
workSet.assignee = original;
//When
manager.operate(workSet);
//Then
assertEquals("Not change assignee when operate work set", original, workSet.assignee);
第二遍读：假定这是个二货写的代码，记录烂的地方
其实在看第一遍的时候就开始忍不住想这些地方应该写的更好了。我果然是不善于发现优点，专爱挑毛病。
- 单一职责。
- 这个类叫做
WorkflowManager, xxxManager这个名字暗示了职责不清。 - 果然，通过Clean Code中学到的办法，统计每个方法与成员变量间的关系。发现每个成员变量都只被很少几个方法使用。说明这个类缺乏内聚性，是多个职责的杂烩。
- 在测试里也有体现，某几个用例关注于某个方法。它们依赖共有的前置条件，验证相似的结果。看起来就像是Test类里内嵌了一个小Test类。
- 这个类叫做
- 未使用的参数。说明当初是觉得“将来会用到”加上的这个参数，而不是由测试驱动产生的。
- 注释。实现和测试代码中都有说明意图的注释。应该重构代码使注释没有必要出现。
- 系统时间。实现代码里有最近更新日期的时间戳字段。测试中在操作完毕后，直接用当前系统日期与之比较。这样做有两个问题。一是依赖环境，另一个问题是如果测试执行和验证的两个点恰好跨0点就会失败。
- 返回
null。由于依赖的对象有可能返回null， 这段代码里相当一部分在处理这种情况，使得主线逻辑有些模糊。可悲的是，之后它又会返回null值给外面…… - 命名。有些方法名用的是类似事件的名称，比如
stateChanged，而不是一个动作。 - 异常处理。代码里有一部分逻辑是处理配置信息异常情况的，这种情况下程序无法正常完成操作。然而处理的不够统一，有些地方悄悄的返回
null值，有些记录了日志，但是没有明确指出这是某项配置的问题，以及这个问题可能造成的后果。 - 测试中assert message很难说它是描述了应该的行为，还是失败时候的错误。比如：
assertNull("no start date after approval", workSet.startDate);
到底是应该有这个日期还是没有呢， - 有些message在程序行为变化后没有随之修改。
- 每个test几乎都有重复的mock权限相关数据的代码，模糊了当前测试的焦点
上面这些严格来说，应该算是可以改进的地方，还称不上是二。不过下面这段就……
public List<Item> setSelectedItems(List<Item> items, WorkSet workSet) {
List<Item> remainItems = new ArrayList<Item>(items);
nextInputItem:
for (Item inputItem : items) {
for (Item item : workSet.items) {
if (item.key.equals(inputItem.key)) {
item.selected = inputItem.selected;
remainItems.remove(inputItem);
continue nextInputItem;
}
}
if (!inputItem.selected) {
remainItems.remove(inputItem);
}
}
return remainItems;
}
两层for嵌套，不但嵌套for，里面还套if，不但if，还continue，不但continue，还从内层continue到外层……
感觉差不多把今天我认为不好的风格全拿出来演练了一遍。瞬间有点“这是我写出来的么？”的感觉。不过稍稍回忆一下，就想起来我当时还去专门查了查continue到外层的写法。
你能看出来这是在干嘛么？
其实是数据库里的workset保存有若干个项目，每个项目有选中状态。每次页面提交时候会返回这些项目通过页面操作后的选中状态。需要更新到数据库中。
而且页面还有可能增加原来数据库里没有项目，这些项目要在数据库新建项目记录，并且和workset关联起来。
这个双层循环做了两件事
- 对于已经有的项目，按照页面传入的列表更新选中状态
- 对于没有的项目，放在另一个列表中作为返回值，方便给下一步做创建项目等操作。
如果让你来重构它，你会怎么做呢？
第三遍读：假定这段代码有个严重Bug，今天找不出来你就完了。记录找到的Bug
虽然没有专人QC，也完全没有测试阶段，但是因为由着我性子写了一堆单元测试和集成测试。每次本地保存代码的时候都会执行一遍。因而我对项目的质量是相当自信的。
最直观的感受是在客户验收演示会上。
当初开发完成后扔在一边，拖了半年多客户才验收。已经记不清具体代码细节了。客户在操作演示过程中，走到一步走不下去了。当时我心情毫无波动，一点也没打开源代码检查的冲动。
果然，稍稍回忆了一下就发现不是bug，而是操作失误。
所以一开始我认为是没法完成这一轮的目标的。准备能找到个错误处理或者很特殊情况下的缺陷就交差吧。
而且读代码找bug真的好难，要完完全全读懂每一个细节在做什么。这时候真巴不得当初代码写的更好读一点就好了。
没想到，真找到了一个
public void statedChanged(WorkSetState originalState, WorkSet workSet) {
WorkPackage workPackage = workSet.workPackage;
if (isApproved(workSet)) {
/* ... */
} else {
for (WorkSet sibling : workPackage.workSets) {
if (isPending(sibling)) break;
workPackage.status = WorkPackageStatus.INPROGRESS;
}
}
workPackageRepository.save(workPackage);
}
这段代码的本意，是一次审批计划（WorkPackage）中会分成多个分组（WorkSet），有任何一个分组还未有开始审批，那么整个计划都处于未开始状态。
然而真正实现的是，只要第一个分组是进行中，整个计划就会变为进行中。如果第一个未开始，则整个计划没有开始。
相当出乎意料竟然有这么明显的一个错。我反思了一下在重重测试包围之下还漏掉这个Bug的原因。
- 有集成测试，但是没有验收测试。没有一个地方把业务流程的故事集中讲一遍。因而在较大范围的测试中漏掉了这个点。
- 单元测试中，仅仅构造了一个计划有两个分组的情况，没覆盖到所有分支。
往深一层想，这个方法需要判定的情况比较多。所以针对它的测试用例本来就很多，显得在这个测试类中占了很大的比例。也就是在第二遍读时发现的好像是内嵌了一个小的Test类的情况。写的时候心里就有种喧宾夺主的感觉，不由得怀疑是不是对这个方法过度测试了。进而没有仔细考虑来设计用例。
其实内心产生不协调的真正的原因是实现类违反了单一职责，而不是测试重心失衡。
感受
这次练习的收获收获颇大，虽然理论上自己有着对代码质量，可维护性，以及测试用例设计等方面的各种观点。但是这次时间胶囊式的体验，还是带来了完全不同的心理感受和思索。
特别是第三轮找Bug的挑战。最后找到的时候是我记忆中找到Bug最开心的一次。
