O que devo fazer como revisor quando o código "precisa ser" ruim?

Trabalho em um projeto mal projetado e superengenharia, onde as revisões obrigatórias de código foram introduzidas há alguns meses.

Me pediram para revisar um pedaço substancial de código que implementa um novo recurso. Ele apresenta as mesmas falhas do restante da nossa base de código. Eu entendo que, em grande parte, essas falhas se infiltram no novo código, por exemplo. tendo que herdar da classe mal projetada ou implementar uma interface mal projetada, mantendo a compatibilidade, e não posso oferecer uma solução muito melhor que não envolva a reescrita de metade da base de código. Mas acho que, do ponto de vista da engenharia, não é bom para uma base de código já quebrada que estamos quebrando ainda mais. O código que estou analisando é definitivamente ruim, mas deve ser para que o recurso seja implementado.

Como devo me comportar com relação a essa revisão específica? Existe uma maneira de eu manter a integridade e permanecer construtivo?

Observe que estou perguntando sobre os limites da revisão de código nesse contexto. Percebo que o problema é causado por outros fatores, incluindo organização e cultura do trabalho, mas o que quero saber é como lidar com a própria revisão.

Simples:

1. Documente sua dívida técnica

Você identificou um pedaço de código que funciona, mas tem alguns problemas técnicos. Isso é dívida técnica . Como outros tipos de dívida, piora com o tempo se não for tratada.

O primeiro passo para lidar com a dívida técnica é documentá-la. Faça isso adicionando itens no rastreador de problemas que descrevem a dívida. Isso o ajudará a obter uma imagem mais clara da magnitude do problema e também a planejar um plano para resolvê-lo.

2. Pague gradualmente sua dívida

Modifique seu processo de desenvolvimento de software para levar em consideração o pagamento da dívida técnica. Isso pode envolver um sprint de endurecimento ocasional ou simplesmente a resolução de itens de dívida em intervalos regulares (por exemplo, 2 itens por semana). A parte importante para garantir que você esteja pagando sua dívida mais rapidamente do que a acumulada (dívida, mesmo dívida técnica, tem interesse nela).

Depois de chegar a um ponto em que você não tem mais um déficit técnico, está no caminho de uma base de código mais saudável :)

Como nota lateral: procure um novo emprego. Este não melhoraria.

Os objetivos do código que você está revisando são:

• Para enviar um recurso, que deve funcionar de acordo com os requisitos.

• Reduzir o crescimento da dívida técnica.

O primeiro objetivo é revisado, verificando se os testes de unidade, integração, sistema e funcional estão aqui, são relevantes e cobrem todas as situações que precisam ser testadas. Você também deve verificar as crenças que o autor original pode ter sobre a linguagem de programação, o que pode levar a erros sutis ou ao código que finge fazer algo diferente do que realmente faz.

O segundo objetivo é aquele em que sua pergunta está focada. Por um lado, não é esperado que o novo código aumente a dívida técnica. Por outro lado, o escopo da revisão é o próprio código, mas no contexto de toda a base de código. A partir daí, você, como revisor, pode esperar duas abordagens do autor original:

• O código externo não é minha culpa. Acabei de implementar o recurso e não me importo com toda a base de código.

  Nessa perspectiva, o código copiará as falhas da base de código e, inevitavelmente, aumentará a dívida técnica: mais códigos ruins são sempre piores.

  Embora seja uma abordagem válida de curto prazo, em longo prazo, resultaria em atrasos crescentes e baixa produtividade e, eventualmente, levaria o processo de desenvolvimento a ser tão caro e arriscado, que o produto pararia de evoluir.

• Escrever um novo código é uma oportunidade para refatorar o antigo.

  Nesta perspectiva, o efeito das falhas do código legado no novo poderia ser limitado. Além disso, a dívida técnica pode ser reduzida, ou pelo menos não aumentada proporcionalmente ao crescimento do código.

  Embora essa seja uma abordagem válida a longo prazo, ela apresenta riscos a curto prazo. O principal é que, a curto prazo, às vezes levaria mais tempo para enviar o recurso específico. Outro aspecto importante é que, se o código legado não for testado, sua refatoração apresenta um enorme risco de introdução de regressões.

Dependendo da perspectiva que você deseja incentivar, você pode estar inclinado a aconselhar os revisores a refatorar mais ou não. Em todos os casos, não espere um código limpo e impecável, com arquitetura e design agradáveis ​​dentro de uma base de código ruim. O que você não deve incentivar é o comportamento em que um desenvolvedor experiente que precisa trabalhar em uma base de código ruim tenta fazer sua parte bem. Em vez de simplificar as coisas, apenas as torna mais complicadas do que antes. Agora, em vez de um código incorreto e uniforme, você tem uma parte com padrões de design, outra parte com código limpo e claro, outra parte que foi extensivamente refatorada ao longo do tempo e nenhuma unidade.

Imagine, por exemplo, que você está descobrindo uma base de código herdada de um site de tamanho médio. Você fica surpreso com a falta de qualquer estrutura usual e com o fato de que o registro, quando concluído, é anexado manualmente a um arquivo de texto, em vez de usar uma estrutura de registro. Você decide que o novo recurso use o MVC e uma estrutura de log.

Seu colega está implementando outro recurso e fica muito surpreso com a falta de um ORM em que o tamanho seria perfeito. Então ele começa a usar um ORM.

Nem você nem seu colega são capazes de passar por centenas de milhares de linhas de código para fazer uso do MVC ou de uma estrutura de log ou de um ORM em qualquer lugar. Na verdade, isso exigiria meses de trabalho: imagine introduzir o MVC; quanto tempo levaria? Ou o que dizer de um ORM em situações em que as consultas SQL eram caoticamente geradas por meio de concatenação (com locais ocasionais para injeção de SQL) dentro do código que ninguém conseguia entender?

Você acha que fez um ótimo trabalho, mas agora, um novo desenvolvedor que ingressa no projeto precisa enfrentar muito mais complexidade do que antes:

• A maneira antiga de tratar os pedidos,

• O caminho MVC,

• O antigo mecanismo de registro,

• A estrutura de log,

• O acesso direto ao banco de dados com consultas SQL criadas dinamicamente,

• O ORM.

Em um projeto em que eu estava trabalhando, havia quatro (!) Estruturas de log usadas lado a lado (além de log manual). O motivo é que toda vez que alguém queria registrar coisas, não havia uma abordagem comum para fazê-lo; portanto, em vez de aprender uma nova estrutura (que em todos os casos era usada apenas em 5% da base de código), basta adicionar outra que ele já sabe. Imagine a bagunça.

Uma abordagem melhor seria refatorar a base de código um passo de cada vez. Tomando novamente o exemplo de log, a refatoração consistiria nas seguintes pequenas etapas:

• Encontre todos os locais onde o log herdado é feito (ou seja, quando o arquivo de log é acessado diretamente) e verifique se todos chamam os mesmos métodos.

• Mova esse código para uma biblioteca dedicada, se aplicável. Não quero logar lógica de armazenamento na minha classe de carrinho de compras.

• Modifique, se necessário, a interface dos métodos de log. Por exemplo, podemos adicionar um nível indicando se a mensagem é informal ou se é um aviso ou um erro.

• Use os métodos recentemente refatorados no novo recurso.

• Migrar para a estrutura de log: o único código afetado é o código na biblioteca dedicada.

Se você estiver revisando códigos como parte do seu processo de desenvolvimento; então você deve definir as regras contra as quais 'julga' o código que está revisando.

Isso deve entrar na sua 'definição de pronto' e pode ser um guia de estilo, documento de arquitetura para a base de código ou análise estática, verificação de requisitos legais, o que a empresa decidir que são os requisitos de 'qualidade do código'

Depois que você fizer isso, as revisões de código se tornarão uma questão de curso, o guia de estilo foi seguido, temos a porcentagem necessária de cobertura de teste, etc.

Se você não tiver isso em mente, as revisões de código podem se tornar apenas uma batalha de quem possui a maior codificação ou, como na sua situação, questionamentos de julgamento chamam de refatoração versus recursos feitos antes do prazo. O que é apenas uma perda de tempo de todos.

Seu principal problema é que uma revisão de código em um novo recurso significativo é a hora errada para ter essa discussão. Nesse ponto, é tarde demais para fazer qualquer coisa, exceto pequenas alterações. O lugar certo está nos estágios de planejamento ou em uma revisão preliminar do projeto, o mais tardar. Se sua empresa não estiver fazendo essas primeiras análises pelo menos informalmente, você deve primeiro mudar essa cultura.

O próximo passo é ser convidado para essas reuniões e ter idéias produtivas nessas reuniões. Principalmente, isso significa não tentar mudar tudo da noite para o dia, mas procurar por pedaços pequenos que você pode isolar e enfrentar. Esses pedaços aumentarão significativamente ao longo do tempo.

Em outras palavras, a chave é sugerir regularmente mudanças menores no início dos projetos, em vez de ser derrubado, sugerindo mudanças maiores no final.

Nos lugares em que tenho revisado o código, eu aceitaria e aceitaria o envio do código, se vier com o compromisso de fazer (pelo menos) alguma refatoração. Seja como um bug arquivado, como uma história ou como uma promessa de enviar outra revisão com (algumas) refatorações feitas.

Nesses casos, se eu sou a pessoa que está escrevendo o código para revisar, geralmente preparo duas alterações, uma com meu (s) novo (s) recurso (s) ou correções de bugs, depois outra que tem essa E uma limpeza. Dessa forma, as alterações na limpeza não prejudicam o (s) novo (s) recurso (s) ou correções de bugs, mas são facilmente apontadas como um token "sim, eu sei que isso não corrige esses problemas, mas existe" pura limpeza "que faz".