O que você diz em uma revisão de código quando a outra pessoa criou uma solução complicada? [fechadas]

37

No outro dia, analisei o código que alguém da minha equipe escreveu. A solução não estava totalmente funcional e o design era muito complicado - o que significava armazenar informações desnecessárias, construir recursos desnecessários e, basicamente, o código tinha muita complexidade desnecessária, como o gold plating, e tentava resolver problemas que não existiam.

Nesta situação eu pergunto "por que isso foi feito dessa maneira?"

A resposta é que a outra pessoa sentiu vontade de fazer assim.

Depois, pergunto se algum desses recursos fazia parte da especificação do projeto ou se eles têm alguma utilidade para o usuário final ou se algum dado extra seria apresentado ao usuário final.

A resposta é não.

Então sugiro que ele exclua toda a complexidade desnecessária. A resposta que geralmente recebo é "bem, já está feito".

A minha opinião é que não é feito, é buggy, não faz o que os usuários querem, e o custo de manutenção será maior do que se fosse feito da maneira mais simples que eu sugeri.

Um cenário equivalente é:
O colega gasta 8 horas de código de refatoração à mão, o que poderia ter sido feito automaticamente no Resharper em 10 segundos. Naturalmente, não confio na refatoração à mão, pois ela é de qualidade duvidosa e não totalmente testada.
Mais uma vez a resposta que recebo é "bem, já está feito".

Qual é a resposta apropriada para essa atitude?

    
por dan 09.12.2016 / 11:02
fonte

19 respostas

25

Mentalidade / atitude

  • Liderar pelo exemplo
  • Admita em particular (um para um, fora da revisão do código)
  • Incentive uma mentalidade de manter-se simples entre os membros da equipe

Gerenciamento de equipe

  • Gaste mais tempo com a especificação de um item de trabalho (como arquitetura, esquema de algoritmo, estrutura de interface de usuário, etc)
  • Incentive os membros da equipe a buscar esclarecimentos sobre o escopo de um item de trabalho
  • Incentive os membros da equipe a discutir maneiras de implementar um item de trabalho
  • Faça estimativas razoáveis para cada item de trabalho antes de começar e faça o melhor esforço para atendê-las
  • Monitore a "melhoria" dos membros da equipe.
    • Depois de ser admoestado ou de ser mostrado o caminho certo para fazer as coisas, veja se o membro da equipe melhora.

Nível de habilidade

  • Aloque algum tempo para sessões de programação de pares ou sessões de treinamento individuais para fazer o melhor uso das ferramentas do desenvolvedor (refatoração, revisão de código)

Gerenciamento de projetos (risco)

  • Conduza a revisão de código com mais frequência, de forma assíncrona (Nota)
    • Nota sobre "de forma assíncrona"
      • O revisor de código deve receber notificações / convites para rever as alterações assim que forem confirmadas
      • O revisor de código deve ter a chance de revisar o código antes de qualquer reunião com o desenvolvedor.
      • Se for necessário esclarecer o desenvolvedor, faça-o informalmente em mensagens instantâneas / e-mail sem emitir uma opinião negativa
por 18.07.2012 / 09:10
fonte
69

What do you say in a code review when the other person built an over complicated solution?

Você diz: "você criou uma solução excessivamente complicada".

So then I suggest that he delete all the unnecessary complexity. The answer I usually get is "well it's already done."

Se for tarde demais para mudar alguma coisa, por que você está fazendo uma revisão de código?

    
por 15.08.2017 / 10:57
fonte
16

"Já está feito" não é uma resposta satisfatória. Feito significa testado e funcionando. Todo código extra que não está fazendo nada útil deve ser mantido da maneira correta (deletada).

Atribua-o novamente a tarefa de refatorar e otimizar sua solução. Se ele não fizer isso, atribua-lhe um par de programadores e espere que ele aprenda algo com o colega.

    
por 18.07.2012 / 10:59
fonte
8

So then I suggest that he delete all the unnecessary complexity. The answer I usually get is "well it's already done".

Essa não é uma resposta aceitável:

  • Se é realmente tarde demais para mudar, então a revisão de código é um desperdício de tempo, e o gerenciamento precisa saber disso.

  • Se isso é realmente uma maneira de dizer "eu não quero mudar", então você precisa assumir a posição de que a complexidade extra é ruim para a base de código, porque os problemas / custo que vai incorrer mais tarde. E reduzindo o potencial para problemas futuros, a verdadeira razão pela qual você está fazendo a revisão do código, em primeiro lugar.

E ...

... the solution wasn't fully functional ...

Isso é possivelmente um resultado direto da complexidade desnecessária. O programador tornou-o tão complexo que já não o compreende completamente e / ou perdeu o seu tempo a implementar a sua complexidade em vez dos pontos de função. Vale a pena salientar ao programador que cortar a complexidade pode levá-lo a um programa de trabalho mais rápido.

Agora, soa como você não tem o poder (ou talvez a confiança) para "empurrar de volta" para isso. Mas, mesmo assim, vale a pena fazer um pouco de barulho sobre isso (sem personalizá-lo) na esperança de que o codificador ofensor faça um trabalho melhor ... da próxima vez.

What is an appropriate response to this attitude?

Por fim, chame a atenção da gerência ... a menos que você tenha o poder de consertá-la por conta própria. (Claro, isso não vai te tornar popular.)

    
por 18.07.2012 / 12:07
fonte
7

Você estava certo, eles estavam errados:

What is an appropriate response to this attitude?

Faça a revisão correta do código. Se eles se recusarem a implementar as mudanças sugeridas sem um motivo, pare de desperdiçar seu tempo com uma revisão de código. Você também pode escalar o problema para o chefe .

    
por 18.07.2012 / 10:29
fonte
5

Uma ação que nossa equipe levou, que melhorou dramaticamente a situação em tais casos, foi a mudança para muito changesets menores .

Em vez de trabalhar em uma tarefa por um dia ou mais e, em seguida, ter uma revisão de código (grande), tentamos fazer o check-in com muito mais frequência (até 10 vezes por dia). Claro que isto também tem alguns inconvenientes, e. o revisor precisa ser muito responsivo, o que diminui sua própria saída (devido a interrupções frequentes).

A vantagem é que os problemas são detectados e podem ser resolvidos cedo, antes que uma grande quantidade de trabalho seja feita da maneira errada.

    
por 18.07.2012 / 10:18
fonte
2

Você deve se concentrar na causa raiz do problema:

  1. A educação dos programadores se concentra no aumento da complexidade dada aos programadores. A capacidade de fazer isso foi testada pela escola. Assim, muitos programadores pensarão que, se implementarem uma solução simples, não fizeram o trabalho corretamente.
  2. Se o programador segue o mesmo padrão que ele fez centenas de vezes na universidade, é exatamente como os programadores estão pensando - mais complexidade é mais desafiadora e, portanto, melhor.
  3. Para corrigir isso, você precisará manter uma separação rigorosa dos requisitos de sua empresa em relação à complexidade em comparação com o que normalmente é exigido na educação do programador. Um bom plano é uma regra como "o nível mais alto de complexidade deve ser reservado apenas para tarefas projetadas para melhorar suas habilidades - e não deve ser usado no código de produção".
  4. Será uma surpresa para muitos programadores que eles não podem executar seus projetos mais loucos no ambiente de código de produção importante. Apenas reserve tempo para os programadores fazerem projetos experimentais e, em seguida, mantenha toda a complexidade nesse lado da cerca.

(na revisão de código já é tarde demais para mudar isso)

    
por 18.07.2012 / 11:16
fonte
2

Eu não sei de nada que funcione depois que o código é escrito.

Antes de o código ser escrito, as pessoas podem discutir formas alternativas de o fazer. A chave é contribuir com ideias uns para os outros, por isso esperamos que um razoável seja escolhido.

Existe outra abordagem que trabalha com contratados - contratos de preço fixo. Quanto mais simples a solução, mais $$ o programador consegue manter.

    
por 18.07.2012 / 16:31
fonte
1

Você não pode consertar o mundo.

Você não pode consertar todo o código do seu projeto. Você provavelmente não pode corrigir as práticas de desenvolvimento em seu projeto, pelo menos não este mês.

Infelizmente, o que você está enfrentando na revisão de código é muito comum. Eu trabalhei em algumas organizações onde encontrei muitas vezes revisando 100 linhas de código que poderiam ter sido escritas em dez, e recebi a mesma resposta que você fez: "Já está escrito e testado" ou "Estamos procurando bugs, não um redesenho ".

É um fato que alguns de seus colegas não podem programar tão bem quanto você pode. Alguns deles podem ser muito ruins nisso. Não se preocupe com isso. Um par de classes com más implementações não vai derrubar o projeto. Em vez disso, concentre-se nas partes do trabalho que afetarão os outros. Os testes da unidade são adequados (se você os tiver)? A interface é utilizável? Está documentado?

Se a interface para o código ruim estiver ok, não se preocupe com isso até que você tenha que mantê-lo, então reescreva-o. Se alguém reclamar, apenas chame de refatoração. Se eles ainda reclamarem, procure uma posição em uma organização mais sofisticada.

    
por 18.07.2012 / 17:58
fonte
0

Deve haver uma política padrão no projeto que controle os procedimentos de verificação de qualidade e as ferramentas usadas.

As pessoas devem saber o que devem fazer e quais ferramentas são aceitas para uso neste projeto.

Se você ainda não fez isso, organize seus pensamentos e faça isso.

A revisão de código deve ter uma lista de verificação de itens padrão. Se você receber "já está feito" e não é, então, pessoalmente, eu não gostaria de ser responsável pelo trabalho deste desenvolvedor como gerente de projeto ou desenvolvedor sênior. Essa atitude não deve ser tolerada. Eu posso entender argumentar sobre como fazer algo ou mesmo tudo, mas uma vez que uma solução é aceita, a mentira não deve ser tolerada e isso deve ser claramente declarado.

    
por 18.07.2012 / 09:37
fonte
0

Sua loja precisa aplicar algumas metodologias de design.

  • Os requisitos precisam ser definidos com clareza.
  • Você precisa desenvolver casos de uso que suportem os requisitos.
  • Você precisa especificar as funções necessárias para implementar os casos de uso.
  • Você precisa especificar quaisquer requisitos não funcionais (tempos de resposta, disponibilidade etc.)
  • Você precisa de um RTM (Requiements Tracabilty Matrix) para mapear cada função do sistema de volta para um caso de uso e um requisito real.
  • Solte qualquer função que não esteja suportando um requisito real.
  • Por fim, em sua análise de código, sinalize qualquer código que não implemente ou suporte diretamente as funções definidas.
por 18.07.2012 / 10:09
fonte
0
Provavelmente não é muito complicado, porque isso faz a maioria das pessoas se sentir mal depois. Eu suponho que quando isso acontece, já foi escrito muito código sem falar uma palavra sobre isso. (Por que isso acontece? Porque essa pessoa tem autoridade suficiente para que seu código não precise ser revisado na realidade?)

Caso contrário, acho que tornar a revisão de código menos formal, porém mais frequente. E antes de escrever grandes módulos, talvez você deva discutir rapidamente qual abordagem tomar.

Dizer "isso é muito complicado" não leva a nada.

    
por 18.07.2012 / 14:32
fonte
0

É lamentável, mas as Revisões de Código são, muitas vezes, mais para o futuro do que para o presente. Especialmente em um ambiente empresarial / corporativo, o código enviado é sempre mais valioso do que o código não expedido.

Isso, claro, depende de quando a revisão de código é concluída. Se for parte do processo de desenvolvimento, você pode ganhar algum benefício agora. Mas se o CR é tratado como mais um post-mortem, então é melhor você apontar o que poderia ser feito melhor no futuro. No seu caso (como outros já disseram), indique o YAGNI e o KISS em geral, e talvez algumas das áreas específicas em que esses princípios poderiam ser aplicados.

    
por 18.07.2012 / 16:08
fonte
0

O que significa complicado demais? Você faz uma declaração ambígua, então você receberá uma resposta ambígua / insatisfatória em resposta. O que é excessivamente complicado para uma pessoa é perfeito para outra.

O propósito de uma revisão é apontar problemas e erros específicos, para não dizer que você não gosta, o que a declaração "excessivamente complexa" implica.

Se você vir um problema (excessivamente complicado), então diga algo mais concreto como:

  • A alteração das partes X para Y não simplificaria o código ou facilitaria o entendimento?
  • Eu não entendo o que você está fazendo aqui na parte X, acho que o que você estava tentando fazer é isso. Apresentar uma maneira mais limpa de fazê-lo.
  • Como você testou isso? Você testou isso? Se for excessivamente complicado, isso geralmente leva a olhares em branco. Pedir testes fará com que a pessoa auto-simplifique o código quando não conseguir descobrir como testar o código original.
  • Parece haver um erro aqui, alterar o código para isso resolveria o problema.

Qualquer um pode apontar problemas, especialmente os ambíguos. Há um subconjunto muito menor que pode apresentar soluções. Seus comentários de revisão devem ser tão específicos quanto possível. Dizer que algo é excessivamente complexo não significa muito, pode até fazer com que os outros pensem que você é incompetente por não ser capaz de entender o código. Tenha em mente que a maioria dos desenvolvedores não tem a menor idéia da diferença entre um design bom ou ruim.

    
por 18.07.2012 / 17:15
fonte
0

Às vezes vale a pena como um grupo se concentrar em alguns dos princípios "ágeis" - eles podem ajudar um grupo ou indivíduo que parece estar um pouco fora do curso.

O foco não precisa significar uma grande reformulação de sua equipe, mas todos devem se sentar e discutir quais práticas são mais importantes para você como uma equipe. Eu sugiro discutir pelo menos estes (e provavelmente mais alguns):

  • A coisa mais simples que poderia funcionar?
  • Você não vai precisar (você está resolvendo problemas que não estavam na especificação)
  • Escreva o teste antes de codificar (ajuda você a focar seu código)
  • Não se repita

Além disso, revisões ocasionais (semanais?) sobre o que funciona, o que não funciona e o que ainda é necessário podem ser realmente úteis ... Por que não dedicar uma hora por semana para discutir os valores e práticas da equipe?

    
por 18.07.2012 / 18:08
fonte
0

Escalonamento, se você tiver um gerente técnico. Isso soa como um hábito que precisa ser quebrado.

Se o código não for compilado para a especificação, então, por definição, ele deverá falhar na revisão de código. Eu não entendo o conceito de "bem nós fizemos algo que ninguém pediu, e não está funcionando, então vamos deixar isso lá em vez de fazer algo que alguém pediu que funcione".

Este é um mau hábito para qualquer desenvolvedor entrar. Se ele / ela estava trabalhando para uma especificação de design, então não combiná-lo sem uma boa razão é um não não.

    
por 18.07.2012 / 18:18
fonte
0

Uma palavra: ágil

Certamente não resolve tudo. Mas reinando em suas iterações (1-2 semanas, por exemplo), limitando o trabalho em andamento e aproveitando o planejamento / revisão do sprint, você deve evitar esses erros semelhantes a quedas de água. Você precisa de uma melhor visibilidade do que realmente está sendo feito - enquanto está sendo feito.

Para o desenvolvimento normal baseado em projetos, recomendo adotar uma abordagem Scrum . Para ambientes de desenvolvimento / integração contínuos, e especialmente se você tiver muitos desenvolvedores trabalhando no mesmo ou em projetos relacionados, considere incorporar elementos de Kanban . Outra abordagem eficaz é aproveitar a programação em par , uma prática definida de Programação extrema .

Sua situação é dificilmente única. E mesmo com equipes pequenas, o processo pode ajudar muito a evitar a situação atual. Com a devida visibilidade e um backlog razoavelmente preparado, perguntas como essas se tornam decisões de planejamento de sprint - evitando que você gere dívida técnica .

    
por 18.07.2012 / 18:25
fonte
-1

O que eu disse no passado é "esse código é complexo e não estou confiante no que ele está tentando fazer, é possível simplificá-lo ou escrevê-lo com mais clareza?"

    
por 18.07.2012 / 12:41
fonte
-2

Você codificador após excluir / reverter seu código: "Ops, seu código foi embora. Por favor, reescreva-o. Como você já escreveu, você precisará de menos de vinte minutos para fornecer SOMENTE o código requerido pela especificação.

"Minha próxima revisão é daqui a 20 minutos.

"Bom dia".

NÃO aceite argumentos!

Feito, IMHO

Chris

    
por 18.07.2012 / 17:11
fonte