Как улучшить код и избежать драк при проверке

Не секрет, что когда дело доходит до формирования новой команды, перед лидерами (Team Leader, Tech Leader) стоит задача создания единого стиля программирования, так как все члены команды новички, и у каждого свой подход к организации кода и подбору кода. практики. Обычно это приводит к длительным дебатам во время ревью кода, которые со временем перерастают в различные интерпретации известных практик, таких как SOLID, KISS, DRY и т. д. Принципы, лежащие в основе этих практик, достаточно размыты, и при достаточной настойчивости их легко найти. парадоксы, где одно противоречит другому. Например, давайте рассмотрим единую ответственность и DRY.

Один из вариантов определения принципа единой ответственности («S» в SOLID) гласит, что каждый объект должен иметь одну ответственность, и эта ответственность должна быть полностью инкапсулирована внутри класса. Принцип DRY (Don't Повторяйте себя) предполагает избегать дублирования кода. Однако если в нашем коде есть объект передачи данных (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. Время разработки не критично, по крайней мере, менее критично, чем надежность продукта.

   
   

   Необходимость первого условия мы рассмотрим позже, хотя она сама по себе вполне очевидна, так как нелогично отправлять на рассмотрение незавершенную задачу. Вторым условием мы гарантируем, что у каждого члена команды не возникнет проблем с выбором алгоритма и реализацией поставленной задачи. В третьем условии мы предполагаем, что команда придерживается определенного стиля (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'); } }

Здесь мы намеренно нарушили все практики, чтобы продемонстрировать эффективность предлагаемого подхода. Однако обратите внимание, что представленный алгоритм функционален; в зависимости от типа создается сущность с определенными параметрами. Тем не менее, наша главная задача — сделать так, чтобы этот код не дошел до стадии проверки, что побудило бы разработчика улучшить его самостоятельно. Следуя условию 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 тестов для каждой сущности. Более того, нам пришлось бы бесконечно передавать фабрики в конструктор в качестве аргументов для этого сервиса, а введение нового типа (или удаление старого) привело бы к изменениям во всех тестах (правда их сейчас всего 5) для SomeFactory . Поэтому логичным решением, которое, скорее всего, увидит большинство разработчиков, является создание реестра (особенно если есть встроенная поддержка регистрации классов по интерфейсу) и объявление интерфейсов для 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), любое добавление/удаление новой фабрики не влечет за собой изменения старых тестов (при условии, что регистрация новых фабрик является нативной и интегрированной в фреймворк).

Подводя итог нашему прогрессу: в поисках решения, позволяющего сократить затраты на обработку отзывов после проверки кода, мы полностью переработали генерацию объектов на основе типов. Наш код теперь соответствует принципам единой ответственности и открытости/закрытости («S» и «O» в аббревиатуре SOLID), хотя мы нигде явно не упоминали их.

Далее усложним задачу и выполним ту же работу с менее очевидными изменениями в коде. Давайте рассмотрим код класса 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 типа float)? Вы можете написать 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 вернул пустой результат, а ObjectA был создан со значениями из массива default
  

Однако если требования задачи это позволяют и 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); } }

И тестов будет всего два:

• тест, когда findSomeParamsById возвращает непустой результат

• тест, когда findSomeParamsById вернул пустой результат, а ObjectA был создан со значениями из массива default

  
  

Подведем итоги проделанной работы.

Изначально у нас был плохо написанный код, который требовал тестового покрытия. Поскольку любой разработчик уверен в своем коде (пока что-то не рухнет с ошибкой), написание тестов для него — долгая и монотонная задача, которая никому не нравится. Единственный способ написать меньше тестов — упростить код, который необходимо охватить этими тестами. В конце концов, упрощая и сокращая количество тестов, разработчик улучшает код, не обязательно следуя каким-то конкретным теоретическим практикам.