O que fazer quando o código enviado para revisão de código parece ser muito complicado?

Se não puder ser revisado, ele não poderá passar na revisão.

Você precisa entender que a revisão de código não é para encontrar bugs. É para isso que o QA é. A revisão de código é para garantir que a manutenção futura do código seja possível. Se você não pode nem mesmo seguir o código agora, como você pode em seis meses, quando você está designado para fazer melhorias de recursos e / ou correções de bugs? Encontrar bugs agora é apenas um benefício colateral.

Se for muito complexo, estará violando uma tonelada de princípios do SOLID . Refatorar, refatorar, refatorar. Divida-o em funções apropriadamente nomeadas, que são muito menos simples. Você pode limpá-lo e seus casos de teste garantirão que ele continue funcionando corretamente. Você tem casos de teste, certo? Se não, você deve começar a adicioná-los.

Tudo o que você mencionou é perfeitamente válido para apontar em uma revisão de código.

Quando recebo uma revisão de código, reviso os testes. Se os testes não fornecem cobertura suficiente, isso é algo a se destacar. Os testes precisam ser úteis para garantir que o código funcione conforme pretendido e continuará a funcionar conforme pretendido nas alterações. Na verdade, esta é uma das primeiras coisas que procuro em uma revisão de código. Se você ainda não provou que seu código atende aos requisitos, não quero investir meu tempo em analisá-lo.

Uma vez que existem testes suficientes para o código, se o código é complexo ou difícil de seguir, também é algo que os humanos deveriam estar observando. As ferramentas de análise estática podem apontar algumas medidas de complexidade e sinalizar métodos excessivamente complexos, bem como encontrar possíveis falhas no código (e devem ser executadas antes de uma revisão de código humano). Mas o código é lido e mantido por humanos e precisa ser escrito para manutenção primeiro. Somente se houver um motivo para usar código menos passível de manutenção, deve ser escrito dessa maneira. Se você precisa ter um código complexo ou não intuitivo, ele deve ser documentado (de preferência no código) porque o código é assim e tem comentários úteis para futuros desenvolvedores entenderem por que e o que o código está fazendo.

O ideal é rejeitar revisões de código que não tenham testes apropriados ou que tenham códigos excessivamente complexos sem um bom motivo. Pode haver razões comerciais para seguir em frente e, para isso, é necessário avaliar os riscos. Se você avançar com a dívida técnica em código, coloque os tickets em seu sistema de acompanhamento de bugs imediatamente, com alguns detalhes do que precisa ser alterado e algumas sugestões para alterá-lo.

Verifying overall correctness manually via code review however is very difficult and time-consuming, if it is even possible at all.

Isso não é remotamente o objetivo de uma revisão de código. A maneira de pensar em uma revisão de código é imaginar que há um bug no código e você precisa corrigi-lo. Com essa mentalidade, navegue pelo código (especialmente comentários) e pergunte a si mesmo: "É fácil entender o quadro geral do que está acontecendo para que eu possa diminuir o problema?" Se sim, é um passe. Caso contrário, é uma falha. Mais documentação é necessária no mínimo, ou possivelmente a refatoração é necessária para tornar o código razoavelmente compreensível.

É importante não ser perfeccionista, a menos que tenha certeza de que é o que seu empregador está procurando. A maioria dos códigos é tão ruim que pode ser facilmente refatorada 10 vezes seguidas, ficando cada vez mais legível. Mas o seu empregador provavelmente não quer pagar para ter o código mais legível do mundo.

Verifying overall correctness manually via code review however is very difficult and time-consuming, if it is even possible at all.

Muitos anos atrás, era meu trabalho fazer exatamente isso, classificando os trabalhos de casa dos alunos. E enquanto muitos entregaram alguma qualidade razoável com um erro aqui e ali, havia dois que se destacaram. Ambos sempre enviavam código sem erros. Um código enviado que eu poderia ler de cima e de baixo em alta velocidade e marcar como 100% correto com esforço zero. O outro código apresentado era um WTF após o outro, mas de alguma forma conseguiu evitar quaisquer erros. Uma dor absoluta para marcar.

Hoje, o segundo teria seu código rejeitado em uma revisão de código. Se a verificação da exatidão é muito difícil e demorada, isso é um problema com o código. Um programador decente descobriria como resolver um problema (leva tempo X) e antes de dar a um revisor de código refatorará para que ele não faça apenas o trabalho, mas obviamente faz o trabalho. Isso leva muito menos que X no tempo e economiza muito tempo no futuro. Muitas vezes, descobrindo bugs antes mesmo de irem para o estágio de uma revisão de código. Em seguida, tornando a revisão de código muito mais rápida. E todo o tempo no futuro, tornando o código mais fácil de se adaptar.

Outra resposta dizia que o código de algumas pessoas poderia ser refatorado 10 vezes, tornando-se mais legível a cada vez. Isso é apenas triste. Esse é um desenvolvedor que deve procurar um trabalho diferente.

Este código antigo foi ligeiramente alterado? (100 linhas de código alteradas em uma base de código de 10000 linhas ainda é uma pequena alteração) Às vezes há restrições de tempo e os desenvolvedores são forçados a permanecer dentro de uma estrutura antiga e inconveniente, simplesmente porque uma reescrita completa demoraria ainda mais e estaria fora do orçamento . + geralmente há risco envolvido, que pode custar milhões de dólares quando avaliado incorretamente. Se for um código antigo, na maioria dos casos você terá que conviver com isso. Se você não entende por si mesmo, converse com eles e ouça o que eles dizem, tente entender. Lembre-se, pode ser difícil de seguir para você, mas perfeitamente bem para outras pessoas. Tome o seu lado, veja do seu fim.

Este novo código ? Dependendo das restrições de tempo, você deve defender refatorar o máximo possível. Não há problema em gastar mais tempo em revisões de código, se necessário. Você não deve timebox para 15min, ter a idéia e seguir em frente. Se o autor gastou uma semana para escrever algo, não há problema em gastar de 4 a 8 horas para revisá-lo. Seu objetivo aqui é ajudá-los a refatorar. Você não retorna o código dizendo "refatorar. Agora". Veja quais métodos podem ser divididos, tente criar ideias para introduzir novas classes, etc.

Muitas vezes, os patches / changelists "complicados" são aqueles que fazem muitas coisas diferentes ao mesmo tempo. Há novo código, código excluído, código refatorado, código movido, testes expandidos; torna difícil ver o quadro geral.

Uma pista comum é que o patch é enorme, mas sua descrição é pequena: "Implemente $ FOO".

Uma maneira razoável de lidar com esse tipo de patch é pedir que ele seja dividido em uma série de partes menores e independentes. Assim como o princípio da responsabilidade única diz que uma função deve fazer apenas uma coisa, uma correção também deve se concentrar em apenas uma coisa.

Por exemplo, os primeiros patches podem conter refatorações puramente mecânicas que não fazem alterações funcionais e, em seguida, os patches finais podem se concentrar na implementação e nos testes reais de $ FOO com menos distrações e pistas falsas.

Para funcionalidade que requer muitos códigos novos, o novo código pode ser introduzido em partes testáveis que não alteram o comportamento do produto até que o último patch da série chame o novo código (um flag flip).

Quanto a isso com tato, costumo dizer que o problema é meu e depois peço a ajuda do autor: "Estou tendo problemas para acompanhar tudo o que está acontecendo aqui. Você poderia dividir esse patch em etapas menores para me ajudar a entender como isso tudo se encaixa? " Às vezes é necessário fazer sugestões específicas para as etapas menores.

Um patch tão grande como "Implement $ FOO" se transforma em uma série de patches como:

1. Apresente uma nova versão do Frobnicate que usa um par de iteradores porque eu precisarei chamá-lo com sequências diferentes de vetor para implementar $ FOO.
2. Altere todos os chamadores existentes do Frobnicate para usar a nova versão.
3. Exclua o antigo Frobnicate.
4. Frobnicate estava fazendo muito. Fatore o refrumple em seu próprio método e adicione testes para isso.
5. Introduza o Zerzify, com testes. Não usado ainda, mas vou precisar dele por $ FOO.
6. Implemente $ FOO em termos de Zerzify e o novo Frobnicate.

Observe que as etapas de 1 a 5 não fazem alterações funcionais no produto. Eles são triviais para revisar, incluindo garantir que você tenha todos os testes corretos. Mesmo que o passo 6 ainda seja "complicado", pelo menos ele é focado em $ FOO. E o log naturalmente dá uma idéia muito melhor de como $ FOO foi implementado (e porque Frobnicate foi alterado).

Como outros apontaram, a revisão de código não é realmente projetada para encontrar bugs. Se você estiver encontrando bugs durante a revisão de código, isso provavelmente significa que você não tem cobertura de teste automatizada suficiente (por exemplo, testes de unidade / integração). Se não houver cobertura suficiente para me convencer de que o código faz o que supostamente deveria, eu normalmente peço mais testes e aponto o tipo de casos de teste que estou procurando e geralmente não permito código no codebase que não tem cobertura adequada.

Se a arquitetura de alto nível é muito complexa ou não faz sentido, geralmente convoco uma reunião com alguns membros da equipe para falar sobre isso. Às vezes é difícil iterar em uma arquitetura ruim. Se o desenvolvedor era um novato, eu geralmente me certifico de que o pensamento deles está à frente do tempo, em vez de reagir a um pedido ruim. Isso geralmente é verdade mesmo com desenvolvedores mais experientes, se o problema não tiver uma solução óbvia que provavelmente será escolhida.

Se a complexidade for isolada ao nível do método que geralmente pode ser corrigido iterativamente e com bons testes automatizados.

Um último ponto. Como revisor, você precisa decidir se a complexidade do código se deve a essencial ou acidental complexidade . Complexidade essencial refere-se às partes do software que são legitimamente difíceis de resolver. Complexidade acidental refere-se a todas as outras partes do código que escrevemos, que é muito complexo sem motivo e pode ser facilmente simplificado.

Eu geralmente me certifico de que o código com complexidade essencial é realmente isso e não pode ser simplificado ainda mais. Eu também viso mais cobertura de teste e boa documentação para essas partes. A complexidade acidental deve quase sempre ser eliminada durante o processo de solicitação pull, porque essas são a maior parte do código com o qual lidamos e podem facilmente causar pesadelos de manutenção, mesmo a curto prazo.

Como são os testes? Eles devem ser claros, simples e fáceis de ler, idealmente com apenas uma afirmação. Os testes devem documentar claramente o comportamento pretendido e casos de uso do código.

Se não for bem testado, é um bom local para começar a analisar.