Как улучшить код и избежать драк при проверке
12 февраля 2024 г.Не секрет, что когда дело доходит до формирования новой команды, перед лидерами (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.
В данной статье предложен подход, позволяющий избежать большинства подобных спорных ситуаций. Более того, каждый разработчик на практике (без возражений со стороны руководителя) поймет, что он делает не так и как это улучшить.
Для начала введем несколько дополнительных условий и определений:
- На момент отправки на проверку задача считается выполненной, и если она пройдет проверку, ее можно выпустить без каких-либо изменений. Другими словами, мы не рассматриваем возможность заранее запланированных изменений/дополнений в коде.
- Команда состоит из одинаково опытных и квалифицированных специалистов, которые не сталкиваются с трудностями при реализации поставленных задач; единственное расхождение заключается в их подходах.
- Стиль кода единообразен и проверен специалистами по проверке кода.
- Время разработки не имеет решающего значения, по крайней мере, менее критично, чем надежность продукта. ол>
- Требования поставленной задачи выполнены.
- Весь новый код должен быть проверен модульными тестами, включая различные алгоритмические условия в программе.
- Новый код не нарушает существующие тесты.
- тест на первое исключение (
getIntByKey
) - тест, когда
findSomeParamsById
вернул непустой результат - тест, когда
findSomeParamsById
вернул пустой результат и сработало второе исключение (getArrayByKey
) - тест, когда
findSomeParamsById
вернул пустой результат, аObjectA
был создан со значениями из массиваdefault
n - тест, когда
findSomeParamsById
вернул непустой результат - тест, когда
findSomeParamsById
вернул пустой результат, аObjectA
был создан со значениями из массиваdefault
Необходимость первого условия мы рассмотрим позже, хотя она сама по себе вполне очевидна, так как нелогично отправлять на рассмотрение незавершенную задачу. Вторым условием мы гарантируем, что у каждого члена команды не возникнет проблем с выбором алгоритма и реализацией поставленной задачи. В третьем условии мы предполагаем, что команда придерживается определенного стиля (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
}
}
n Это оказалось довольно просто, но это всего лишь один из восьми необходимых тестов для одного из пяти типов. После того, как все тесты написаны, любая обратная связь во время обзора, требующая изменений, может нарушить эти тесты, и разработчику придется их переписывать или корректировать. Например, добавление новой зависимости (скажем, логгера) приведет к изменению заводской инициализации во всех тестах:
$someFactory = new SomeFactory(
$aRepository = $this->createMock(ARepository::class),
$this->createMock(LoggerInterface::class)
);
Обратите внимание, как выросла стоимость комментария: если раньше добавление/изменение зависимости требовало только модификации класса SomeFactory
, то теперь все тесты (а их может быть больше 40) также придется переделывать. Естественно, после нескольких итераций таких изменений разработчик захочет свести к минимуму усилия, необходимые для устранения отзывов. Как это может быть сделано? Ответ очевиден — вынести логику создания сущностей для каждого типа в отдельный класс. Обратите внимание, что мы не опираемся на принципы SOLID/DRY и т.п., а также не ведем абстрактных дискуссий о читаемости и отладке кода, поскольку каждый из этих аргументов может быть оспорен. Мы просто упрощаем написание тестов, и против этого у разработчика нет контраргументов.
После модификации у нас будет по 5 фабрик для каждого типа (ObjectType::A
, ObjectType::B
, ObjectType::C
, < code>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), любое добавление/удаление новой фабрики не влечет за собой изменения старых тестов (при условии, что регистрация новых фабрик является нативной и интегрированной в фреймворк).< /п>
n Подводя итог нашему прогрессу: в поисках решения, позволяющего сократить затраты на обработку отзывов после проверки кода, мы полностью обновили генерацию объектов на основе типов. Наш код теперь соответствует принципам единой ответственности и открытости/закрытости («S» и «O» в аббревиатуре SOLID), хотя мы нигде явно не упоминали их.
n Далее усложним задачу и выполним ту же работу с менее очевидными изменениями в коде. Давайте рассмотрим код класса 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< /code> возвращает пустой массив данных, 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, [..]
. n Далее посмотрим на блок:
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 теста, а именно:
Однако, если требования задачи это позволяют и 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);
}
}
И тестов будет всего два:
Подведем итоги проделанной работы.
Изначально у нас был плохо написанный код, который требовал тестового покрытия. Поскольку любой разработчик уверен в своем коде (пока что-то не рухнет с ошибкой), написание тестов для него — долгая и монотонная задача, которая никому не нравится. Единственный способ написать меньше тестов — упростить код, который необходимо охватить этими тестами. В конечном итоге, упрощая и сокращая количество тестов, разработчик улучшает код, не обязательно следуя каким-то конкретным теоретическим наработкам.
н
Оригинал