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

115

O código é difícil de seguir, mas parece estar (principalmente) funcionando bem, pelo menos com testes superficiais. Pode haver pequenos erros aqui e ali, mas é muito difícil saber lendo o código se eles são sintomáticos de problemas mais profundos ou correções simples. Verificar manualmente a correção geral manualmente por meio da revisão de código é muito difícil e demorado, se é que isso é possível.

Qual é o melhor curso de ação nesta situação? Insistir em um recomeço? Recondicionamento parcial? Re-fatoração primeiro? Corrigir apenas os bugs e aceitar a dívida técnica ? Faça uma avaliação de risco sobre essas opções e decida? Algo mais?

Brad Thomas
fonte
4
Aparece ou é? Uma medição rápida dos arquivos de origem confirmará ou negará suas suspeitas. Após a medição, basta exibir os números de medição na revisão de código e sugerir uma refatoração para reduzir os números de complexidade.
Jon Raynor
4
Por favor, defina "muito complicado". O código é muito complicado porque usa padrões de design conhecidos que são simplesmente desconhecidos para sua equipe ou porque falha em usar padrões bem conhecidos para sua equipe? As razões precisas para julgar o código "muito complicado" são essenciais para criar uma avaliação correta de como avançar. Uma declaração tão simples quanto "muito complicada" em uma área de conhecimento tão profunda e complicada quanto a revisão de código sugere uma caça às bruxas do desenvolvedor para mim.
Pieter Geerkens
7
@PieterGeerkens Ou, talvez, seja muito complicado porque está resolvendo um problema complicado?
Casey #

Respostas:

251

Se não puder ser revisado, 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 serve o controle de qualidade. A revisão do código é para garantir que a manutenção futura do código seja possível. Se você não consegue seguir o código agora, como pode, em seis meses, quando é designado para executar aprimoramentos de recursos e / ou correções de bugs? Encontrar bugs agora é apenas um benefício colateral.

Se for muito complexo, está violando vários princípios do SOLID . Refatorar, refatorar, refatorar. Divida-o em funções nomeadas corretamente, que fazem muito menos, mais simples. Você pode limpá-lo e seus casos de teste garantirão que ele continue funcionando corretamente. Você tem casos de teste, certo? Caso contrário, você deve começar a adicioná-los.

Kevin Fee
fonte
49
Isso muito. Lembre-se de que não é apenas você e o escritor que irão ler este código. Também será um estagiário aleatório em 10 anos, então você quer ter certeza de que ele tem a chance de entender o que está acontecendo.
David Grinberg
2
boa resposta. isso depende do objetivo da "revisão de código". legibilidade é uma coisa, estrutura outra - mas elas estão intimamente relacionadas. fwiw Estou trabalhando com um código-fonte aberto escrito pelo MAJOR corps, e é quase ilegível porque os nomes var e fn são muito fáceis de entender.
19
@DavidGrinberg Para todos os fins práticos, "você em seis meses" é uma pessoa completamente diferente.
chrylis -on strike-
2
Guarde o código por algum tempo (tempo suficiente para ele não se lembrar de tudo). Peça ao codificador original para revisá-lo. Veja se ele entende isso.
19416 Nelson
4
Eu discordo que a revisão de código "não é" para encontrar bugs. Ele geralmente encontra bugs, e esse é um aspecto muito poderoso e útil das revisões de código. Melhor ainda, ajuda a encontrar maneiras de evitar bugs inteiramente no código futuro. O ponto talvez seja exagerado, e deve ser que não seja exclusivamente para encontrar bugs!
Cody Grey
45

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

Quando recebo uma revisão de código, analiso os testes. Se os testes não oferecem cobertura suficiente, é algo a destacar. Os testes precisam ser úteis para garantir que o código funcione conforme o planejado e continue a funcionar conforme o planejado nas alterações. De fato, essa é uma das primeiras coisas que procuro em uma revisão de código. Se você 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, isso também é algo que os humanos devem observar. As ferramentas de análise estática podem apontar algumas medidas de complexidade e sinalizar métodos excessivamente complexos, além de encontrar possíveis falhas no código (e devem ser executadas antes de uma revisão do código humano). Mas o código é lido e mantido por humanos, e precisa ser escrito primeiro para manutenção. Somente se houver um motivo para usar um código menos sustentável, ele deve ser escrito dessa maneira. Se você precisar de um código complexo ou não intuitivo, ele deve ser documentado (de preferência no código) por que o código é dessa maneira e ter comentários úteis para futuros desenvolvedores entenderem o porquê e o que o código está fazendo.

Idealmente, rejeite revisões de código que não tenham testes apropriados ou que tenham código excessivamente complexo sem um bom motivo. Pode haver razões comerciais para avançar e, para isso, você precisará avaliar os riscos. Se você continuar com dívidas técnicas em código, coloque tickets no seu sistema de rastreamento de bugs imediatamente com alguns detalhes do que precisa ser alterado e algumas sugestões para alterá-lo.

Thomas Owens
fonte
30

Verificar manualmente a correção geral manualmente por meio da revisão de código é muito difícil e demorado, se é que isso é possível.

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 erro 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 panorama geral do que está acontecendo para que eu possa diminuir o problema?" Se assim for, é um passe. Caso contrário, é um fracasso. É necessária mais documentação, no mínimo, ou possivelmente refatoração para tornar o código razoavelmente compreensível.

É importante não ser perfeccionista a menos que tenha certeza de que é isso que o seu empregador procura. A maioria dos códigos é uma droga, tanto que poderia ser refatorada facilmente 10 vezes seguidas, ficando mais legível a cada vez. Mas seu empregador provavelmente não quer pagar para ter o código mais legível do mundo.

DepressedDaniel
fonte
4
Ótimo comentário! "A maioria dos códigos é tão chata que pode ser facilmente refatorada 10 vezes seguidas, ficando mais legível a cada vez" Garoto, eu tenho sido culpado de fazer isso :) #
597 Dean Radcliffe
1
"A maioria dos códigos é tão ruim que pode ser refatorada facilmente 10 vezes seguidas, ficando mais legível a cada vez." De fato, é assim que é no mundo real.
Peter Mortensen
@ PeterMortensen É realmente verdade que você encontra muito disso no mundo real. Mas não é do interesse de ninguém ter o código escrito dessa maneira. Eu acho que há duas razões pelas quais é assim. A educação que os desenvolvedores recebem se esforça muito pouco para ensinar como escrever código legível. E em algumas empresas isso é percebido como uma perda de tempo: "Se o desenvolvedor já escreveu um código de trabalho, por que devemos nos preocupar se é legível ou não? Basta enviar a coisa".
kasperd
15

Verificar manualmente a correção geral manualmente por meio da revisão de código é muito difícil e demorado, se é que isso é possível.

Muitos anos atrás, era realmente meu trabalho fazer exatamente isso classificando o dever de casa dos alunos. E enquanto muitos entregavam uma qualidade razoável com um bug aqui e ali, havia dois que se destacavam. Ambos sempre enviaram código que não apresentava bugs. Um código enviado que eu pude ler de cima e de baixo em alta velocidade e marcar como 100% correto com zero esforço. O outro código enviado foi um WTF após o outro, mas de alguma forma conseguiu evitar erros. Uma dor absoluta para marcar.

Hoje, o segundo código teria seu código rejeitado em uma revisão de código. Se a verificação da correção for 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 submetê-lo a uma revisão de código refatorá-lo para que ele não apenas faça o trabalho, mas obviamente o faça. Isso leva muito menos que X no tempo e economiza muito tempo no futuro. Muitas vezes, descobrindo bugs antes que eles cheguem ao estágio de uma revisão de código. Em seguida, faça o código revisar muito mais rápido. E o tempo todo no futuro, facilitando a adaptação do código.

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

gnasher729
fonte
Leva muito menos tempo para refatorar meu código 10 vezes, e preciso escrever a primeira versão do código. Se alguém mais souber que eu fiz essa refatoração, falhei.
19416 Ian
6

Esse código antigo foi alterado ligeiramente? (100 linhas de código alteradas em uma base de código de 10000 linhas ainda são 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 levaria ainda mais tempo e está 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 ele. Se você não entender por conta própria, converse com eles e ouça o que eles dizem, tente entender. Lembre-se, pode ser difícil segui-lo, mas perfeitamente adequado para outras pessoas. Fique do lado deles, veja do fim deles.

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

Neolisk
fonte
2
Você não retorna o código dizendo "refatorar. Agora" - por quê? Recebi esses comentários de revisão pelo menos uma vez e da última vez que lembro que eles foram úteis e corretos. Eu tive que reescrever grande parte do código a partir do zero e isso foi a coisa certa a fazer, porque, olhando para trás, eu mesmo percebi que o código antigo era uma bagunça incontestável. Avaliador era simplesmente o suficiente qualificados para notar que (e eu aparentemente não era)
mosquito
4
@gnat: Por um lado, porque é rude. Você fica melhor quando explica o que há de errado com o código e se esforça para ajudar a outra pessoa a melhorá-lo. Em uma grande empresa, fazê-lo de outra forma pode tirá-lo da porta rapidamente. Especialmente se você estiver revendo o código de uma pessoa mais experiente.
Neolisk
Nesse caso, como mencionei acima, foi em uma grande empresa estabelecida que teve o cuidado de não sair da porta dos desenvolvedores mais qualificados, pelo menos não com o objetivo de compartilhar diretamente seus conhecimentos técnicos quando solicitados a
gnat
1
@gnat: A abordagem "refatorar. agora" pode funcionar em baixa, ou seja, quando um desenvolvedor sênior com mais de 10 anos de experiência diz refatorar para um desenvolvedor júnior que foi contratado há um mês sem experiência ou situação semelhante. Para cima - você pode ter um problema. Como você pode não saber quanta experiência o outro desenvolvedor possui, é seguro assumir o respeito como comportamento padrão. Não vai machucá-lo com certeza.
Neolisk
1
@ Neolisk: Um desenvolvedor experiente que teve que escrever código sob pressão do tempo e sabe que não é bom o suficiente pode ficar muito feliz se você rejeitar o código, dando a ele tempo e uma desculpa para melhorá-lo. O PHB decidindo que é bom o suficiente deixa o desenvolvedor infeliz; o revisor que decide que não é bom o suficiente o faz feliz.
gnasher729
2

Muitas vezes, patches / changelists "complicados" são aqueles que fazem muitas coisas diferentes ao mesmo tempo. Há um novo código, código excluído, código refatorado, código movido, testes expandidos; dificulta a visão geral.

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

Uma maneira razoável de lidar com esse patch é pedir que ele seja dividido em uma série de pedaços menores e independentes. Assim como o princípio de responsabilidade única diz que uma função deve fazer apenas uma coisa, um patch deve se concentrar apenas em 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 do $ FOO com menos distrações e arenques vermelhos.

Para funcionalidades que exigem muito código novo, o novo código geralmente pode ser introduzido em partes testáveis ​​que não alteram o comportamento do produto até que o último patch da série realmente chame o novo código (um flag flag).

Quanto a fazer isso com tato, costumo dizer que é meu problema 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 tudo isso se encaixa? juntos?" Às vezes é necessário fazer sugestões específicas para as etapas menores.

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 precisarei chamá-la com sequências diferentes de vetor para implementar $ FOO.
  2. Alterne todos os chamadores existentes do Frobnicate para usar a nova versão.
  3. Exclua o antigo Frobnicate.
  4. Frobnicate estava fazendo muito. Fatore a etapa de reformulação em seu próprio método e adicione testes para isso.
  5. Apresente o Zerzify, com testes. Ainda não usado, mas vou precisar por US $ 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 fáceis de revisar, incluindo garantir que você tenha todos os testes certos. Mesmo que a etapa 6 ainda seja "complicada", pelo menos está focada em $ FOO. E o log naturalmente dá uma idéia muito melhor de como o $ FOO foi implementado (e por que o Frobnicate foi alterado).

Adrian McCarthy
fonte
Uma abordagem, se estiver usando o Git, é compor uma solicitação de recebimento de várias confirmações. Cada confirmação é o mais atômico e independente possível e tem sua própria descrição. Em seguida, adicione uma observação útil no corpo do PR que cada alteração pode ser revisada manualmente. Geralmente, é assim que lida com PRs muito grandes, como refatores globais ou grandes mudanças de ferramentas não disponíveis.
Jimmy Breck-McKye
1

Como outros apontaram, a revisão de código não é realmente projetada para encontrar bugs. Se você encontrar erros durante a revisão do código, isso provavelmente significa que você não possui 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 deveria , geralmente peço mais testes e aponto o tipo de casos de teste que estou procurando e geralmente não permito código na base de código que não possui cobertura adequada .

Se a arquitetura de alto nível é muito complexa ou não faz sentido, costumo convocar 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 novato, normalmente asseguro-me de analisar antecipadamente o que pensam deles, em vez de reagir a uma solicitação de pull ruim. Isso geralmente acontece mesmo com desenvolvedores mais experientes, se o problema não tiver uma solução óbvia que provavelmente será escolhida.

Se a complexidade estiver isolada no nível do método, geralmente isso pode ser corrigido iterativamente e com bons testes automatizados.

Um último ponto. Como revisor, você precisa decidir se a complexidade do código é devido à complexidade essencial ou acidental . A complexidade essencial está relacionada à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 são muito complexas sem motivo e podem ser facilmente simplificadas.

Normalmente, garanto que o código com complexidade essencial é realmente esse e não pode ser mais simplificado. Também pretendo obter mais cobertura de teste e boa documentação para essas peças. A complexidade acidental quase sempre deve ser limpa durante o processo de solicitação de recebimento, porque esses são a maior parte do código com o qual lidamos e podem facilmente causar pesadelo na manutenção, mesmo a curto prazo.

c_maker
fonte
0

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

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

Tom Squires
fonte