コードを改善し、レビューでの争いを回避する方法

新しいチームを結成する際、リーダー (チーム リーダー、技術リーダー) が統一されたプログラミング スタイルを確立するという課題に直面することは周知の事実です。チーム メンバー全員が新人であり、それぞれがコードの編成と選択に対して独自のアプローチを持っているためです。実践。通常、これはコード レビュー中に長い議論につながり、最終的には SOLID、KISS、DRY などのよく知られているプラクティスのさまざまな解釈にエスカレートします。これらのプラクティスの背後にある原則は非常に曖昧であり、十分な粘り強さがあれば、簡単に見つけることができます。一方が他方と矛盾するパラドックス。たとえば、単一責任と DRY について考えてみましょう。

単一責任原則 (SOLID の「S」) を定義するバリエーションの 1 つは、各オブジェクトが 1 つの責任を持つ必要があり、この責任はクラス内に完全にカプセル化される必要があると規定しています。 DRY 原則 (Don'trepeat Yourself) では、コードの重複を避けることが推奨されています。ただし、コード内にさまざまなレイヤー/サービス/モジュールで使用できるデータ転送オブジェクト (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.

この記事では、そのような論争の多い状況のほとんどを回避できるアプローチを提案します。さらに、実際の各開発者は（リーダーの反対なしに）自分たちが間違っていることとそれを改善する方法を理解するでしょう。

まず、いくつかの追加の条件と定義を紹介します。

1. レビューのために送信された時点で、タスクは完了したとみなされ、レビューに合格した場合は、変更せずにリリースできます。言い換えれば、コードの事前に計画された変更/追加の可能性は考慮されていません。

2. チームは同等の経験と資格を持つスペシャリストで構成されており、タスクの実装において何の困難も感じません。唯一の相違点は、彼らのアプローチにあります。

3. コード スタイルは一貫しており、コード チェッカーによって検証されます。

4. 開発時間は重要ではありませんが、少なくとも製品の信頼性ほど重要ではありません。

   
   

   最初の条件の必要性については後で検討しますが、未完了のタスクをレビューのために提出するのは非論理的であるため、それ自体は非常に明白です。 2 番目の条件では、各チーム メンバーがアルゴリズムの選択と割り当てられたタスクの実装に問題がないことを保証します。 3 番目の条件では、チームが特定のスタイル (PSR) に準拠していると仮定し、「キャメルケースとスネークケースのどちらが優れているのか」といった質問は生じません。そして、この作業では、最終条件はタスク完了のための努力の変化を計算することを控えています。
   

単体テスト

多くの読者は、単体テストによってコードの品質が向上することを認識しています。通常、これを述べた後、テスト駆動開発 (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'); } }

ここでは、提案されたアプローチの有効性を実証するために、意図的にすべての慣行に違反しました。ただし、提示されたアルゴリズムは機能することに注意してください。タイプに応じて、特定のパラメータを持つエンティティが作成されます。それにもかかわらず、私たちの主なタスクは、このコードがレビュー段階に到達しないようにして、開発者に独自の改善を促すことです。条件 1 に従って、レビューのためにコードを送信するには、テストを作成する必要があります。そのようなテストを 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 } }

これは非常に単純であることが判明しましたが、これは 5 つのタイプのうちの 1 つに必要な 8 つのテストのうちの 1 つにすぎません。すべてのテストが作成された後、変更を必要とするレビュー中のフィードバックによってこれらのテストが中断される可能性があり、開発者はテストを書き直すか調整する必要があります。たとえば、新しい依存関係 (ロガーなど) を追加すると、すべてのテストでファクトリの初期化が変更されます。

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

コメントのコストがどのように増加しているかに注意してください。以前は依存関係の追加/変更にSomeFactoryクラスの変更のみが必要であったのに、現在ではすべてのテスト (40 を超える可能性があります) も変更する必要があります。当然のことながら、このような変更を数回繰り返した後、開発者はフィードバックに対処するために必要な労力を最小限に抑えたいと考えるでしょう。これはどうすればできるのでしょうか?答えは明らかです。各タイプのエンティティ作成ロジックを別のクラスに分離します。私たちは SOLID/DRY 原則などに依存しておらず、コードの可読性やデバッグに関する抽象的な議論には参加していないことに注意してください。これらの議論にはそれぞれ異論がある可能性があります。私たちはテストの作成を単純化しているだけであり、これに対して開発者が反論することはできません。

変更後は、各タイプ ( ObjectType::A 、 ObjectType::B 、 ObjectType::C 、 ObjectType::D 、 ObjectType::E ) に 5 つのファクトリが存在します。以下は、 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); } }

ご覧のとおり、1) 重複に関するテスト、2) ファクトリが見つからない場合のテスト、3) ファクトリが見つかった場合のテストの 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 乗は float 型の 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); } }

一方で、新しい依存関係を追加し、おそらくそれを作成する必要さえあったでしょう。しかしその代わりに、他のすべての工場ではそのような問題を心配する必要はありません。現在のファクトリのテストは 1 つにすぎず、 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 ) のメソッドが呼び出され、 getSomeParamsメソッドで null またはエンティティを返します。 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);

変更されたコードでは、考えられるシナリオは 2 つだけです。findSomeParamsById 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空の結果を返し、2 番目の例外 ( 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); } }

そして、テストは 2 つだけです。

• findSomeParamsById空ではない結果を返したときのテスト

• findSomeParamsById空の結果を返し、 ObjectA default配列の値で作成されたときのテスト

  
  

行った作業をまとめてみましょう。

当初、私たちはテストカバレッジを必要とするコードの書き方が不十分でした。開発者は誰でも自分のコードに自信を持っているので（何かがエラーでクラッシュするまでは）、そのコードのテストを書くのは長くて単調な作業であり、誰も好きではありません。作成するテストの数を減らす唯一の方法は、これらのテストでカバーする必要があるコードを簡素化することです。最終的に、開発者は、必ずしも特定の理論的な実践に従うことなく、テストを簡素化し、減らすことによってコードを改善します。