Cómo mejorar el código y evitar peleas en la revisión

No es un secreto que cuando se trata de formar un nuevo equipo, los líderes (líder de equipo, líder tecnológico) enfrentan el desafío de establecer un estilo de programación unificado, ya que todos los miembros del equipo son nuevos y cada uno tiene su propio enfoque para organizar el código y seleccionarlo. prácticas. Normalmente, esto conduce a largos debates durante las revisiones de código, que eventualmente derivan en varias interpretaciones de prácticas conocidas como SOLID, KISS, DRY, etc. Los principios detrás de estas prácticas son bastante confusos y, con suficiente perseverancia, es fácil encontrar paradojas donde una contradice a la otra. Por ejemplo, consideremos Responsabilidad Única y DRY.

Una variación de la definición del Principio de Responsabilidad Única (la "S" en SOLID) establece que cada objeto debe tener una responsabilidad, y esta responsabilidad debe estar completamente encapsulada dentro de la clase. El principio DRY (No te repitas) sugiere evitar la duplicación de código. Sin embargo, si tenemos un objeto de transferencia de datos (DTO) en nuestro código que se puede utilizar en diferentes capas/servicios/módulos, ¿cuál de estos principios deberíamos seguir? Sin duda, muchos libros de programación abordan situaciones similares, generalmente afirmando que si estamos tratando con diferentes objetos/funciones con el mismo conjunto de propiedades y lógica pero que pertenecen a diferentes dominios, no constituye una duplicación. Sin embargo, ¿cómo se puede demostrar que estos objetos DEBEN pertenecer a dominios diferentes y, lo más importante, está el líder preparado (y confiado) para afirmar y probar esta afirmación?

 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.

Este artículo sugiere un enfoque que permite evitar la mayoría de estas situaciones conflictivas. Además, cada desarrollador en la práctica (sin objeciones del líder) comprenderá qué está haciendo mal y cómo mejorarlo.

Para empezar, introduzcamos varias condiciones y definiciones adicionales:

1. En el momento de enviarla para revisión, la tarea se considera completa y, si pasa la revisión, se puede publicar sin ningún cambio. En otras palabras, no consideramos la posibilidad de cambios/adiciones planificados previamente en el código.

2. El equipo está formado por especialistas igualmente experimentados y calificados que no enfrentan ningún desafío en la implementación de tareas; la única discrepancia reside en sus enfoques.

3. El estilo del código es coherente y lo verifican los verificadores de código.

4. El tiempo de desarrollo no es crítico, al menos menos crítico que la confiabilidad del producto.

   
   

   Consideraremos la necesidad de la primera condición más adelante, aunque es bastante obvia por sí sola, ya que es ilógico someter a revisión una tarea inconclusa. Con la segunda condición, nos aseguramos de que cada miembro del equipo no tenga problemas para elegir un algoritmo e implementar la tarea asignada. En la tercera condición, asumimos que el equipo se adhiere a un estilo específico (PSR) y no surgen preguntas como "qué es mejor, CamelCase o Snake_case". Y la condición final se abstiene de calcular cambios en el esfuerzo para completar la tarea en este trabajo.
   

Pruebas unitarias

Muchos lectores saben que las pruebas unitarias mejoran la calidad del código. Normalmente, después de afirmar esto, se menciona como ejemplo la metodología de desarrollo basado en pruebas (TDD), que de hecho mejora la calidad del código, pero se aplica relativamente raramente en la práctica porque escribir pruebas antes de la implementación requiere un conjunto de habilidades de programación de alto nivel.

¿Cómo pueden las pruebas unitarias ayudar a mejorar el código sin depender de las prácticas conocidas mencionadas anteriormente? Primero, recordemos que las pruebas unitarias se aplican para probar un método/módulo/clase específico utilizando objetos/módulos simulados como dependencias.

Siguiendo la primera condición, la tarea debe considerarse completada en el momento de enviarla para su revisión. Por tanto, introduzcamos una definición de lo que consideramos una tarea completada. Una tarea se considera completada sólo cuando cumple todas las condiciones que se enumeran a continuación:

• Se cumplen los requisitos de la tarea asignada.

• Todo el código nuevo debe estar cubierto por pruebas unitarias, incluidas varias condiciones algorítmicas en el programa.

• El nuevo código no viola las pruebas existentes.

  Dado que tenemos tiempo ilimitado para escribir nuevas pruebas y mantener las antiguas (condición 4), y cada desarrollador puede escribir estas pruebas y cumplir con los requisitos de la tarea (condición 2), podemos considerar que cualquier tarea puede potencialmente completarse. Ahora, dado que hemos introducido la definición de tarea completada, podemos justificar la condición 1: el código no se puede enviar para revisión si no está cubierto por pruebas; de lo contrario, el código será rechazado sin revisión. Por lo tanto, un desarrollador sabe que solucionar problemas de código después de recibir comentarios implica corregir pruebas. Este punto aparentemente menor se convierte en una fuerza impulsora fundamental para escribir un buen código.

  
  

  Consideremos el siguiente ejemplo de código (en este artículo, el lenguaje PHP se usa como ejemplo, pero puede ser cualquier lenguaje tipo C compatible con el paradigma de programación orientada a objetos):

 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'); } }

Aquí, violamos intencionalmente todas las prácticas para demostrar la efectividad del enfoque propuesto. Sin embargo, tenga en cuenta que el algoritmo presentado es funcional; Dependiendo del tipo, se crea una entidad con parámetros específicos. Sin embargo, nuestra tarea principal es asegurarnos de que este código no llegue a la etapa de revisión, lo que obligará al desarrollador a mejorarlo de forma independiente. Siguiendo la condición 1, para enviar el código para su revisión, debemos escribir pruebas. Escribamos una de esas pruebas:

 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 } }

Resultó bastante sencillo, pero ésta es sólo una de las ocho pruebas necesarias para uno de los cinco tipos. Una vez escritas todas las pruebas, cualquier comentario durante la revisión que requiera cambios puede interrumpir estas pruebas y el desarrollador tendrá que reescribirlas o ajustarlas. Por ejemplo, agregar una nueva dependencia (digamos, un registrador) dará como resultado cambios en la inicialización de fábrica en todas las pruebas:

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

Observe cómo ha aumentado el costo de un comentario: si antes agregar/cambiar una dependencia solo requería modificaciones en la clase SomeFactory , ahora también será necesario modificar todas las pruebas (que podrían ser más de 40). Naturalmente, después de varias iteraciones de dichos cambios, un desarrollador querrá minimizar el esfuerzo necesario para abordar los comentarios. ¿Cómo se puede hacer esto? La respuesta es obvia: aísle la lógica de creación de entidades para cada tipo en una clase separada. Tenga en cuenta que no nos basamos en principios SÓLIDO/SECO, etc., y no participamos en discusiones abstractas sobre la legibilidad y depuración del código, ya que cada uno de estos argumentos puede ser cuestionado. Simplemente estamos simplificando la redacción de pruebas y el desarrollador no tiene argumentos en contra de esto.

Después de la modificación, tendremos 5 fábricas para cada tipo ( ObjectType::A , ObjectType::B , ObjectType::C , ObjectType::D , ObjectType::E ). A continuación se muestra un ejemplo de la fábrica para 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); } }

Y la fábrica general se verá así:

 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'); } }

Como podemos ver, el código general ha aumentado. Veamos las pruebas de FactoryA y la prueba modificada de 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 }

El número total de pruebas aumentó en 5 (el número de tipos posibles), mientras que el número de pruebas para las fábricas permaneció igual. Entonces, ¿qué hace que este código sea mejor? La principal ventaja es la reducción del esfuerzo necesario para las correcciones tras una revisión del código. De hecho, al cambiar las dependencias en FactoryA , solo se ven afectadas las pruebas de FactoryA .

De acuerdo, el código ya tiene mejor aspecto y, quizás sin querer, nos hemos adherido en parte al principio de responsabilidad única. ¿Es este el final? Como se mencionó anteriormente, todavía necesitamos escribir 5 pruebas para cada entidad. Además, tendríamos que pasar infinitas fábricas al constructor como argumentos para este servicio, e introducir un nuevo tipo (o eliminar uno antiguo) conduciría a cambios en todas las pruebas (aunque ahora solo son 5) para SomeFactory . Por lo tanto, una solución lógica, que probablemente verán la mayoría de los desarrolladores, es crear un registro (especialmente si hay soporte nativo para el registro de clases por interfaz) y declarar interfaces para DTO y fábricas como:

 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); } }

Resaltemos la elección de definir el método getType como estático. En la implementación actual, no hay diferencia si este método es estático o dinámico. Sin embargo, si comenzamos a escribir una prueba para este método (no importa cuán absurda pueda parecer esta idea), podemos notar que en el caso de un método dinámico, la prueba se vería así:

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

Mientras que para un método estático, parecería mucho más corto:

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

Así, gracias a la pereza, elegimos la solución correcta (quizás sin saberlo) y evitamos que el método getType dependa potencialmente del estado del objeto de clase FactoryB .

Veamos el código de registro:

 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); } }

Como podemos ver, tenemos que escribir 3 pruebas: 1) una prueba de duplicación, 2) una prueba cuando no se encuentra la fábrica y 3) una prueba cuando se encuentra la fábrica. La clase SomeFactory ahora parece un método proxy y, por lo tanto, se puede eliminar.

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

Además de la reducción del número de pruebas (de 5 a 3), cualquier adición/eliminación de una nueva fábrica no implica cambios en las pruebas antiguas (suponiendo que el registro de nuevas fábricas sea nativo e integrado en el marco).

Para resumir nuestro progreso: en la búsqueda de una solución para reducir el costo de abordar los comentarios después de una revisión del código, renovamos por completo la generación de objetos basados en tipos. Nuestro código ahora se adhiere a los principios de Responsabilidad única y Abierto/Cerrado (la "S" y la "O" en el acrónimo SOLID), aunque no los mencionamos explícitamente en ninguna parte.

A continuación, hagamos la tarea más compleja y realicemos el mismo trabajo con cambios menos obvios en el código. Examinemos el código en la clase 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); } }

¿Podemos simplificar la escritura de pruebas para este código? Analicemos el primer bloque if:

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

Intentemos cubrirlo con pruebas:

 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']); }

Si la cuestión de la existencia se aborda fácilmente, la prueba del tipo contiene muchos escollos. En esta prueba pasamos una cadena, pero ¿qué pasa con otros tipos? ¿Un número grande se considera un número entero o de punto flotante (por ejemplo, en PHP, 10 elevado a 100 devolverá una representación corta como 1.0E+100 del tipo flotante)? Podrías escribir un DataProvider para todos los escenarios posibles, o podrías extraer la lógica de validación en una clase separada y obtener algo como:

 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); } }

Por un lado, agregamos una nueva dependencia, y quizás incluso tuvimos que crearla. Pero a cambio, en todas las demás fábricas no tenemos que preocuparnos por estos problemas. La prueba en la fábrica actual es solo una y cubre todas las variaciones posibles del parámetro 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); }

Veamos el siguiente bloque de código, a saber:

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

En este bloque, se llama al método de la dependencia aRepository ( findById ), que devuelve nulo o una entidad con el método getSomeParams . El método getSomeParams , a su vez, devuelve una matriz de datos.

Como podemos ver, la variable $aEntity sólo es necesaria para llamar al método getSomeParams . Entonces, ¿por qué no obtener el resultado de getSomeParams directamente si existe y una matriz vacía si no existe?

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

Comparemos las pruebas antes y después. Antes de los cambios, teníamos 3 comportamientos posibles: 1) cuando se encuentra la entidad y getSomeParams devuelve una matriz de datos no vacía, 2) cuando se encuentra la entidad y getSomeParams devuelve una matriz de datos vacía, 3) cuando no se encuentra la entidad.

 // 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);

En el código modificado, sólo hay dos escenarios posibles: findSomeParamsById devuelve una matriz vacía o no.

 // 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']);

Además de reducir la cantidad de pruebas, nos deshicimos de $this->createConfiguredMock(SomeEntity::class, [..] .
A continuación, veamos el bloque:

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

Como ya tenemos una clase que puede extraer datos del tipo requerido, podemos usarla eliminando las comprobaciones del código de fábrica:

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

Al final, obtenemos una clase como:

 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); } }

El método createByParameters tendrá sólo 4 pruebas, a saber:

• una prueba para la primera excepción ( getIntByKey )
• una prueba cuando findSomeParamsById devolvió un resultado no vacío
• una prueba cuando findSomeParamsById devolvió un resultado vacío y se activa la segunda excepción ( getArrayByKey )
• una prueba cuando findSomeParamsById devolvió un resultado vacío y se creó ObjectA con valores de la matriz default
  

Sin embargo, si los requisitos de la tarea lo permiten y ErrorException se puede reemplazar con ExtractorException, el código será aún más corto:

 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); } }

Y sólo habrá dos pruebas:

• una prueba cuando findSomeParamsById devolvió un resultado no vacío

• una prueba cuando findSomeParamsById devolvió un resultado vacío y ObjectA se creó con valores de la matriz default

  
  

Resumamos el trabajo realizado.

Inicialmente, teníamos un código mal escrito que necesitaba cobertura de prueba. Dado que cualquier desarrollador confía en su código (hasta que algo falla con un error), escribir pruebas para él es una tarea larga y monótona que a nadie le gusta. La única forma de escribir menos pruebas es simplificar el código que deben cubrir estas pruebas. Al final, al simplificar y reducir el número de pruebas, el desarrollador mejora el código, sin seguir necesariamente ninguna práctica teórica específica.