Como melhorar o código e evitar brigas na revisão

Não é segredo que quando se trata de formar uma nova equipe, os líderes (Team Leader, Tech Leader) enfrentam o desafio de estabelecer um estilo de programação unificado, pois todos os membros da equipe são novos e cada um tem sua própria abordagem para organizar o código e selecionar práticas. Normalmente, isso leva a longos debates durante as revisões de código, que eventualmente se transformam em várias interpretações de práticas bem conhecidas, como SOLID, KISS, DRY, etc. paradoxos onde um contradiz o outro. Por exemplo, vamos considerar Responsabilidade Única e DRY.

Uma variação da definição do Princípio de Responsabilidade Única (o "S" no SOLID) afirma que cada objeto deve ter uma responsabilidade, e essa responsabilidade deve ser totalmente encapsulada dentro da classe. O princípio DRY (Don't Repeat Yourself) sugere evitar a duplicação de código. Porém, se tivermos um objeto de transferência de dados (DTO) em nosso código que possa ser usado em diferentes camadas/serviços/módulos, qual destes princípios devemos seguir? Sem dúvida, muitos livros de programação abordam situações semelhantes, normalmente afirmando que se estivermos lidando com objetos/funções diferentes com o mesmo conjunto de propriedades e lógica, mas pertencentes a domínios diferentes, isso não constitui duplicação. Contudo, como se pode provar que estes objectos DEVEM pertencer a domínios diferentes e, o mais importante, está o líder pronto (e confiante) para afirmar e provar esta afirmação?

 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 artigo sugere uma abordagem que permite evitar a maioria dessas situações contenciosas. Além disso, cada desenvolvedor na prática (sem objeções do líder) entenderá o que está fazendo de errado e como melhorar.

Para começar, vamos apresentar várias condições e definições adicionais:

1. No momento do envio para revisão, a tarefa é considerada concluída e, caso seja aprovada na revisão, poderá ser liberada sem nenhuma alteração. Em outras palavras, não consideramos a possibilidade de alterações/adições pré-planejadas no código.

2. A equipa é composta por especialistas igualmente experientes e qualificados que não enfrentam desafios na implementação das tarefas; a única discrepância reside nas suas abordagens.

3. O estilo do código é consistente e verificado por verificadores de código.

4. O tempo de desenvolvimento não é crítico, pelo menos menos crítico que a confiabilidade do produto.

   
   

   Consideraremos a necessidade da primeira condição mais tarde, embora seja bastante óbvia por si só, pois é ilógico submeter uma tarefa inacabada para revisão. Com a segunda condição, garantimos que cada membro da equipe não tenha problemas em escolher um algoritmo e implementar a tarefa atribuída. Na terceira condição, assumimos que a equipe adere a um estilo específico (PSR), e não surgem questões como “o que é melhor, CamelCase ou Snake_case”. E a condição final é abster-se de calcular mudanças no esforço para conclusão da tarefa neste trabalho.
   

Testes unitários

Muitos leitores estão cientes de que o teste unitário melhora a qualidade do código. Normalmente, depois de afirmar isso, a metodologia de desenvolvimento orientado a testes (TDD) é mencionada como exemplo, o que de fato melhora a qualidade do código, mas é relativamente raramente aplicado na prática porque escrever testes antes da implementação requer um conjunto de habilidades de programação de alto nível.

Como os testes unitários podem ajudar a melhorar o código sem depender das práticas conhecidas mencionadas anteriormente? Primeiro, vamos lembrar que os testes unitários são aplicados para testar um método/módulo/classe específico usando objetos/módulos simulados como dependências.

Seguindo a primeira condição, a tarefa deve ser considerada concluída no momento da submissão para revisão. Portanto, vamos apresentar uma definição para o que consideramos uma tarefa concluída. Uma tarefa é considerada concluída somente quando satisfaz todas as condições listadas abaixo:

• Os requisitos da tarefa atribuída são atendidos.

• Todo novo código deve ser coberto por testes unitários, incluindo diversas condições algorítmicas no programa.

• O novo código não quebra os testes existentes.

  Como temos tempo ilimitado para escrever novos testes e manter os antigos (condição 4), e cada desenvolvedor pode escrever esses testes e atender aos requisitos da tarefa (condição 2), podemos considerar que qualquer tarefa pode ser potencialmente concluída. Agora, como introduzimos a definição de tarefa concluída, podemos justificar a condição 1: o código não pode ser submetido para revisão se não for coberto por testes; caso contrário, o código será rejeitado sem revisão. Portanto, um desenvolvedor sabe que corrigir problemas de código após feedback envolve corrigir testes. Este ponto aparentemente secundário torna-se uma força motriz fundamental para escrever um bom código.

  
  

  Vamos considerar o seguinte exemplo de código (neste artigo, a linguagem PHP é usada para exemplos, mas pode ser qualquer linguagem semelhante a C com suporte ao paradigma de programação 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'); } }

Aqui, violamos intencionalmente todas as práticas para demonstrar a eficácia da abordagem proposta. Entretanto, observe que o algoritmo apresentado é funcional; dependendo do tipo, é criada uma entidade com parâmetros específicos. No entanto, a nossa principal tarefa é garantir que este código não chegue à fase de revisão, levando o desenvolvedor a melhorá-lo de forma independente. Seguindo a condição 1, para enviar o código para revisão, precisamos escrever testes. Vamos escrever um desses testes:

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

Acabou sendo bastante simples, mas este é apenas um dos oito testes necessários para um dos cinco tipos. Depois que todos os testes forem escritos, qualquer feedback durante a revisão que exija alterações poderá quebrar esses testes e o desenvolvedor terá que reescrevê-los ou ajustá-los. Por exemplo, adicionar uma nova dependência (digamos, um logger) resultará em alterações na inicialização de fábrica em todos os testes:

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

Observe como o custo de um comentário aumentou: se anteriormente adicionar/alterar uma dependência exigia apenas modificações na classe SomeFactory , agora todos os testes (que poderiam ser mais de 40) também precisarão ser alterados. Naturalmente, após várias iterações de tais mudanças, um desenvolvedor desejaria minimizar o esforço necessário para abordar o feedback. Como isso pode ser feito? A resposta é óbvia: isole a lógica de criação de entidade para cada tipo em uma classe separada. Observe que não estamos confiando nos princípios SOLID/DRY, etc., e não estamos envolvidos em discussões abstratas sobre legibilidade e depuração de código, pois cada um desses argumentos pode ser contestado. Estamos simplesmente simplificando a escrita de testes e não há contra-argumentos para um desenvolvedor contra isso.

Após a modificação, teremos 5 fábricas para cada tipo ( ObjectType::A , ObjectType::B , ObjectType::C , ObjectType::D , ObjectType::E ). Abaixo está um exemplo de 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); } }

E a fábrica geral ficará assim:

 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, o código geral aumentou. Vejamos os testes para FactoryA e o teste modificado para 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 }

O número total de testes aumentou em 5 (o número de tipos possíveis), enquanto o número de testes para fábricas permaneceu o mesmo. Então, o que torna esse código melhor? A principal vantagem é a redução do esforço necessário para correções após uma revisão de código. Na verdade, ao alterar as dependências em FactoryA , apenas os testes de FactoryA são afetados.

Concordo que o código já parece melhor e, talvez sem querer, aderimos parcialmente ao princípio da responsabilidade única. Este é o fim de tudo? Conforme mencionado anteriormente, ainda precisamos escrever 5 testes para cada entidade. Além disso, teríamos que passar infinitamente as fábricas para o construtor como argumentos para este serviço, e a introdução de um novo tipo (ou a remoção de um antigo) levaria a alterações em todos os testes (embora agora sejam apenas 5) para SomeFactory . Portanto, uma solução lógica, que a maioria dos desenvolvedores provavelmente verá, é criar um registro (especialmente se houver suporte nativo para registro de classes por interface) e declarar interfaces para DTOs e 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); } }

Vamos destacar a escolha de definir o método getType como estático. Na implementação atual, não há diferença se este método é estático ou dinâmico. Porém, se começarmos a escrever um teste para este método (por mais absurda que essa ideia possa parecer), poderemos notar que no caso de um método dinâmico, o teste ficaria assim:

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

Enquanto para um método estático, pareceria muito mais curto:

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

Assim, graças à preguiça, escolhemos a solução certa (talvez sem saber) e evitamos que o método getType dependesse potencialmente do estado do objeto da classe FactoryB .

Vejamos o código do 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, temos que escrever 3 testes: 1) um teste para duplicação, 2) um teste quando a fábrica não for encontrada e 3) um teste quando a fábrica for encontrada. A classe SomeFactory agora se parece com um método proxy e pode, portanto, ser removida.

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

Além da redução do número de testes (de 5 para 3), qualquer adição/remoção de uma nova fábrica não implica alterações em testes antigos (assumindo que o registo de novas fábricas é nativo e integrado no quadro).

Resumindo nosso progresso: na busca por uma solução para reduzir o custo de tratamento do feedback após uma revisão de código, reformulamos completamente a geração de objetos baseados em tipos. Nosso código agora adere aos princípios de Responsabilidade Única e Aberto/Fechado (o "S" e "O" na sigla SOLID), embora não os tenhamos mencionado explicitamente em nenhum lugar.

A seguir, vamos tornar a tarefa mais complexa e realizar o mesmo trabalho com alterações menos óbvias no código. Vamos examinar o código na classe 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 a escrita de testes para este código? Vamos analisar o primeiro bloco if:

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

Vamos tentar cobrir isso com testes:

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

Se a questão da existência for abordada facilmente, o teste para o tipo contém muitas armadilhas. Neste teste, passamos uma string, mas e os outros tipos? Um número grande é considerado um número inteiro ou de ponto flutuante (por exemplo, em PHP, 10 elevado a 100 retornará uma representação curta como 1.0E+100 do tipo float)? Você poderia escrever um DataProvider para todos os cenários possíveis ou extrair a lógica de validação em uma classe separada e obter 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 um lado, adicionamos uma nova dependência, e talvez até tenhamos que criá-la. Mas em troca, em todas as outras fábricas, não precisamos nos preocupar com tais problemas. O teste na fábrica atual é apenas um e cobre todas as variações possíveis do 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); }

Vejamos o próximo bloco de código, a saber:

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

Neste bloco é chamado o método da dependência aRepository ( findById ), que retorna nulo ou uma entidade com o método getSomeParams . O método getSomeParams , por sua vez, retorna uma matriz de dados.

Como podemos ver, a variável $aEntity só é necessária para chamar o método getSomeParams . Então, por que não obter o resultado de getSomeParams diretamente, se existir, e um array vazio, se não existir?

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

Vamos comparar os testes antes e depois. Antes das mudanças, tínhamos 3 comportamentos possíveis: 1) quando a entidade é encontrada, e getSomeParams retorna um array não vazio de dados, 2) quando a entidade é encontrada, e getSomeParams retorna um array vazio de dados, 3) quando o entidade não foi encontrada.

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

No código modificado, existem apenas dois cenários possíveis: findSomeParamsById retorna um array vazio ou não.

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

Além de reduzir o número de testes, nos livramos de $this->createConfiguredMock(SomeEntity::class, [..] .
A seguir, vamos dar uma olhada no bloco:

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

Como já temos uma classe que pode extrair dados do tipo requerido, podemos utilizá-la, removendo as verificações do código de fábrica:

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

No final, obtemos uma classe 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); } }

O método createByParameters terá apenas 4 testes, a saber:

• um teste para a primeira exceção ( getIntByKey )
• um teste quando findSomeParamsById retornou um resultado não vazio
• um teste quando findSomeParamsById retornou um resultado vazio e a segunda exceção ( getArrayByKey ) é acionada
• um teste quando findSomeParamsById retornou um resultado vazio e ObjectA foi criado com valores da matriz default
  

No entanto, se os requisitos da tarefa permitirem e ErrorException puder ser substituído por ExtractorException, o código será ainda mais curto:

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

E haverá apenas dois testes:

• um teste quando findSomeParamsById retornou um resultado não vazio

• um teste quando findSomeParamsById retornou um resultado vazio e ObjectA foi criado com valores da matriz default

  
  

Vamos resumir o trabalho realizado.

Inicialmente, tínhamos um código mal escrito que precisava de cobertura de testes. Como qualquer desenvolvedor está confiante em seu código (até que algo trave com erro), escrever testes para ele é uma tarefa longa e monótona da qual ninguém gosta. A única maneira de escrever menos testes é simplificar o código que precisa ser coberto por esses testes. No final, ao simplificar e reduzir o número de testes, o desenvolvedor aprimora o código, sem necessariamente seguir nenhuma prática teórica específica.