1,821 讀數

如何改进代码并避免审查中的争论

经过 Aleksei Dovbenko23m2024/02/11

太長; 讀書

单元测试和懒惰：一种提高技术团队中个人技能的无争议方法。

众所周知，在组建新团队时，领导者（Team Leader、Tech Leader）面临着建立统一编程风格的挑战，因为所有团队成员都是新成员，每个人都有自己的组织代码和选择的方法做法。通常，这会导致代码审查期间的冗长争论，最终升级为对 SOLID、KISS、DRY 等众所周知的实践的各种解释。这些实践背后的原理相当模糊，只要有足够的坚持，很容易发现悖论，其中一个与另一个相矛盾。例如，让我们考虑单一职责和 DRY。

定义单一职责原则（SOLID 中的“S”）的一种变体规定每个对象都应该有一个职责，并且该职责应该完全封装在类中。 DRY 原则（不要重复自己）建议避免代码重复。但是，如果我们的代码中有一个可以在不同层/服务/模块中使用的数据传输对象（DTO），我们应该遵循哪些原则？毫无疑问，许多编程书籍都解决了类似的情况，通常指出，如果我们处理具有相同属性和逻辑集但属于不同域的不同对象/函数，则并不构成重复。然而，如何证明这些对象应该属于不同的域，最重要的是，领导者是否准备好（并且有信心）断言和证明这一声明？

 One frequently practiced approach is making categorical statements like "This is our way/It's the leader's word and we take it for granted" and similar authoritative declarations that emphasize the authority and expertise of the person who came up with these rules. This approach undoubtedly succeeds when dealing with an established team and a project with an existing codebase upon which development continues. But what should be done when the team is new, and the project has just begun? Appeals to authority may not work, as the Team/Tech Leader has not yet established their authority, and each team member believes that their knowledge and approach will be the optimal solution for the future project.

本文提出了一种方法，可以避免大多数此类有争议的情况。此外，每个开发人员在实践中（没有领导的反对）都会明白自己做错了什么以及如何改进。

首先，让我们介绍几个附加条件和定义：

提交审核时，任务视为完成，审核通过后即可发布，无需做任何修改。换句话说，我们不考虑代码中预先计划的更改/添加的可能性。
该团队由同样经验丰富且合格的专家组成，他们在执行任务时不会遇到任何挑战；唯一的差异在于他们的方法。
代码风格一致，并经过代码检查器验证。
开发时间并不重要，至少不如产品的可靠性那么重要。

我们稍后会考虑第一个条件的必要性，尽管它本身就很明显，因为提交未完成的任务进行审查是不合逻辑的。对于第二个条件，我们确保每个团队成员在选择算法和执行分配的任务时没有任何问题。在第三个条件中，我们假设团队遵循特定的风格（PSR），并且不会出现“CamelCase 或 Snake_case 哪个更好”之类的问题。并且最终条件不计算本作品中完成任务所需的努力变化。

单元测试

许多读者都知道单元测试可以提高代码质量。通常，在陈述这一点之后，会提到测试驱动开发（TDD）方法作为示例，该方法确实提高了代码质量，但在实践中应用相对较少，因为在实现之前编写测试需要高级编程技能。

单元测试如何在不依赖前面提到的众所周知的实践的情况下帮助改进代码？首先，让我们回想一下，单元测试用于使用模拟对象/模块作为依赖项来测试特定的方法/模块/类。

满足第一个条件后，任务在提交审查时应被视为已完成。因此，让我们介绍一下我们所认为的已完成任务的定义。仅当任务满足下列所有条件时才被视为完成：

满足所分配任务的要求。
所有新代码都必须经过单元测试，包括程序中的各种算法条件。
新代码不会破坏现有测试。
由于我们有无限的时间来编写新测试和维护旧测试（条件 4），并且每个开发人员都可以编写这些测试并满足任务要求（条件 2），因此我们可以认为任何任务都有可能完成。现在，既然我们已经介绍了已完成任务的定义，我们就可以证明条件1是合理的：如果代码没有被测试覆盖，则不能提交审查；否则，代码将被拒绝而不经过审查。因此，开发人员知道在反馈后修复代码问题涉及修复测试。这个看似微不足道的点却成为编写优秀代码的根本驱动力。

让我们考虑以下代码示例（在本文中，使用 PHP 语言作为示例，但它可以是任何支持面向对象编程范例的类 C 语言）：

 class SomeFactory { public function __construct( private readonly ARepository $aRepository, ) { } /** * @throws ErrorException */ public function createByParameters(ObjectType $type, array $parameters): ObjectE|ObjectD|ObjectA|ObjectB|ObjectC { switch ($type) { case ObjectType::A: if (!array_key_exists('id', $parameters) || !is_int($parameters['id'])) { throw new ErrorException('Some message'); } $aEntity = $this->aRepository->findById($parameters['id']); $data = []; if ($aEntity !== null) { $data = $aEntity->getSomeParams(); } if (count($data) === 0) { if (array_key_exists('default', $parameters) && is_array($parameters['default'])) { $data = $parameters['default']; } else { throw new ErrorException('Some message'); } } return new ObjectA($data); case ObjectType::B: // some code return new ObjectB($parameters); case ObjectType::C: // some code return new ObjectC($parameters); case ObjectType::D: // some code return new ObjectD($parameters); case ObjectType::E: // some code return new ObjectE($parameters); } throw new RuntimeException('some message'); } }

在这里，我们故意违反所有做法，以证明所提出方法的有效性。然而，请注意，所提出的算法是有效的；根据类型，创建具有特定参数的实体。尽管如此，我们的主要任务是确保这段代码不会到达review阶段，促使开发人员独立改进它。满足条件1后，为了提交代码供审查，我们需要编写测试。让我们写一个这样的测试：

 class SomeFactoryTest extends TestCase { public function testCreateByParametersReturnsObjectAWithDefaultMethods(): void { $someFactory = new SomeFactory( $aRepository = $this->createMock(ARepository::class), ); $parameters = [ 'id' => $id = 5, 'default' => ['someData'], ]; $aRepository->expects($this->once()) ->method('findById') ->with($id) ->willReturn(null); $actualResult = $someFactory->createByParameters(ObjectType::A, $parameters); $this->assertInstanceOf(ObjectA::class, $actualResult); // additional checkers for $actualResult } }

事实证明这很简单，但这只是五种类型之一的必要八项测试之一。编写完所有测试后，审查期间任何需要更改的反馈都可能会破坏这些测试，开发人员将不得不重写或调整它们。例如，添加新的依赖项（比如说记录器）将导致所有测试中工厂初始化的更改：

 $someFactory = new SomeFactory( $aRepository = $this->createMock(ARepository::class), $this->createMock(LoggerInterface::class) );

请注意注释的成本是如何增加的：如果以前添加/更改依赖项只需要修改SomeFactory类，那么现在所有测试（可能超过 40 个）也需要更改。当然，在多次迭代此类更改之后，开发人员会希望尽量减少处理反馈所需的工作量。如何才能做到这一点？答案很明显 - 将每种类型的实体创建逻辑隔离到一个单独的类中。请注意，我们不依赖 SOLID/DRY 原则等，并且我们不参与有关代码可读性和调试的抽象讨论，因为每个论点都可能存在争议。我们只是简化了测试的编写，开发人员对此没有反驳的理由。

修改后，我们将为每种类型拥有 5 个工厂（ ObjectType::A 、 ObjectType::B 、 ObjectType::C 、 ObjectType::D 、 ObjectType::E ）。下面是ObjectType::A (FactoryA) 工厂的示例：

 class FactoryA { public function __construct( private readonly ARepository $aRepository, ) { } public function createByParameters(array $parameters): ObjectA { if (!array_key_exists('id', $parameters) || !is_int($parameters['id'])) { throw new ErrorException('Some message'); } $aEntity = $this->aRepository->findById($parameters['id']); $data = []; if ($aEntity !== null) { $data = $aEntity->getSomeParams(); } if (count($data) === 0) { if (array_key_exists('default', $parameters) && is_array($parameters['default'])) { // 6 7 $data = $parameters['default']; } else { throw new ErrorException('Some message'); } } return new ObjectA($data); } }

一般工厂会是这样的：

 class SomeFactory { public function __construct( private readonly FactoryA $factoryA, private readonly FactoryB $factoryB, private readonly FactoryC $factoryC, private readonly FactoryD $factoryD, private readonly FactoryE $factoryE, ) { } /** * @throws ErrorException */ public function createByParameters(ObjectType $type, array $parameters): ObjectE|ObjectD|ObjectA|ObjectB|ObjectC { switch ($type) { case ObjectType::A: return $this->factoryA->createByParameters($parameters); case ObjectType::B: return $this->factoryB->createByParameters($parameters); case ObjectType::C: return $this->factoryC->createByParameters($parameters); case ObjectType::D: return $this->factoryD->createByParameters($parameters); case ObjectType::E: return $this->factoryE->createByParameters($parameters); } throw new RuntimeException('some message'); } }

我们可以看到，整体代码增加了。让我们看看FactoryA的测试和SomeFactory的修改后的测试。

 class FactoryATest extends TestCase { public function testCreateByParametersReturnsObjectAWithDefaultMethods(): void { $factoryA = new FactoryA( $aRepository = $this->createMock(ARepository::class), ); $parameters = [ 'id' => $id = 5, 'default' => ['someData'], ]; $aRepository->expects($this->once()) ->method('findById') ->with($id) ->willReturn(null); $actualResult = $factoryA->createByParameters($parameters); $this->assertInstanceOf(ObjectA::class, $actualResult); // additional checkers for $actualResult } }

 class SomeFactoryTest extends TestCase { public function testCreateByParametersReturnsObjectA(): void { $someFactory = new SomeFactory( $factoryA = $this->createMock(FactoryA::class), $this->createMock(FactoryB::class), $this->createMock(FactoryC::class), $this->createMock(FactoryD::class), $this->createMock(FactoryE::class), ); $parameters = ['someParameters']; $factoryA->expects($this->once()) ->method('createByParameters') ->with($parameters) ->willReturn($objectA = $this->createMock(ObjectA::class)); $this->assertSame($objectA, $someFactory->createByParameters(ObjectType::A, $parameters)); } // the same test for another types and fabrics }

测试总数增加了5个（可能的类型数量），而工厂测试数量保持不变。那么，是什么让这段代码变得更好呢？主要优点是减少了代码审查后纠正所需的工作量。事实上，当更改FactoryA中的依赖项时，只有FactoryA的测试受到影响。

同意，代码已经看起来更好了，而且，也许无意中，我们已经部分遵守了单一责任原则。难道就这样结束了吗？正如前面提到的，我们仍然需要为每个实体编写 5 个测试。此外，我们必须无休止地将工厂作为该服务的参数传递到构造函数中，并且引入新类型（或删除旧类型）将导致SomeFactory的所有测试发生变化（尽管现在只有 5 个）。因此，大多数开发人员可能会看到的一个逻辑解决方案是创建一个注册表（特别是在通过接口对类注册提供本机支持的情况下）并为 DTO 和工厂声明接口，例如：

 interface ObjectInterface { } class ObjectA implements ObjectInterface { // some logic }

 interface FactoryInterface { public function createByParameters(array $parameters): ObjectInterface; public static function getType(): ObjectType; }

 class FactoryB implements FactoryInterface { public static function getType(): ObjectType { return ObjectType::B; } public function createByParameters(array $parameters): ObjectB { // some logic return new ObjectB($parameters); } }

让我们重点介绍将getType方法定义为静态的选择。在当前的实现中，该方法无论是静态还是动态都没有区别。但是，如果我们开始为此方法编写测试（无论这个想法看起来多么荒谬），我们可能会注意到，在动态方法的情况下，测试将如下所示：

 public function testGetTypeReturnsTypeA(): void { $mock = $this->getMockBuilder(FactoryA::class) ->disableOriginalConstructor() ->onlyMethods([]) ->getMock(); $this->assertSame($mock->getType(), ObjectType::A); }

而对于静态方法，它看起来会短得多：

 public function testGetTypeReturnsTypeA(): void { $this->assertSame(FactoryA::getType(), ObjectType::A); }

因此，由于懒惰，我们选择了正确的解决方案（可能是在不知不觉中）并防止getType方法潜在地依赖于FactoryB类对象的状态。

我们看一下注册表代码：

 class SomeRegistry { /** @var array<int, FactoryInterface> */ private readonly array $factories; /** * @param FactoryInterface[] $factories */ public function __construct(array $factories) { $mappedFactory = []; foreach ($factories as $factory) { if (array_key_exists($factory::getType()->value, $mappedFactory)) { throw new RuntimeException('Duplicate message'); } $mappedFactory[$factory::getType()->value] = $factory; } $this->factories = $mappedFactory; } public function createByParams(ObjectType $type, array $parameters): ObjectInterface { $factory = $this->factories[$type->value] ?? null; if ($factory === null) { throw new RuntimeException('Not found exception'); } return $factory->createByParameters($parameters); } }

正如我们所看到的，我们必须编写 3 个测试：1）重复测试，2）未找到工厂时的测试，3）找到工厂时的测试。 SomeFactory类现在看起来像一个代理方法，因此可以删除。

 class SomeFactory { public function __construct( private readonly SomeRegistry $someRegistry, ) { } public function createByParameters(ObjectType $type, array $parameters): ObjectInterface { return $this->someRegistry->createByParams($type, $parameters); } }

除了测试数量的减少（从 5 个减少到 3 个）之外，任何新工厂的添加/删除都不会导致旧测试的更改（假设新工厂的注册是本地的并集成到框架中）。

总结一下我们的进展：为了寻求一种解决方案来降低代码审查后处理反馈的成本，我们彻底修改了基于类型的对象的生成。我们的代码现在遵循单一职责和开放/封闭原则（SOLID 缩写中的“S”和“O”），即使我们没有在任何地方明确提及它们。

接下来，让我们使任务变得更复杂，并在代码中进行不太明显的更改来执行相同的工作。让我们检查一下FactoryA类中的代码：

 class FactoryA implements FactoryInterface { public function __construct( private readonly ARepository $aRepository, ) { } public static function getType(): ObjectType { return ObjectType::A; } public function createByParameters(array $parameters): ObjectA { if (!array_key_exists('id', $parameters) || !is_int($parameters['id'])) { throw new ErrorException('Some message'); } $aEntity = $this->aRepository->findById($parameters['id']); $data = []; if ($aEntity !== null) { $data = $aEntity->getSomeParams(); } if (count($data) === 0) { if (array_key_exists('default', $parameters) && is_array($parameters['default'])) { $data = $parameters['default']; } else { throw new ErrorException('Some message'); } } return new ObjectA($data); } }

我们可以简化这段代码的测试编写吗？让我们分解第一个 if 块：

 if (!array_key_exists('id', $parameters) || !is_int($parameters['id'])) { throw new ErrorException('Some message'); }

让我们尝试用测试来覆盖它：

 public function testCreateByParametersThrowsErrorExceptionWhenParameterIdDoesntExist(): void { $this->expectException(ErrorException::class); $factoryA = new FactoryA( $this->createMock(ARepository::class), ); $factoryA->createByParameters([]); } public function testCreateByParametersThrowsErrorExceptionWhenParameterIdNotInt(): void { $this->expectException(ErrorException::class); $factoryA = new FactoryA( $this->createMock(ARepository::class), ); $factoryA->createByParameters(['id' => 'test']); }

如果存在性问题很容易涵盖，那么对该类型的测试就会包含许多陷阱。在这个测试中，我们传递了一个字符串，但是其他类型呢？大数字被视为整数还是浮点数（例如，在 PHP 中，10 的 100 次方将返回浮点数类型的短表示形式，如 1.0E+100）？您可以为所有可能的场景编写一个 DataProvider，或者您可以将验证逻辑提取到一个单独的类中并得到如下内容：

 class FactoryA implements FactoryInterface { public function __construct( private readonly ARepository $aRepository, private readonly ExtractorFactory $extractorFactory ) { } public static function getType(): ObjectType { return ObjectType::A; } public function createByParameters(array $parameters): ObjectA { $extractor = $this->extractorFactory->createByArray($parameters); try { $id = $extractor->getIntByKey('id'); } catch (ExtractorException $extractorException) { throw new ErrorException('Some message', previous: $extractorException); } $aEntity = $this->aRepository->findById($id); $data = []; if ($aEntity !== null) { $data = $aEntity->getSomeParams(); } if (count($data) === 0) { if (array_key_exists('default', $parameters) && is_array($parameters['default'])) { $data = $parameters['default']; } else { throw new ErrorException('Some message'); } } return new ObjectA($data); } }

一方面，我们添加了一个新的依赖项，也许我们甚至必须创建它。但反过来，在所有其他工厂，我们不必担心此类问题。当前工厂的测试只是其中之一，它涵盖了id参数的所有可能变化：

 public function testCreateByParametersThrowsErrorExceptionWhenParameterIdDoesntExist(): void { $this->expectException(ErrorException::class); $factoryA = new FactoryA( $this->createMock(ARepository::class), $extractorFactory = $this->createMock(ExtractorFactory::class), ); $parameters = ['someParameters']; $extractorFactory->expects($this->once()) ->method('createByArray') ->with($parameters) ->willReturn($extractor = $this->createMock(Extractor::class)); $extractor->expects($this->once()) ->method('getIntByKey') ->with('id') ->willThrowException($this->createMock(ExtractorException::class)); $factoryA->createByParameters($parameters); }

让我们看下一个代码块，即：

 $aEntity = $this->aRepository->findById($id); $data = []; if ($aEntity !== null) { $data = $aEntity->getSomeParams(); } if (count($data) === 0) { // next code

在此块中，调用依赖项aRepository ( findById ) 的方法，该方法返回 null 或使用getSomeParams方法返回实体。 getSomeParams方法又返回一个数据数组。

正如我们所看到的，变量$aEntity仅需要调用getSomeParams方法。那么，为什么不直接获取getSomeParams的结果（如果存在），如果不存在则获取空数组？

 $data = $this->aRepository->findSomeParamsById($id); if (count($data) === 0) {

我们来对比一下之前和之后的测试。在更改之前，我们有 3 种可能的行为：1）当找到实体时， getSomeParams返回一个非空数据数组，2）当找到实体时， getSomeParams返回一个空数据数组，3）当未找到实体。

 // case 1 $aRepository->expects($this->once()) ->method('findById') ->with($id) ->willReturn($this->createConfiguredMock(SomeEntity::class, [ 'getSomeParams' => ['not empty params'] ])); // case 2 $aRepository->expects($this->once()) ->method('findById') ->with($id) ->willReturn($this->createConfiguredMock(SomeEntity::class, [ 'getSomeParams' => [] ])); // case 3 $aRepository->expects($this->once()) ->method('findById') ->with($id) ->willReturn(null);

在修改后的代码中，只有两种可能的情况： findSomeParamsById返回空数组或不返回空数组。

 // case 1 $aRepository->expects($this->once()) ->method('findSomeParamsById') ->with($id) ->willReturn([]); // case 2 $aRepository->expects($this->once()) ->method('findSomeParamsById') ->with($id) ->willReturn(['not empty params']);

除了减少测试数量之外，我们还摆脱了$this->createConfiguredMock(SomeEntity::class, [..] 。
接下来，我们看一下块：

 if (count($data) === 0) { if (array_key_exists('default', $parameters) && is_array($parameters['default'])) { $data = $parameters['default']; } else { throw new ErrorException('Some message'); } }

由于我们已经有一个可以提取所需类型数据的类，因此我们可以使用它，从工厂代码中删除检查：

 if (count($data) === 0) { try { $data = $extractor->getArrayByKey('default'); } catch (ExtractorException $extractorException) { throw new ErrorException('Some message', previous: $extractorException); } }

最后，我们得到一个类：

 class FactoryA implements FactoryInterface { public function __construct( private readonly ARepository $aRepository, private readonly ExtractorFactory $extractorFactory ) { } public static function getType(): ObjectType { return ObjectType::A; } public function createByParameters(array $parameters): ObjectA { $extractor = $this->extractorFactory->createByArray($parameters); try { $id = $extractor->getIntByKey('id'); } catch (ExtractorException $extractorException) { throw new ErrorException('Some message', previous: $extractorException); } $data = $this->aRepository->findSomeParamsById($id); if (count($data) === 0) { try { $data = $extractor->getArrayByKey('default'); } catch (ExtractorException $extractorException) { throw new ErrorException('Some message', previous: $extractorException); } } return new ObjectA($data); } }

createByParameters 方法只有 4 个测试，即：

第一个异常的测试 ( getIntByKey )
findSomeParamsById返回非空结果时的测试
当findSomeParamsById返回空结果并触发第二个异常 ( getArrayByKey ) 时进行测试
当findSomeParamsById返回空结果并且使用default数组中的值创建ObjectA时的测试

不过，如果任务要求允许，并且ErrorException可以替换为ExtractorException,代码会更短：

 class FactoryA implements FactoryInterface { public function __construct( private readonly ARepository $aRepository, private readonly ExtractorFactory $extractorFactory ) { } public static function getType(): ObjectType { return ObjectType::A; } /** * @throws ExtractorException */ public function createByParameters(array $parameters): ObjectA { $extractor = $this->extractorFactory->createByArray($parameters); $id = $extractor->getIntByKey('id'); $data = $this->aRepository->findSomeParamsById($id); if (count($data) === 0) { $data = $extractor->getArrayByKey('default'); } return new ObjectA($data); } }

并且只有两个测试：