Не секрет, что при формировании новой команды руководители (Team Leader, Tech Leader) сталкиваются с проблемой формирования единого стиля написания программ, так как все члены команды новые, и у каждого из них свой подход к организации кода и выбору используемой практики. Как правило, в большинстве случаев это приводит к длинным диспутам на ревью, которые в итоге перетекают в различные толкования известных практик, таких как SOLID, KISS, DRY, и т.д. Принципы использования этих практик довольно размыты и, при должном упорстве, легко найти парадокс, когда одна из них противоречит другой. Например, рассмотрим Single Responsibility и DRY.
Одна из вариаций определения принципа единой ответственности (Single Responsibility - буква S из аббревиатуры SOLID) гласит, что каждый объект должен иметь одну ответственность, и эта ответственность должна быть полностью инкапсулирована в класс. Принцип DRY (Don’t repeat yourself) предлагает избегать дублирования в коде. Однако, если у нас в коде есть один набор данных (DTO), который может использоваться в разных слоях/сервисах/модулях, какому из этих принципов нам следовать? Безусловно, во многих книгах по программированию разбираются похожие ситуации, как правило, в них говорится, что если речь идет о разных объектах/функциях с одинаковым набором свойств и логики, но принадлежащим разным доменным областям, то дублированием это не является. Но как доказать что эти объекты ДОЛЖНЫ принадлежать разным доменным областям, и, главное, готов (и уверен ли в своих силах) руководитель доказывать это утверждение?
Один из часто практикуемых вариантов - это категоричное заявление “У нас так принято/Слово руководителя закон” и тому подобные ультимативные утверждения, которые подчеркивают авторитет и экспертизу придумавшего эти правила. Такой подход определенно имеет шанс на успех, если мы говорим об уже состоявшейся команде и проекте с некой кодовой базой, на основе которой продолжается разработка. Но что делать, когда команда новая, а проект только начался? Взывание к авторитету не может работать, так как Team/Tech Leader еще им не обладает, а каждый член команды считает, что именно его знания и подход станет оптимальным решением для будущего проекта.
В данной статье предлагается подход, позволяющий избежать большинство подобных спорных ситуаций. Более того, каждый разработчик на практике (и без замечаний со стороны руководителя) будет понимать, что именно он делает неправильно и как можно сделать лучше.
Для начала введем несколько дополнительных условий и определений:
на момент отправки на ревью задача считается завершенной, и, в случае прохождения ревью, может быть зарелизена без каких-либо изменений. То есть, мы не рассматриваем возможность заранее запланированных изменений/дополнений в коде;
команда состоит из равнозначных по опыту и квалификации специалистов, у которых нет проблем в имплементации задач, специалисты имеют только несогласованность в подходах;
стиль кода согласован и проверяется code checkers;
время разработки не критично (как минимум, менее критично, чем надежность продукта).
Необходимость первого условия рассмотрим позже, хотя само по себе оно вполне очевидно, так как нелогично отправлять на проверку незавершенную задачу. Вторым условием мы гарантируем, что каждый член команды не имеет проблем с выбором алгоритма и реализацией поставленной задачи. В третьем условии мы допускаем, что команда следует определенному стилю (PSR) и вопросы типа “что лучше CamelCase или snake_case” не возникают. И финальным условием мы отказываемся от расчета изменения трудозатрат на выполнения задач в данной работе.
Unit tests
Многие из читателей знают, что unit тестирование повышает качество кода, как правило, после данного утверждения приводят в пример методологию TDD (Test-driven development), которая, действительно, повышает качество кода, но довольно редко применима на практике ввиду того, что писать тест ДО реализации может только программист высокого класса.
Так как может помочь unit тестирование в решении задачи улучшения кода без использования перечисленных ранее известных практик? Для начала напомним, что unit тесты применяются для тестирования определенного метода/модуля/класса, используя в качестве зависимостей объекты/модули заглушки (mocks).
Следуя первому условию, на момент отправки на ревью задача должна быть завершенной, поэтому введем определение, что мы называем завершенной задачей. Задача считается завершенной тогда и только тогда, когда она удовлетворяет всем перечисленным ниже условиям:
выполнены требования поставленной задачи;
весь новый код должен быть покрыт unit тестами, включая различные условия алгоритмов программ;
новый код не ломает существующие тесты.
Так как время на написание новых тестов и поддержку старых у нас не ограничено (условие 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.
Согласитесь, код уже выглядит лучше, и, сами того не подозревая, мы отчасти выполнили принцип single responsibility. На этом все? Как было сказано ранее, нам все еще надо написать 5 тестов для каждой сущности. Более того, фабрики для этого сервиса нужно бесконечно прокидывать в конструктор как аргументы, и ввод нового типа (или удаление старого) приводит к изменению всех тестов (хоть их уже не 40, а всего 5) для SomeFactory. Потому логичным решением - большинство разработчиков и сами увидят эту возможность - будет сделать registry (особенно если уже есть нативная поддержка регистрации классов по интерфейсу) и объявить интерфейсы для 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.
Рассмотрим код registry:
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), любое добавление/удаление новой фабрики не влечет за собой изменения старых тестов (при условии, что регистрация новых фабрик нативная и встроена во фреймворк).
Подведем промежуточный итог: в поисках решения для уменьшения цены исправлений замечаний после ревью, мы полностью переделали генерацию объектов в зависимости от типов. Наш код стал соответствовать single responsibility и open/closing принципам (буквы 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']);
}
Если вопрос существования покрывается легко, то тест для типа содержит множество подводных камней. В данном тесте мы пробросили строку, но что будет с другими типами? Считается ли большое число целым числом (integer) или типом с плавающей точкой (например, в PHP 10 в сотой степени вернет короткую запись вида 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'.
Итог
Изначально у нас был плохо написанный код, который необходимо было покрыть тестами. Так как любой разработчик уверен в своем коде (пока что-то не упало с ошибкой), написание тестов для него - долгая и монотонная работа, которую никто не любит. Единственный вариант писать меньше тестов - упростить код, который этими тестами надо покрыть. В конечном счете, разработчик, упрощая и уменьшая количество тестов, улучшает код, при этом не руководствуясь какими-то не было теоретическими практиками.
Безусловно, такой подход увеличивает время разработки, но в тоже время, он заставляет писать легко тестируемый, а, следовательно, хороший код, повышая как уровень разработчика, так и уровень команды в целом, так как этот процесс быстро входит в привычку.
