重构:这个类太大了

一个来自真实(有缺陷)代码库的重构示例。

在这篇文章中,我将介绍一组来自真实代码库的重构。这并非旨在展示完美,而是代表现实。

2020 年 4 月 14 日



这是一个关于重构的故事。它是 TDD 红-绿-重构循环[1] 中的第三项,也是我们一直都在做的事情,对吧?除非我们没有做。

我有一个不受约束的代码库,它一直遭受着重构的忽视,因此我一直在将其恢复正常。在这篇文章中,我将使用一个过大的类,并将其缩小。

问题概述

故事从一个无聊的家务开始。我写了一些个人记账软件 - Reconciliate。它在命令行上运行,并执行以下操作

  1. 加载我自己的逗号分隔数据
    • 我最近的银行和信用卡交易记录。
    • 我预测的每月和年度交易(基于中央电子表格中的数据)。
    • 任何未对账的先前记录数据。
  2. 加载第三方逗号分隔数据(来自银行和信用卡公司)。
  3. 将第三方数据与我的数据进行对账。
  4. 将所有内容写回中央电子表格。

我有一些需要修复的错误和一些需要添加的功能。但我的典型工作流程是在晚上把最小的孩子哄睡后,才在笔记本电脑前工作,而且离我睡觉的时间只有很短的时间,而且通常是在我上次看到代码后的几周后。在这种情况下,很容易想到诸如“好吧,我知道这段代码很乱,但我现在没有时间去理解它并修复它……”之类的事情。

显然,这不是理想的。

有一个类,特别是ReconciliationIntro 类,每次我看到它都会让我头疼。它臃肿、复杂,而且无法装进我的脑袋 [2]。这造成了一个恶性循环:“因为这段代码很难理解,所以重构将花费比我拥有的更多的时间和精力,所以我将忍受更长时间 - 即使这意味着我甚至无法再进行小的更改,因为我需要花费很长时间才能理解代码的当前状态,并决定更改应该放在哪里。”

例如,我想添加处理另一张信用卡的功能。在很多地方,我使用了多态性策略模式来将每张信用卡的独特行为整齐地封装起来。但ReconciliationIntro 类是一个例外,由于代码设计不佳以及缺乏明确的上下文分离,如果我添加另一张信用卡,我将使一个已经臃肿的类变得更糟。它包含四种类型数据(银行收入、银行支出、信用卡 1 和信用卡 2)的四个重复代码路径,如果我现在添加信用卡 3,我将最终遵循相同的反模式。

我的总体目标是使用策略模式[3] 将此代码替换为更通用的代码。但有四个问题阻碍了我

  1. 这个大型类(ReconciliationIntro)承担了太多责任。
  2. 有很多私有嵌套代码,由于它没有公共接口,因此很难进行单元测试。
  3. ReconciliationIntro 中,有一个大型方法做了太多事情。
  4. ReconciliationIntro 中有几个方法使用相同的重复模式,但细节不同。

我计划按上述顺序解决所有这些问题。这种准备性重构 将使我能够轻松地封装每张信用卡/账户的行为。正如Kent Beck 所说,“让更改变得容易,然后进行简单的更改。”

本文重点介绍如何解决上述列表中的第一个问题:这个类太大了

为什么要重构?

我无法轻松地理解ReconciliationIntro 类,因为它承担了太多责任。它最初被设计为软件的入口大厅,它所做的只是显示一些消息,然后实例化执行主要工作的类。但随着时间的推移,许多其他代码都潜入了进来。我想让添加另一张信用卡变得更容易,所以我将从将这个大型类分解成更小的类开始。

好处将是多方面的

  • 通过将方法组移至单独的类,我将创建明确的上下文,并最大程度地减少紧密耦合。这意味着
    • 当我想进行更改或修复问题时,我会知道去哪里。
    • 当我修改代码时,我只需要处理适合我脑袋[2] 的小而孤立的部分 - 我不必理解整个系统。
    • 任何状态操作都只会发生在小范围内 - 因此我不用担心系统的一个区域会改变另一个区域的状态。

我该怎么做?

在我看来,重构(以及编写新代码)时最重要的原则是要采取微小的步骤。我有几个相互关联的目标

  • 在每一步中,我都希望代码能够编译,测试能够运行。
  • 如果更改导致任何测试失败,我希望能够立即修复它们。通过进行小的更改和小提交,我可以看到哪个更改导致测试失败,并且只需要回滚/检查少量代码即可找到问题。
  • 我可以在脑海中跟踪自己的位置。很容易开始一个相对简单的重构,却发现它会产生超出你最初意图的影响。此时,如果你没有养成在每一步都让代码编译并让测试通过的习惯,你可能会发现自己陷入了难以逃脱的兔子洞:你的代码无法编译,你的测试甚至无法运行,更不用说通过了。

为了使这能够奏效,我需要对要重构的代码进行测试覆盖。这在我开始重构之前就已经完成了,尽管值得注意的是,我未来重构的目标之一是使代码更容易测试。

重构,作者:Martin Fowler 是推荐的进一步阅读材料,它阐述了一些基本的重构原则。他还强调了以微小步骤移动并在每次小提交后构建代码/运行测试的价值。

除了这些基本原则之外,我还将努力遵循下面概述的一系列逻辑步骤。

1. 重排方法

我将首先使用区域ReconciliationIntro 中的所有方法重新排列成合理的组 (提交 f2d9932) [4]

图 1:重新排列方法到区域后的 ReconciliationIntro

现在我已经将我的方法分组到我认为合理的上下文 中,我想将它们提取到单独的类中。但从哪里开始呢?文件加载代码包含最多的重复,并且造成了最大的痛苦。最终,我想使这段代码更通用,但首先我想将其提取出来,以便我能够在没有干扰的情况下看到它。我将提取一个新的FileLoader 类。其他分组也将成为单独的类,但它们会简单得多,因此我将本文的大部分内容集中在文件加载代码上。

2. 分析文件加载方法之间的关系

到目前为止,我所做的只是将一些代码在同一个类中移动。我在提交之前重建了代码并运行了所有测试,但除非我笨手笨脚,否则我希望我之前的提交是微不足道的。我需要对下一步做什么更加谨慎。

我使用了一个区域来识别要提取到一个新的 FileLoader 类中的方法,但我如何才能确定这将起作用?是否存在任何隐藏的依赖关系?我将通过绘制要移动的方法之间的关系来发现这一点。要移动的方法用蓝色标记。

图 3:要移动的文件加载方法(缩写

我立刻就能看到它不是一个严格的独立上下文:一些蓝色方法调用了我想保留在单独类中的方法(以黑色和绿色显示)。我可以通过几种方式来处理这个问题,我将在下面讨论。但现在我已经到了可以定义行动计划的地方。

行动计划

当我完成时,最初的 ReconciliationIntro 大类将被分解成简化版本加上五个新类,如下面的图表所示。请注意,区域和类之间没有一对一的映射,因为稍后当我到达步骤 7时,我将最终将我的第一个分组细化为一些更细粒度的边界。

图 4:行动计划

我的总体目标是将这个大类分解成更小的类。在上面的步骤 1 和步骤 2 中,我使用区域图表来识别代码的不同区域,然后分析一些方法之间的关系。这给了我足够的信息来开始思考如何以微小的步骤来完成这项工作。

结果计划总结如下,并在下面详细描述。我通过思考如何使用尽可能小的步骤来进行,从而得出了这个计划。我通常所做的是进行设置,以便下一个更改尽可能小而简单。我正在进行小的更改以促进更多小的更改 - 再次遵循Kent Beck 的格言,“让更改变得容易,然后进行容易的更改”。

根据您的具体情况,您可能不会遵循相同的计划,但如果您不确定如何进行,它是一个不错的模板。您的首要任务是为安全增量更改做好准备

  1. 重新排列方法

    将类中的所有方法分组到合理的组中,如上面所述。

  2. 分析文件加载方法之间的关系

    确定文件加载代码如何与 ReconciliationIntro 类中的其余代码连接,如上面所述。

  3. 修改保留在后面的方法

    有一些方法将保留在父类中,但目前被要移动的方法调用。需要进行一些修改才能使其正常工作。

  4. 为新的 FileLoader 类创建覆盖测试

    新的 FileLoader 类需要由测试覆盖。要移动的代码的测试已经存在,但它们将被移动到一个新的 FileLoaderTests 类中。

  5. 创建新的 FileLoader 类并移动两个方法

    遵循微小步骤的原则,我将只移动两个方法(一个公共方法和一个被它调用的私有方法),然后再移动其余方法。

  6. 将其他方法移动到新的 FileLoader

    这是事情变得令人兴奋的地方。我感兴趣的所有文件加载代码终于有了新的归宿!

  7. 提取更多新类

    处理完文件加载代码后,我将为其他代码区域创建一些新的类。

新的 FileLoader 类将负责加载各种逗号分隔的数据源并将它们合并以准备进行对账。这个文件加载代码是目前大部分问题的根源,所以我会详细描述它。您可以在这里查看重构前的原始代码,但如果您点击该链接,您只会发现它太大而无法容纳在您的脑海中!简化的重构后版本在这里

所以,现在我可以继续执行计划

3. 修改保留的方法

我已经确定了两个方法 - Set_pathRecursively_ask_for_budgeting_months - 它们被文件加载方法调用

图 5:被文件加载方法调用的方法(缩写

只要这些方法与文件加载类没有太紧密的耦合,我就可以:

  • 将它们设为公共方法,以便我可以从新的 FileLoader 类中调用它们。
  • 将客户端调用从文件加载代码中移到其他本地的 ReconciliationIntro 方法中。

我将对 Recursively_ask_for_budgeting_months 使用第一种方法,对 Set_path 使用第二种方法,这将使我处于以下状态

图 6:移动后的文件加载方法(缩写

请注意,图表中标记为 FileLoader 方法的方法在这一步结束时仍将保留在 ReconciliationIntro 类中,但这将使我能够将它们移动到 FileLoader 类中,这将在下面的步骤 5步骤 6中发生。

Recursively_ask_for_budgeting_months 最终将成为另一个类的公共方法,但现在我只想确保我可以从其他地方调用它。事实证明它已经是公共方法,这是为了便于测试而进行的。这本身就是一个代码异味 - 它表明它最好作为单独类的公共接口的一部分。

从文件加载代码中调用的另一个方法是 Set_path。这会更改内部路径变量的值,所以我将选择选项 2:我将单独调用它并将结果数据通过参数传递给文件加载方法。

请注意,这些通常不会保留为单独的提交(我可以使用微提交,然后将它们压缩成更大的提交),但我将保留小的提交以使步骤清晰。我在每一步都编译并运行测试

  1. 修改调用方法(Create_pending_csvs),使其接受一个参数,但最初给它一个默认值(提交 acc3519[4],以便代码仍然可以编译

    private void Create_pending_csvs()
    {
      // Some code
    }
    
    private void Create_pending_csvs(string path = "")
    {
      // Some code
    }
    
  2. 在调用 Create_pending_csvs 之前,单独调用 Set_path。获取结果新的 _path 成员变量(参见边栏),并将其传递给 Create_pending_csvs提交 c5ebc2f[4]

    case "1":
    {
      Create_pending_csvs();
    }
    break;
    
    case "1":
    {
      Set_path();
      Create_pending_csvs(_path);
    }
    break;
    
  3. Create_pending_csvs 中删除对 Set_path 的调用,并使用传递的值而不是成员变量(提交 6df8f97[4]

    private void Create_pending_csvs(string path = "")
    {
      try
      {
        Set_path();
        var pending_csv_file_creator = new PendingCsvFileCreator(_path);
    
    private void Create_pending_csvs(string path = "")
    {
      try
      {
        Set_path();
        var pending_csv_file_creator = new PendingCsvFileCreator(path);
    
  4. 最后,从 Create_pending_csvs 参数中删除默认值 - 从而强制所有客户端传递一个值(提交 2be56ea[4]。请注意,通过按此顺序进行操作,我始终保持代码可以编译

    private void Create_pending_csvs(string path = "")
    {
      // Some code
    }
    
    private void Create_pending_csvs(string path = "")
    {
      // Some code
    }
    

4. 为新的 FileLoader 类创建覆盖测试

我首选要移动的方法是 Bank_and_bank_out_ _Merge_bespoke_data_with_pending_file。我要做的第一件事是将任何覆盖测试复制到即将创建的 FileLoader 类的新的测试类中。此方法已经有一个测试 - M_MergeBespokeDataWithPendingFile_ WillAddMostRecentCredCardDirectDebits - 它的作用是确保此方法将新的直接借记数据与“待处理”文件(用于包含所有新的交易数据)正确合并。

请注意,我只移动测试,而不是编写新的测试。人们可能已经习惯了 TDD [1] 的概念,即在编写代码之前编写测试,因此他们认为在每次处理代码时都需要编写新的测试。在重构时,情况往往并非如此。理想情况下,我的功能将已经被测试覆盖,在重构时,我不会更改功能。因此,我不会编写新的测试,而是使用现有的测试来验证功能仍然按预期工作。

现在是审查此测试的好时机:我已经有一段时间没有编写它了,所以我应该能够快速发现它是否有意义。我希望我的测试清晰易读 - 它们应该充当系统行为的文档。我首先注意到它包含一个断言方法 - Assert_direct_debit_details_are_correct - 它的名称不恰当。“正确”的定义是什么?我重写了测试以使其更易读,这涉及相当多的更改。为了使本文易于理解,我不会详细介绍所做的更改,但您可以在提交 f090f26提交 6a6cece中查看它们。[4]

现在我已经重构了测试,我将把它复制到一个新的测试类中,以及一些相关的私有辅助方法。请注意,虽然此测试和其他测试注定要在新的 FileLoader 类上运行,但它们仍然会在旧的 ReconciliationIntro 类上运行,直到我确定我的新测试类拥有它需要的一切。还要注意,在所有内容安全移动之前,我的测试代码是重复的。

我首先在与原始文件相同的文件中创建新的测试类(commit 491c795[4],这样便于查看我正在复制的内容。然后,我可以使用 Resharper [5] 和 Visual Studio 将所有内容移动到一个新文件中(commit c9317c0[4]

图 7:移动 BBO 测试

5. 创建新的 FileLoader 类并移动两个方法

现在我的测试类已经启动并运行,我可以创建新的 FileLoader 类。

链条中最底层的方法 - 我的方法树 中最低的叶子 - 是 Bank_and_bank_out__Add_most_recent_credit_card_direct_debits。这是一个私有方法,没有独立的测试(它通过公共调用方法进行测试),因此我将移动它及其调用者(Bank_and_bank_out__Merge_bespoke_data_with_pending_file)。它们将是第一个被移动到新类中的两个方法。

同样,我将逐步移动,以避免我的测试变红,并确保我的代码始终能够构建。在执行以下每个步骤后,我都会确保代码能够构建并且测试通过。

  1. 这是我开始时的状况。已创建 FileLoaderTests 类,但它正在测试仍位于 ReconciliationIntro 类中的代码

    图 8:新的 FileLoader 类第 1 部分(缩写

  2. 我首先创建一个新的 FileLoader 类。我的一个文件加载方法(Bank_and_bank_out__ Add_most_recent_credit_card_direct_debits)是私有的,并且在这个过程结束时也将是私有的。它只会被即将跟随它进入新类的方法调用。它没有单独的测试覆盖,因此我可以直接将其复制到新类中,并在其调用者移动时准备就绪(参见 commit 0341476[4]。但我需要将其暂时设置为公共方法

    图 9:新的 FileLoader 类第 2 部分(缩写

  3. 现在,我可以在 ReconciliationIntro 类中创建新 FileLoader 类的实例,并调用其新的公共方法,而不是旧的私有方法。我还可以删除旧的私有方法(参见 commit bde2ae2[4]

    图 10:新的 FileLoader 类第 3 部分(缩写

  4. 将原始调用者复制到新类中(参见 commit f0a5a59[4]。请注意,此时它被复制了

    图 11:新的 FileLoader 类第 4 部分(缩写

  5. 将我正在测试的对象从 ReconciliationIntro 实例更改为新的 FileLoader 实例。将测试指向原始调用者的新副本。请注意,由于它调用的私有方法也被复制了,因此我的测试将通过(参见 commit 3d573e3[4]

    图 12:新的 FileLoader 类第 5 部分(缩写

  6. 现在,我可以直接从 ReconciliationIntro 类中调用原始调用者(参见 commit 77c0b14[4]

    图 13:新的 FileLoader 类第 6 部分(缩写

  7. 将最初的私有方法再次设为私有,并删除原始调用者。我还会删除旧的测试类,因为它的所有测试现在都在 FileLoaderTests 中被复制了。(参见 commit 27f1a59[4]

    图 14:新的 FileLoader 类第 7 部分(缩写

6. 将其他方法移动到新的 FileLoader

现在,我可以移动所有其他方法。我将逐个处理它们,从最简单的开始,并注意依赖关系(方法之间的依赖关系,以及对任何成员数据的依赖关系)。我需要考虑以下几点

  • 我是否要重命名任何方法?
  • 我是否要将任何参数列表替换为新的对象?
  • 是否存在任何冗余参数?
  • 内部嵌套的被调用者是否正在更改状态?这会产生什么影响?

我可以在移动它们之前将它们内联到更低层,但如果这样做,我会破坏将它们作为公共方法调用的测试。因此,我将它们单独移动。我按照下面解释的顺序进行操作,一次操作一个,从链条末端开始 - 即以下树中最外层的叶子

图 15:FileLoader 方法树(缩写

对于每个方法,我使用以下方法

  • 在目标类中创建该方法的副本,保留原始方法。将新方法设为公共方法。
  • 调用新方法,而不是原始方法。
  • 将任何覆盖测试复制到新的测试类中,并确保它们测试新代码。
  • 删除旧方法和旧测试。

我已经移动了 Bank_and_bank_out__ Merge_bespoke_data_with_pending_fileBank_and_bank_out__ Add_most_recent_credit_card_direct_debits(从 commit 0341476commit 27f1a59),因此现在我将对其他每个方法重复相同的操作(从 commit 7cd53f6commit 7ab95f2[4]。这次我没有提交每个微小的步骤,但我仍然执行相同的步骤集。在每个步骤之后,我都会确保代码能够构建并且测试通过(除了我故意使测试失败的情况)。

我之前重构了一些测试,我可以对遵循相同模式的测试重复这些更改。对于某些方法,移动非常简单,因为它们没有测试覆盖。这是我进行此重构的部分原因,以便使该代码更易于测试。

7. 提取更多新类

在开头添加区域 时,我发现了一些 获取预算月份 功能,它创建了自己的清晰 上下文,因此我将这些方法提取到一个新的 BudgetingMonthService 类中。这非常快且简单,因为这些方法只有一个公共入口点(参见 commit 6103f0b[4]

ReconciliationIntro 仍然太大,但这些方法相互调用,而且现在我已经花更多时间重新熟悉代码,我不确定我剩下的两个区域 用户说明和输入调试电子表格操作 是否是划分剩余代码的最佳方式。为了帮助自己思考,我使用电子表格快速说明调用层次结构。

图 16:调用层次结构

这使我能够看到,实际上存在三个独立的代码区域,而不是两个:用户说明收集文件/路径信息调试模式切换代码

我删除了原始区域(commit 4c57927),并用四个新区域替换它们(commit 3446a54[4]。我重新排列方法以适应新的区域,这些区域现在将转换为三个新类:CommunicatorPathSetterDebugModeSwitcherFileLoaderBudgetingMonthService 未显示在此图中,因为它们已经被提取了)

图 17:识别最终的 ReconciliationIntro 类

我使用与 BudgetingMonthService 相同的原则,逐步安全地创建这些新类,并在它们完成其目的后删除新区域(参见 commit 7f464a4commit 7e118c1)。

值得注意的是,当从较大的类中提取新类时,我希望它们是独立的。出于这个原因,我故意避免了有时会出现的反模式,即提取的类被自动作为依赖项注入到原始类的构造函数中。

PathSetter 类被证明并非微不足道 - 请参见 commit 2921220commit 398539a[4]。这是由于当前路径设置代码的稍微复杂性,这是我在 步骤 3 结束时 注意到的。通过将此代码提取到一个单独的类中并赋予它自己的清晰 上下文,我已经使这段代码变得更好了一些 - 但它仍然需要一些关注。

最后,我的 ReconciliationIntro更简洁、更简单,原始的 41 个方法已减少到三个:StartReconciliateDo_matching

图 18:最终的 ReconciliationIntro 类

回顾

我的类太大了。我养成了不良的编码习惯,我想在添加任何新功能之前停下来,让事情变得更好。

为了修复我的过大类,我采取了以下措施

以微小的步骤移动,并在每一步都编译并运行测试。现在我有一个更小的类,它调用来自其他几个也更小的类的功能,并且每个类都更容易被我理解。

下一步是什么?

请注意,本文在重构过程中结束,因此如果您想在继续执行下一步之前查看代码的相关状态,则需要 检出 提交 6103f0b。另请注意,我在本文中完成了大部分 步骤 7,该步骤是在该提交 *之后* 完成的(解释 在此)。

提交 6103f0b 中,我已经将文件加载代码提取到 FileLoader 类中,并且对主要挑战有了更清晰的认识。以下四个方法(在 此处 的原位可见)显然存在问题。

  • Load_bank_and_bank_in
  • Load_bank_and_bank_out
  • Load_cred_card1_and_cred_card1_in_out
  • Load_cred_card2_and_cred_card2_in_out

它们存在以下问题。

  • 存在大量重复 - 乍一看,它们看起来完全相同。
  • 它们太长了。
  • 它们在内部创建对象并将它们以一种紧密相互依赖的方式传递给彼此,这不可测试。

这些都是我想解决的问题,但在进行更多重构之前,我确实需要围绕这些方法进行一些测试。这就是我接下来要做的。我希望能在一篇后续文章中写下它,但我已经吸取了教训,不要对这种事情做出承诺,所以现在它是一项读者的练习。


致谢

非常感谢以下人士,他们非常友好地阅读了本文的早期草稿,并提供了宝贵的反馈和建议:Paula PaulMartin FowlerPriti BiyaniRiccardo NovagliaDan Terhorst-NorthKevlin HenneySteve FreemanJon SkeetSal FreudenbergJoe RayChris ShepherdLuke MortonScott GiminianiRichard Foster、Marcos Bezerra、Sam Carrington

脚注

1: TDD 代表测试驱动开发,这是一种确保所有代码单元都经过测试,并且测试描述系统行为的技术。这是通过在编写使这些测试通过的代码 *之前* 编写测试来完成的。关于它还有很多可以说的,但这并不是本文的重点。您可以 在此 阅读更多相关内容。

2: “在我的脑海中” 是 Dan Terhorst-North 的 Software, Faster 书籍(仍在进行中的作品)中的模式之一。他谈到了能够通过“在我的脑海中”来理解任何代码块的重要性。这个名字来自 James Lewis,Dan 在 这次演讲 中描述了它,但 Dan 告诉我这个概念可能起源于 Alistair Jones

3: 重构为策略模式:此重构的目标很大程度上是为了能够使用 策略模式。Joshua Kerievsky 的 专门针对“重构为模式”的书籍 - 如果你想了解更多,值得一读。正如 Martin Fowler 所说,“许多人表示他们发现重构方法是学习模式的更好方法,因为你看到了问题和解决方案之间相互作用的逐步过程。”

4: 提交链接:不要觉得有义务遵循提交链接!更多内容请参见 如何阅读本文 侧边栏

5: Resharper 是一个常用的 Visual Studio 扩展,用于代码编辑等操作。但是它不是免费的,并且越来越被本地的 Visual Studio 工具所取代。

重大修订

2020 年 4 月 14 日:首次发布