Nas revisões de código, o revisor deve sempre apresentar uma solução para os problemas? [fechadas]

93

Ao revisar o código, normalmente tento fazer recomendações específicas sobre como resolver os problemas. Mas, devido ao tempo limitado que se pode gastar para revisar, isso nem sempre funciona bem. Nesses casos, acho mais eficiente se o desenvolvedor apresentar uma solução.

Hoje revi alguns códigos e descobri que uma classe obviamente não era bem projetada. Ele tinha vários atributos opcionais que foram designados apenas para determinados objetos e deixados em branco para outros. A maneira padrão de resolver isso seria dividir a classe e usar a herança. No entanto, nesse caso específico, essa solução parecia complicar demais as coisas. Eu não estava envolvido no desenvolvimento deste software e não estou familiarizado com todos os módulos. Portanto, não me senti suficientemente qualificado para tomar uma decisão específica.

Outro caso típico que experimentei muitas vezes é que encontro uma função, classe ou nome de variável obviamente sem sentido ou até enganoso, mas não consigo inventar um bom nome.

Então, geralmente, como revisor, não há problema em dizer "esse código é defeituoso porque ... é diferente" ou você precisa apresentar uma solução específica?

Frank Puffer
fonte
24
@gnat: Não, o código não é muito complicado. E é apenas um exemplo. Geralmente, pergunto se o revisor é responsável por apresentar uma solução.
Frank Puffer
5
não, eu diria que, como revisor, você não é obrigado a dizer como melhorá-lo. Se você pode descrever o que parece errado lá dentro, faça-o; caso contrário - forneça comentários gerais. (Um dos mais comentários de revisão úteis me lembro de receber foi literalmente como "esta classe é tudo lixo total",)
mosquito
5
"A maneira padrão de resolver isso seria dividir a classe e usar a herança." Espero que você não esteja revendo meu código!
Gardenhead # 23/17
7
Identificar problemas em potencial pode ser suficiente. O revisor analisa o código como usuário, mantenedor ou designer. Fornecer uma visão de ângulo diferente ou detectar problemas que o codificador talvez ainda não tenha percebido pode ajudar o codificador a melhorar seu trabalho. E se você encontrar algo que goste, não faria mal denunciar isso também. Não deve ser um exercício corretivo, mas esclarecedor. É por isso que é chamado de "revisão por pares".
Martin Maat

Respostas:

139

Como revisor, seu trabalho é verificar se um código (ou um documento) atende a determinados objetivos que foram acordados antes da revisão.

Alguns desses objetivos normalmente envolvem um julgamento, independentemente de o objetivo ter sido cumprido ou não. Por exemplo, o objetivo de que o código deve ser mantido normalmente requer uma chamada de julgamento.

Como revisor, é seu trabalho apontar onde os objetivos não foram alcançados e é o trabalho do autor garantir que o trabalho dele realmente atenda aos objetivos. Dessa forma, não é seu trabalho informar como as correções devem ser feitas.

Por outro lado, apenas dizer ao autor "isso é falha. Corrija-o" geralmente não leva a uma atmosfera positiva na equipe. Para uma atmosfera positiva, é bom pelo menos indicar por que algo está errado em seus olhos e fornecer uma alternativa melhor se você tiver uma.
Além disso, se você estiver revisando algo que parece "errado", mas realmente não tem uma alternativa melhor, também poderá deixar um comentário como "Este código / design não se encaixa bem comigo, mas eu não tem uma alternativa clara. Podemos discutir isso? " e então tente fazer algo melhor juntos.

Bart van Ingen Schenau
fonte
23
+1 para discussão para chegar a uma solução em conjunto - esta é a maneira que eu aprender o máximo dos programadores seniores rever o meu código
dj18
19
+1. Ao dar feedback, é melhor fazer críticas construtivas sempre que possível.
FrustratedWithFormsDesigner
7
Concordo especialmente com o último pedaço. Não há problema em dizer: "esta solução parece errada porque ..." ou "Estou preocupado que essa parte possa ser problemática porque ..." sem fornecer uma solução.
Daniel T.
11
@dotancohen: O "podemos discutir isso" era para ser uma pergunta. Pessoalmente, eu teria a discussão de qualquer maneira, mesmo que seja apenas para aprender alguma coisa.
Bart van Ingen Schenau
11
O perigo sutil é que, com discussões e implementações suficientes, isso deixa de ser uma revisão e se torna uma programação em pares. A programação em pares é boa, mas uma vez concluída, você precisa de uma terceira pessoa para revisar porque a independência foi perdida.
Candied_orange 24/05
35

Algumas boas respostas aqui, mas acho que falta um ponto importante. Faz uma grande diferença o código que você está revisando, a experiência dessa pessoa e como ela lida com essas sugestões. Se você conhece bem seu colega de equipe e espera que uma observação como "esse código seja defeituoso porque ... faça diferente" seja suficiente para que ele encontre uma solução melhor, esse comentário poderá ser bom. Mas definitivamente existem pessoas em que esse comentário não é suficiente e precisam ser informadas com precisão sobre como melhorar seu código. Então, IMHO, esta é uma decisão que você só pode fazer para cada caso.

Doc Brown
fonte
29

Então, geralmente, como revisor, é bom dizer "esse código é defeituoso, faz diferente" ou você precisa apresentar uma solução específica?

Nenhum dos dois é o IMO ideal. A melhor coisa a fazer é conversar com o autor e corrigir o problema de forma colaborativa.

As revisões de código não precisam ser assíncronas. Muitos problemas serão desbloqueados se você parar de vê-los como um processo burocrático e demorar um pouco para a comunicação ao vivo.

guillaume31
fonte
"Processo burocrático" é uma maneira muito boa de colocar isso!
Frnhr 25/05
17

Nas revisões de código, o revisor deve sempre apresentar uma solução para os problemas?

Não. Se você está fazendo isso, não é um revisor, é o próximo codificador.

Nas revisões de código, o revisor nunca deve apresentar uma solução para problemas?

Não. Seu trabalho é comunicar o problema em questão. Se apresentar uma solução deixar o problema claro, faça-o. Só não espere que eu siga sua solução. A única coisa que você pode fazer aqui é esclarecer. Você não pode ditar a implementação.

Quando um revisor deve apresentar uma solução para problemas?

Quando essa é a maneira mais eficaz de se comunicar. Somos macacos de código e não profissionais de inglês. Às vezes, a melhor maneira de mostrar que o código é péssimo ... é menos que o ideal ... é mostrar a eles o código que suga menos ... é mais otimizado ... oh, você sabe o que quero dizer.

candied_orange
fonte
8
Não codifique no vácuo, porque é uma merda.
Hum. Quando sugiro uma solução para um problema, geralmente tenho benefícios que tenho conhecimento, mas levaria muito tempo para fornecer uma lista exaustiva de todos eles. (Elas geralmente estão relacionadas à estabilidade e à confiança de que a coisa continuará funcionando enquanto mudamos outras coisas à sua volta.) Portanto, se você fizer algo que não resolva tantos problemas, eu não ficaria exatamente feliz (pelo menos a menos que você possa me dizer uma boa razão pela qual o que eu sugeri não deu certo). Como você lidaria com isso?
Jpmc26
11
PS: "Code monkey" é frequentemente usado para descrever um programador não qualificado e impensado que simplesmente faz o que é dito mesmo que seja uma má idéia e não tenha boas sensibilidades de design. Veja o dicionário urbano . Até a Wikipedia observa que às vezes é depreciativo.
Jpmc26
@ jpmc26 se você for usar código para se comunicar comigo, espero que você use um código que mostre como o problema pode ser resolvido melhor. Além disso, o Code Monkey pode ser usado com carinho. Certamente mais carinho do que majores ingleses obter
candied_orange
"Macaco-código se levanta, pega café. Macaco-código vai ao trabalho. O macaco-código tem uma reunião chata, com o gerente chato Rob. Rob diz que o macaco-código é muito diligente, mas sua saída fede ..."
Baldrickk
13

A questão principal é que, se as pessoas soubessem como escrever melhor o código, normalmente o teriam feito em primeiro lugar. Se uma crítica é suficientemente específica depende muito da experiência do autor. Programadores muito experientes podem ser capazes de fazer críticas como "essa classe é muito complicada" e voltar à prancheta e criar algo melhor, porque eles apenas tiveram um dia de folga devido a uma dor de cabeça ou estavam sendo desleixados porque estavam com pressa.

Geralmente, porém, você precisa pelo menos identificar a fonte da complicação. "Essa aula é muito complicada porque quebra a Lei de Demeter em todo o lugar". "Esta classe mistura responsabilidades da camada de apresentação e da persistência." Aprender a identificar esses motivos e explicá-los de forma sucinta faz parte de se tornar um revisor melhor. Você raramente precisa entrar em muitos detalhes sobre soluções.

Karl Bielefeldt
fonte
4
A minha frustração mais comum com o Code Reviews é que, se eu soubesse uma maneira melhor, provavelmente teria escrito dessa maneira.
precisa
Eu amo sua primeira frase. Isso me fez pensar em me perguntar: "esse código é bom o suficiente?" então, lançando uma moeda para decidir se quer ou não melhorá-la! (Normalmente eu apenas mantê-la até eu ficar sem tempo, mas talvez eu pudesse parar quando ele é bom o suficiente em vez disso?)
IMO "Este código é complicado porque quebra a Lei de Demeter" é um comentário ruim. "Este código é complicado porque X está muito acoplado a Y e Z" é melhor.
User253751 25/05
"Se as pessoas soubessem como escrever melhor o código, normalmente o fariam em primeiro lugar". Há exceções. Eu desenvolvi esse código que funciona, mas vai nos morder na bunda em algum momento no futuro. O gerente não técnico não entende "Não gosto desse código e quero três dias para melhorá-lo". O gerente não técnico entende "Joe revisou e rejeitou este código e preciso de três dias para aprimorá-lo".
gnasher729
4

Existem dois tipos de programadores ruins: aqueles que não seguem práticas padrão e aqueles que "apenas" seguem práticas padrão.

Quando eu tenho contato de trabalho limitado / fornecendo feedback a alguém, eu não diria: "Este é um design ruim". mas algo como "Você pode me explicar essa aula?" Você pode descobrir que é uma boa solução, o desenvolvedor sinceramente fez o melhor que pôde ou até mesmo o reconhecimento de que é uma solução ruim, mas é boa o suficiente.

Dependendo da resposta, você terá uma idéia melhor de como abordar cada situação e pessoa. Eles podem reconhecer rapidamente o problema e descobrir a correção por conta própria. Eles podem pedir ajuda ou apenas tentarão resolvê-los por conta própria.

Existem práticas sugeridas em nossos negócios, mas quase todas têm exceções. Se você entende o projeto e como a equipe está se aproximando dele, esse pode ser o contexto para determinar o objetivo da revisão de código e como lidar com as preocupações.

Sei que isso é mais uma abordagem do problema do que uma solução explícita. Haverá muita variabilidade para cobrir todas as situações.

JeffO
fonte
11
Mas, se exigir que a explicação seja um design reconhecidamente bom, faltam comentários embutidos.
Wildcard
11
Às vezes, as regras não têm exceções, mas geralmente não.
@Wildcard - isso depende da capacidade e preferências / opiniões das pessoas que o observam.
JeffO 23/05
11
@Wildcard Eu adotei a abordagem de que o feedback deveria ser formulado como uma pergunta, mas a resposta (eventualmente) assumirá a forma de um comentário ou talvez uma alteração de código (por exemplo, melhor nomeação). Isso deixa a porta aberta para o desenvolvedor explicar seu pensamento e discutir as opções, em vez de sentir uma demanda ou acidentalmente fazer o trabalho deles por eles.
IMSoP
3

Quando reviso o código, apenas forneço uma solução para os problemas que identifico se posso fazê-lo com pouco esforço. Eu tento ser específico sobre o que acho que é o problema, consultando a documentação existente sempre que possível. Esperar que um revisor forneça uma solução para todos os problemas identificados cria um incentivo perverso - isso desencorajará o revisor de apontar problemas.

BagOfSpanners
fonte
3

Minha opinião é mais forte no sentido de não fornecer código na maioria dos casos, por várias razões:

  • Se a explicação por si só não for suficiente, eles sempre poderão pedir uma amostra do que você está pensando.
  • Você não está desperdiçando seu tempo tentando se familiarizar com algum código que não tocou há muito tempo, apenas para modificá-lo um pouco, enquanto outra pessoa passou o tempo fazendo exatamente isso.
  • Se eles já estão familiarizados com o trecho de código e você não, dar apenas o feedback pode resultar em um código melhor do que você escreveria. Dar a alguém uma solução pronta geralmente fará com que ela a use, sem considerar melhorá-la.
  • Sempre fornecer uma solução é quase condescendente. Você está trabalhando com alguém, espero que seja bom o suficiente para ser contratado. Se você conseguiu aprender por que algo é uma má ideia, por que eles não aprenderiam ouvindo feedback e fazendo isso sozinhos?
  • Sempre fornecer uma solução é apenas estranho. Imagine que você está programando em pares na mesa deles: eles terminaram algumas linhas que você acha que não são ótimas. Você apenas diz a eles o que viu e por quê, ou pega o teclado deles e mostra sua versão imediatamente? É assim que sempre é possível fornecer sua solução para outras pessoas.
  • Você sempre pode dizer o que faria, sem gastar tempo para realmente escrever. Você fez exatamente isso ao descrever o primeiro problema na pergunta.
  • Não distribua comida, ensine a pescar;)

Claro, existem alguns casos em que você pensa em alguma alternativa específica e vale a pena anexá-la. Mas isso é realmente raro na minha experiência. (muitas revisões, grandes projetos) O autor original sempre pode solicitar uma amostra, se necessário.

Mesmo assim, por causa do terceiro motivo, ao fornecer uma amostra, pode valer a pena dizer, por exemplo, "usar x.foo()tornaria isso mais simples" em vez de uma solução completa - e deixar que o autor a escreva. Isso também economiza seu tempo.

viraptor
fonte
Seu quinto ponto me fez sorrir, eu estava imaginando "teclados de duelo" para ver quem conseguiria uma ótima solução primeiro. Quem sabia que a programação por pares poderia ser como aqueles dois jogos de arcade de carros de corrida ou um esporte de contato total? " Steve acabou de fazer um check brutal de Ron no BSOD, a 2 minutos da grande penalidade ... "
2

Eu acho que a chave para codificar revisões é concordar com as regras antes da revisão.

Se você tiver um conjunto claro de regras, não haverá necessidade de oferecer uma solução. Você está apenas verificando se as regras foram seguidas.

O único momento em que a questão de um substituto surgiria seria se o desenvolvedor original não pensasse em uma maneira de implementar o recurso que se encaixa nas regras. Digamos que você tenha um requisito de desempenho, mas o recurso leva mais tempo que o seu limite após várias tentativas de otimização.

Contudo! se suas regras forem subjetivas "Os nomes devem ser aprovados por mim!" então, sim, você acabou de se nomear chefe e deve esperar solicitações de listas de nomes aceitáveis.

Seu exemplo de herança (parâmetros opcionais) é talvez melhor, int que eu já vi regras de revisão de código que proíbem métodos longos e parâmetros de função 'muitos'. Mas normalmente estes podem ser resolvidos trivialmente pela divisão. É a parte "essa solução parecia complicar demais as coisas" onde sua objetividade é intrusiva e talvez exija justificativa ou uma solução alternativa.

Ewan
fonte
2
"Acho que a chave para as revisões de código é concordar com as regras antes da revisão". Este seria o caso ideal. Na prática, não podemos assumir que todo desenvolvedor conheça todas as regras. Revisões de código são úteis para espalhar esse conhecimento e explicar as regras com exemplos práticos. Talvez essa seja uma das maiores vantagens de fazer revisões de código ..
Frank soprador
escrever as regras no documento padrões de codificação e dar a novos devs
Ewan
11
Escrevemos os padrões de codificação e eles são fornecidos aos novos desenvolvedores. Isso funciona na maioria das vezes, mas às vezes há interpretações errôneas. Além dos padrões de codificação escritos, existem princípios gerais como DRY ou SOLID que também são abordados nas revisões de código. Esperamos um conhecimento básico sobre isso de nossos desenvolvedores e também fazemos alguns treinamentos internos para aprimorá-lo. Esse é um processo contínuo e as revisões de código fazem parte dele.
Frank Puffer
0

Se uma solução em potencial é rápida e fácil de digitar, tento incluí-la nos comentários da revisão por pares. Caso contrário, pelo menos listo minha preocupação e por que acho esse item em particular problemático. No caso de nomes de variáveis ​​/ funções em que você não consegue pensar em algo melhor, geralmente reconheço que não tenho uma idéia melhor e encerro o comentário na forma de uma pergunta aberta, apenas no caso de alguém poder . Dessa forma, se ninguém tiver uma opção melhor, a revisão não será realmente realizada.

Para o exemplo que você deu na sua pergunta, com a classe mal projetada. Gostaria de deixar alguns comentários de que, embora pareça exagero, a herança provavelmente seria a melhor maneira de resolver o problema que o código está tentando resolver e deixar por isso mesmo. Eu tentaria formular de uma maneira que indique que não é um impedimento de exibição e que depende do critério do desenvolvedor corrigir ou não. Também gostaria de abrir com um reconhecimento de que você não está particularmente familiarizado com essa parte do código e convidar desenvolvedores e / ou revisores mais informados para esclarecer se há uma razão para que isso seja feito da maneira que é.

Eric Hydrick
fonte
0

Vá e converse com a pessoa cujo código você está revisando. Diga a eles, de maneira amigável, que você achou um pouco difícil de entender e, em seguida, discuta com eles como isso pode ficar mais claro.

A comunicação escrita leva a grandes quantidades de tempo perdido, bem como a ressentimentos e mal-entendidos.

Face a face, a largura de banda é muito maior e existe um canal lateral emocional para evitar hostilidade.

Se você realmente fala com o cara, é muito mais rápido, e você pode fazer um novo amigo e descobrir que os dois gostam mais do seu trabalho.

John Lawrence Aspden
fonte
este não parece oferecer nada substancial sobre os pontos feitos e explicado em antes 11 respostas
mosquito