用enum的constant-specific class，消除了复杂类NewsListAdapter的17个if-else分支

[Bin WU (Ben) 伍斌]

我已经让GitHub的开源安卓应用的Android Studio支持了单元测试，并在单元测试的保护下，用enum的constant-specific class，消除了复杂类NewsListAdapter的17个if-else分支。这就是我最近“驯服开源烂代码”分享的话题大致内容。代码如下，期待反馈：https://github.com/wubin28/android/commits/master

伍斌_Ben

[申健Jacky]

这个重构手法是没问题。

但是单看https://github.com/wubin28/android/commit/31f1a34c47988eda0ea2e0855b51c467014aa539  这个commit，代码比原来多了不少（这与Java的啰嗦气质也有关系），还得看实际调用的部分因此而节省的代码量才好比较。是不是只有setIconAndFormatStyledText（）这一出用到了这个Type常量。只有两处以上用到了，才有重构价值。

Br,

申健Jacky Shen

个人网站：www.JackyShen.com   新浪微博：@申导

| 敏捷教练 | Agile Coach | Scrum培训 | CSP | ATDD |

| XP | 管理3.0 | Facilitation | Kanban | 软件开发顾问 |

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

在 2015年3月23日，上午11:08，Bin WU (Ben) 伍斌 <wub...@gmail.com> 写道：

我已经让GitHub的开源安卓应用的Android Studio支持了单元测试，并在单元测试的保护下，用enum的constant-specific class，消除了复杂类NewsListAdapter的17个if-else分支。这就是我最近“驯服开源烂代码”分享的话题大致内容。代码如下，期待反馈：https://github.com/wubin28/android/commits/master

伍斌_Ben

--
您收到此邮件是因为您订阅了Google网上论坛上的“agileshanghai”群组。
要退订此群组并停止接收此群组的电子邮件，请发送电子邮件到agileshangha...@googlegroups.com。
要查看更多选项，请访问https://groups.google.com/d/optout。

[武可]

关于罗嗦的问题，我觉得是不是可以考虑这样。在父类中调用generateIconAndFormatStyledText，子类中只需要指定foarmat...方法，以及返回的ICON常量即可。

另外是否可以更进一步，把各种format...方法抽象为一个Formater对象? 这样的话，可能一个Hashmap就可以处理这段逻辑了。

[申健Jacky]

你这是FP/C语言的路子：p，问题是Java里的lambda不是不好使吗

Br,

申健Jacky Shen

个人网站：www.JackyShen.com   新浪微博：@申导

| 敏捷教练 | Agile Coach | Scrum培训 | CSP | ATDD |

| XP | 管理3.0 | Facilitation | Kanban | 软件开发顾问 |

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

[Zhao Ran]

我理解enum constant-specific class本质上就是EventType -->
generateIconAndFormatStyledText函数块的MAP，在Android上Java不支持函数作为变量的限制下，用接口+多态是个很不错的思路。武可的建议我也有同感，format...方法似乎不是IconAndViewTextManager的职责。

另外测试函数有大量的样板代码，如果只是EventType（输入）和
Icon（输出）不同，可以用@RunWith(Parameterized.class)处理。棘手的是其中还有对formatXXX的mock和verify，有什么方法可以去除重复呢？

在 15/3/24，武可<madc...@gmail.com> 写道：

> 关于罗嗦的问题，我觉得是不是可以考虑这样。在父类中调用generateIconAndFormatStyledText，子类中只需要指定foarmat...方法，以及返回的ICON常量即可。
> 另外是否可以更进一步，把各种format...方法抽象为一个Formater对象? 这样的话，可能一个Hashmap就可以处理这段逻辑了。
>
> 2015-03-24 9:47 GMT+08:00 申健Jacky <meb...@gmail.com>:
>
>> 这个重构手法是没问题。
>> 但是单看
>> https://github.com/wubin28/android/commit/31f1a34c47988eda0ea2e0855b51c467014aa539
>> 这个commit，代码比原来多了不少（这与Java的啰嗦气质也有关系），还得看实际调用的部分因此而节省的代码量才好比较。是不是只有
>> setIconAndFormatStyledText（）这一出用到了这个Type常量。只有两处以上用到了，才有重构价值。
>>
>>
>> Br,
>> 申健Jacky Shen

>> 个人网站：www.JackyShen.com <http://www.jackyshen.com/> 新浪微博：@申导

>> | 敏捷教练 | Agile Coach | Scrum培训 | CSP | ATDD |
>> | XP | 管理3.0 | Facilitation | Kanban | 软件开发顾问 |
>> ========================================
>>
>> 在 2015年3月23日，上午11:08，Bin WU (Ben) 伍斌 <wub...@gmail.com> 写道：
>>
>> 我已经让GitHub的开源安卓应用的Android
>> Studio支持了单元测试，并在单元测试的保护下，用enum的constant-specific
>> class，消除了复杂类NewsListAdapter的17个if-else分支。这就是我最近“驯服开源烂代码”分享的话题大致内容。代码如下，期待反馈：
>> https://github.com/wubin28/android/commits/master
>>
>> 伍斌_Ben
>>
>> --
>> 您收到此邮件是因为您订阅了Google网上论坛上的“agileshanghai”群组。
>> 要退订此群组并停止接收此群组的电子邮件，请发送电子邮件到agileshangha...@googlegroups.com。
>> 要查看更多选项，请访问https://groups.google.com/d/optout。
>>
>>
>> --
>> 您收到此邮件是因为您订阅了Google网上论坛上的“agileshanghai”群组。
>> 要退订此群组并停止接收此群组的电子邮件，请发送电子邮件到agileshangha...@googlegroups.com。
>> 要查看更多选项，请访问https://groups.google.com/d/optout。
>>
>

> --
> 您收到此邮件是因为您订阅了 Google 网上论坛的“agileshanghai”群组。

[Bin WU (Ben) 伍斌]

这个重构主要还是练习如何用多态来替换if-else语句。确实没有减少多少代码。不过用enum替换了if-else 后，在android studio 里面的Inspect code工具来扫描代码时，会发现这个类的由多条if-else所造成的类的复杂性问题就被解决了。

Ben

[Bin WU (Ben) 伍斌]

代码就是按照您说的，父类就是enum EventType，子类就是这个enum中的每一个枚举常量，它们都override了父类的generateIconAndFormatStyledText()方法。

Hashmap也是一个不错的思路。

Ben

[Bin WU (Ben) 伍斌]

我已经把android studio中的JDK改为java8，连接手机，运行程序、运行集成测试和单元测试都没有问题。接下来就可以尝试用java 8 里面的lambda来搞一搞，看看是否能精简代码。

Ben

[武可]

可能我没说清楚，想说的意思是，子类不直接override generateIconAndFormatStyledText方法。

而是在父类里调用其它方法，子类override其它发放。

比如每个子类的override方法里基本上做了两件事

   1. Formatxxx

   2. Return ICON_xxx

父类的generateIconAndFormatStyledText调用formatText()，然后return getIcon()。子类中override这两个方法即可。

再进一步，对于ICON_xxx的返回值，可以改成父类中的一个成员变量，构造时传入。这样只要在实例化每个子类是传入对应常量值，子类只需要override formatText一个方法。

也许还可以继续重构，把Formatxxx方法改为方法对象，大致上等于

  Formatxxx(a,b,c) => new xxxFormatter(a, b, c).exec();

如果在定义各子类时a，b，c等参数已经可以确定，那么Formatter实例就可以产生了。这样一来，子类不需要override formatText方法。

因而只需要一个类，在实例化中传入Formatter和ICON常量即可。

[Ben Wu]

不妨fork一下我的repo，在您的repo里直接改代码，表达您的想法。

Ben

Sent from my iPhone

[Joseph Yao]

好主意，顶一下。:) 我才 Fork 了伍斌的 repo 并装了 Android Studio，打算自己过一下代码看看。支持大家用代码交流。

[Joseph Yao]

@伍斌，

从你3月份发布这个重构练习之后，我就一直想在你的基础上把那段代码的重构做下去。本来想在 2015 年做完的，看来是来不及了。虽然代码和单元测试已经发生了较大的变化，但是我想离完成大概还有三分之一吧（程序员的估算，你懂的）。最新的代码可以看这里 https://github.com/JosephYao/android/tree/more_refactoring，我还会继续做下去，心得体会什么的等做完再分享了。希望在农历新年之前可以完成吧（程序员的估算 again）。:)

另外，我把你的Kata更新到Kata接力里面了（第十七棒），感谢你的分享。https://www.evernote.com/l/ALxVummdBfdLqpAyFpL_8oytwI31kOgXU_4

谢谢，

Joseph

[Joseph Yao]

新年好，先给大家拜个晚年。趁着春节在家的机会，我终于把重构做完了（不出所料的没能在春节前完成）。算到今天，这次的重构刚好做了半年的时间（第一次提交是去年的 8月11日）。这期间，我得到不少体会，学习和心得，在这里分享出来，大家在春节里闲来无事可以看看。:) 最终代码可以在这里找到 https://github.com/JosephYao/android/tree/more_refactoring。为了方便大家理解，我加上了一些 tag，也推荐安装一个 SourceTree 来看代码。

先说一下背景，这次重构最早起源于伍斌的分享 https://github.com/wubin28/android/commits/master，这个代码库实际上是 Github 的 Android 客户端，其中有个类叫 NewsListAdaptor。通过重构，伍斌把其中有关事件处理的代码分解到了 IconAndViewTextManager 和 EventType 这两个类里面，并且还加上了必要的单元测试。我理解这个重构中的重点是把原本的 if-else 结构代码变成了 EventType 中的枚举值，消除了很多重复代码。大家可以通过 ben_refactor_start_point 和 ben_refactor_end_point 这两个标签来看到重构前后不同的代码。@伍斌，如果我漏说了什么，请帮忙补充一下。:)

为什么我觉得伍斌已经重构过的代码还可以继续重构下去？

伍斌分享代码之后有过讨论，提出了一些可以进一步重构的点（比如武可之前的回复）。不过可能因为没有展示进一步重构的代码实例，讨论没能进行下去。伍斌当时提出 fork 代码继续重构的想法，我很认同，所以就决定这样做了。当然，我也看到了一些问题，觉得有继续重构的必要。

• EventType 这个类有一些代码臭味。比如，名字里面有“Type”就是一个臭味，因为类已经是代表类型了。具体来说，Event或许本身应该是一个类或者接口，而他的那些枚举值则更应该像是不同的子类。另外，EventType 中的枚举值除了返回 icon 的字符串外，都只是调用了 IconAndViewTextManager 中的一个方法。这种代码使得 EventType 这个枚举类更像是一个“中间人”。
• 除了代码，单元测试中也存在问题。在 IconAndViewTextManagerTest 中，虽然测试的是 setIconAndFormatStyledText 方法，但是却把 IconAndViewTextManager 中被 EventType 调用的那些方法（如 formatCommitComment）都隔离掉了（如 doNothing().when(spyIconAndViewTextManager).formatCommitComment(event, null, null);）。这种做法导致测试并不彻底，仅仅是验证了设置的 icon 是否正确。

最终代码和重构中的体会，学习和心得

我重构的起点是 joseph_refactor_start_point 这个标签，结束的地方就是这个分支的 head。重构的代码依然是 IconAndViewTextManager 中的 setIconAndFormatStyledText 方法和其相关的代码。下面是我在整个过程中的一些体会，学习和心得。

代码：

• 从最终代码来看，EventType 这个类消失了，取而代之的是 UserEvent。同时，IconAndViewTextManager 这个类的大部分代码也都被“转移”到了 UserEvent 和其他业务对象身上。整个代码结构由表示“用户事件”的业务对象（UserEvent）以及表示不同显示目的的业务对象（如 User, Repo, CommitComment 等等）组成。每个业务对象实际上都包括相应的接口，工厂类和实现类。以 UserEvent 为例，UserEvent 是接口，UserEventFactory 是工厂类，CommitCommentUserEvent 就是实现类之一（表示某一种用户事件）。又如，以 User 为例，User 是接口，UserFactory 是工厂类，EmptyUser 和 NonEmptyUser 是两个实现类，表示不显示和需要显示的用户。
• 从代码职责来看，UserEvent 关心的是如何来显示某个事件中的所有信息（如 User, Repo, CommitComment 等等）。他关心的是这些信息显示的顺序和组合，但是不关心具体每个信息是如何显示的。而这些信息，比如 User，则关心具体应该如何显示，但是不关心如何被 UserEvent 所使用。所有这些业务对象实际上是对原有代码的分解，每个对象只负责一件事情，这符合“单一职责”的设计原则。
• 对将来可能发生的代码修改而言，如果有新的信息需要显示，只要新增一个业务对象的实现就可以了。比如，如果需要增加一个新的加星的 Action ，那只要实现一个 StarAction 并修改 ActionFactory 就可以了。新的 UserEvent 也可以用类似的方法添加。而且因为每个对象的职责单一且独立，应该很容易找到需要改的代码在哪里，这符合“开闭”的设计原则。
• 相比最终代码和 joseph_refactor_start_point 这个标签上的代码，类的数量从原来的 2 个增加到了 81 个。或许你会觉得这样的代码不够“直接”，因为无法一眼看到代码的全貌和执行过程，这是一个常见的反馈。直观的感受的确是这样，但是我觉得那种“直接”的代码往往需要更多的时间来理解和修改（因为可能需要修改不止一个地方）。而像这样不够“直接”的代码通常只要改一个地方就可以了，因为很多代码的修改根本就可以不需要了解代码的全貌，这样的代码才是低耦合的。即使要了解代码的全貌，我感觉只要有个强大的调试器就可以了。比如，貌似在最新的 Visual Studio 里面，调试的时候会自动“内联”显示代码，而不是跳转到另一个类或者文件去。

测试：

• 从最终测试代码来看，IconAndViewTextManagerTest 这个类消失了，那个类里面的测试被其他针对每个 UserEvent 的测试所代替了。新的 UserEvent 测试对 main 和 details 这两个 StyledText 进行了隔离，测试了 setIconAndFormatStyledText 这个方法在不同 UserEvent 中的 icon 和对 main 和 details 的显示操作行为（比如 bold 和 append）。这些测试基本做到了对每个 UserEvent 的每行代码的覆盖。
• 在这些测试中，我使用 Test Data Builder Pattern（以下简称 Builder ）来构建测试所需要的对象和数据，比如 Event (不是那个 UserEvent) 和他所包含的各种 Payload。Builder 是一种非常有效的测试数据组织模式，可以减少测试中的重复代码，简化测试数据的准备但又不失灵活性。他最早应该是由 Nat Pryce 在他的 Blog 中提出的 http://natpryce.com/articles/000714.html。Nat 是 GOOS 一书的两位作者之一，而 GOOS 中的代码实例也大量使用了这个模式。我在其他项目中也有使用 Builder 的测试，效果都非常不错。
• Builder 对测试屏蔽了数据的构建细节，这一点类似 Factory 的作用。在这次重构中，这种封装带来了一个好处。如果切到标签 builder_still_use_mock，你会看到那时的 Builder 都是用 mock 框架来构建数据的（如 mock(CreatePayload.class) ）。但是那之后，我发现其实那些 Payload 对象都是一些简单的数据对象（所谓的 Java Bean），这样的对象实际上应该直接构建（比如 new CreatePayload() ）。于是，我在之后的提交中就开始逐步修改这些 Payload 的构建。幸好使用了 Builder，使得测试不用做任何修改，测试也没有因此而运行失败。另一方面，我在另一个项目中遇到过相反的情况，就是一开始 Builder 只需要直接构建对象就可以了，后来新的测试需要隔离对象的行为，就只好改用 mock 了，因为用了 Builder 所以测试不用修改。
• 在使用 Builder 的过程中，我也发现了一种情况需要思考。以 CommitCommentPayloadBuilder 为例，他内部使用了另一个 CommitCommentBuilder 来创建一个 CommitComment，不过他自己对外提供了两个方法来给 CommitCommentBuilder 设置 CommitId 和 Comment。也就是说 CommitCommentPayloadBuilder 对使用者隐藏了使用 CommitCommentBuilder 这个细节，而并没有选择把后者暴露给测试代码。这实际上和其他测试中直接 EventBuilder 和 CommitCommentPayloadBuilder 的做法有所不同。我之所以选择这样做，实际上是牺牲了 Builder 的一部分灵活性，目的是为了减少测试代码对测试数据对象层级结构的依赖。如果将来 CommitCommentPayload 与 CommitComment 之间的关系发生变化（比如 CommitId 转移到了 CommitCommentPayload 身上），那么测试代码就不需要修改。反之，如果把 CommitCommentBuilder 暴露给测试，那么所有使用这个 Builder 的测试则都需要修改。相对的，Event 和 Payload 之间的关系和结构更加稳定，我觉得暴露给测试没有问题。这种手法我在其他项目中也有使用并得到了好的效果，感觉用在自己不熟悉的遗留代码对象上面会比较合适。
• 另外，我第一次在测试代码中使用 Java 的 Generic（就是传说中的 < >）去掉了测试代码的重复。具体可以看 no_comment_builder_yet 这个标签和之后的两个提交。
• 最后，我本来想用 Mutation Test 工具 pitest 来看一下测试代码的有效性的（就是看看测试是否可以发现错误的代码修改）。无奈 pitest 的 gradle 插件并不支持 Android https://github.com/szpak/gradle-pitest-plugin/issues/31，只能暂时作罢了。 

过程：

• 对遗留代码进行重构之前，添加测试必不可少。我做的第一件事情就是写一个新的测试（TestCommitCommentEvent 中的 actor_should_be_bold）。这个测试很重要，他证明了直接 mock 掉 main 和 details 这两个依赖（来自 Android）并不是很困难。后面所有的测试和 Builder 都是在这个测试基础上重构和发展出来的。
• 虽然添加测试很重要，但是我也没有一味的先补完所有 Event 的测试再去重构代码。在第一个 CommitCommentEvent 的测试补完之后，我就开始对 CommitCommentEvent 相关代码进行重构，目的是更早的验证自己的设计思路。
• 对于 Builder 和业务对象（如 PayloadRef 和 Repo）的提取，我也做得比较早。因为只有更早的重构出这些对象，才有机会在后续的重构中不断验证和调整这些设计思路。
• 小步重构，小步添加测试和频繁运行测试非常重要。我用 gradle test 来运行测试，平均半分钟到一分钟运行一次吧。后来发现 gradle test --daemon 可以加速测试运行，效果不错。我在整个重构过程一共提交了近 290 次，每次提交的代码量一般在50行之内，很少有超过 100 行代码的。
• 在这半年里面，我每次要重新开始重构代码时，基本都不记得自己上次做到哪里了。除了看最后一次提交记录之外，我都会看一下最新的测试代码测了什么逻辑，还需不需要补充测试。好在测试代码都容易理解，我很快能知道从哪里继续下去。我现在觉得测试代码除了要可读之外，另外一个重要的特性是意图明确，没有任何无关的信息，且测试代码的因果关系明确。这样的测试可以让人更快更准确的理解，而且新增测试也很容易。欢迎大家看一下那 87 个测试并给出你的反馈，特别是那些不容易理解的地方。

最终的代码和测试代码的设计都不是我在一开始的时候就可以想到的，这里面的业务对象和Builder 都在重构过程中“浮现”出来的，都是嗅着“代码臭味”而找到的设计。

不知不觉写了这么多，我也是醉了。说实话，我做了半年才把这次重构做到一个可以分享的地步，我更是醉了。不多说了，再次祝大家新年快乐。

谢谢，

Joseph