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

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

Alguns desses objetivos normalmente envolvem um julgamento se o objetivo foi cumprido ou não. Por exemplo, o objetivo de que o código deve ser passível de manutenção normalmente requer uma avaliação.

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

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

Algumas boas respostas aqui, mas acho que um ponto importante está faltando. Faz uma grande diferença, cujo código você está analisando, como essa pessoa é experiente e como ela lida com essas sugestões. Se você conhece bem o seu companheiro de equipe e espera que uma nota como "este código seja falho porque ..., faça diferente" para ser suficiente para ele encontrar uma solução melhor, então um comentário pode ficar bem. Mas há definitivamente pessoas em que tal comentário não é suficiente e quem precisa saber exatamente como melhorar seu código. Então IMHO esta é uma chamada de julgamento que você só pode fazer para o caso individual.

So generally, as a reviewer, is it fine to say "this code is flawed, do it differently" or do you have to come up with a specific solution?

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 precisar de um pouco de tempo para a comunicação ao vivo.

In code reviews, should the reviewer always present a solution for issues?

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

In code reviews, should the reviewer never present a solution for issues?

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

When should a reviewer present a solution for issues?

Quando essa é a maneira mais eficaz de se comunicar. Nós somos macacos de código, não majores ingleses. Às vezes, a melhor maneira de mostrar que o código é uma porcaria ... é menos que o ideal ... é mostrar a eles um código que suga menos ... é mais opção ... oh, diabos, você sabe o que quero dizer.

A questão principal é que se as pessoas soubessem escrever melhor o código, elas geralmente o fariam em primeiro lugar. Se uma crítica é específica o suficiente depende muito da experiência do autor. Programadores muito experientes podem ser capazes de aceitar críticas como "esta aula é muito complicada" e voltar para a prancheta e criar algo melhor, porque eles só tiveram um dia de folga devido a uma dor de cabeça ou estavam sendo desleixados porque estavam com pressa.

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

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

Quando tive contato limitado de trabalho / fornecimento de feedback a alguém, eu não disse: "Este é um projeto ruim". mas algo como "Você pode explicar essa aula para mim?" Você pode descobrir que é uma boa solução, o dev sinceramente fez o melhor que pôde ou até mesmo um reconhecimento de que é uma má solução, mas é bom o suficiente.

Dependendo da resposta, você terá uma ideia melhor sobre 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 tentam resolvê-lo sozinhos.

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

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

Quando reviso o código, só forneço uma solução para os problemas que identifico, se puder fazê-lo com pouco esforço. Eu tento ser específico sobre o que eu acho que é o problema, referindo-se à documentação existente sempre que possível. Esperar que um revisor forneça uma solução para cada problema identificado cria um incentivo perverso - desencorajará o revisor a apontar problemas.

Minha opinião está indo mais strong em não fornecer código na maioria dos casos, por uma série de razões:

• Se a explicação por si só não for suficiente, eles podem sempre 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 toca 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 a parte do código e você não está, dar apenas o feedback pode resultar em um código melhor do que você escreveria. Dar a alguém uma solução pronta, muitas vezes, faz com que ele apenas o use, sem considerar melhorá-lo ainda mais.
• Sempre fornecer uma solução é quase condescendente. Você está trabalhando com alguém, esperançosamente porque eles eram bons o suficiente para serem contratados. Se você soubesse por que algo é uma má ideia, por que eles não aprenderiam ouvindo o feedback e fazendo eles mesmos?
• Sempre fornecer uma solução é simplesmente estranho. Imagine que você está programando em pares na sua mesa: eles acabaram de concluir algumas linhas que você acha que não são ótimas. Você apenas diz a eles o que viu e porque, ou pega o teclado deles e mostra sua versão imediatamente? É isso que sempre fornece a sua solução para outras pessoas.
• Você sempre pode dizer o que faria em vez de gastar o tempo para realmente escrevê-lo. Você fez exatamente isso ao descrever o primeiro problema na questão.
• Não dê comida, ensine a pescar;)

Claro, existem alguns casos em que você está pensando em alguma alternativa específica e vale a pena anexá-la. Mas isso é muito raro na minha experiência. (muitas resenhas, grandes projetos) O autor original sempre pode pedir uma amostra se precisar.

Mesmo assim, por causa da terceira razão, ao dar uma amostra, pode valer a pena dizer, por exemplo, "usar x.foo() tornaria isso mais simples" do que uma solução completa - e deixar o autor escrevê-la. Isso também economiza seu tempo.

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

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

A única vez em que surgir a questão de um substituto seria se o devedor original não conseguisse pensar em uma maneira de implementar o recurso que se ajusta às regras. Digamos que você tenha um requisito de desempenho, mas o recurso demora mais do que seu limite após várias tentativas de otimização.

No entanto! se suas regras são subjetivas "Os nomes devem ser aprovados por mim!" então, sim, você acaba de nomear-se como chefe de nomeação e deve esperar pedidos de listas de nomes aceitáveis.

Seu exemplo de herança (parâmetros opcionais) talvez seja melhor, visto que vi regras de revisão de código que proíbem métodos longos e parâmetros de função 'muitos'. Mas normalmente isso pode ser resolvido trivialmente por divisão. É a parte "esta solução parecia complicar demais" parte onde sua objetividade está se intrometendo e talvez exija justificação ou uma solução alternativa.

Se uma solução em potencial for rápida e fácil de digitar, tento incluí-la em meus comentários de revisão por pares. Se não, eu pelo menos listo minha preocupação e porque eu acho esse item específico problemático. No caso de nomes de variáveis / funções em que você não consegue pensar em algo melhor, eu geralmente reconheço que não tenho uma ideia melhor e termino o comentário na forma de uma pergunta aberta para o caso de alguém poder . Dessa forma, se ninguém apresentar uma opção melhor, a revisão não está sendo realmente interrompida.

Para o exemplo que você deu em sua pergunta, com a classe mal projetada. Gostaria de deixar alguns comentários de que, embora pareça que pode ser um exagero, a herança provavelmente seria a melhor maneira de resolver o problema que o código está tentando resolver e deixá-lo assim. Eu tentaria expressar de uma maneira que indicasse que não é um show-stopper e cabe ao critério do desenvolvedor se corrigir ou não. Gostaria também de abrir com um reconhecimento de que você não está particularmente familiarizado com essa parte do código e convidar mais desenvolvedores e / ou revisores experientes para esclarecer se há uma razão pela qual é feito da maneira que é.

Vá e fale 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 depois discuta com eles como isso poderia ser esclarecido.

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

Cara-a-cara, a largura de banda é muito maior e a pessoa tem o canal lateral emocional para evitar a hostilidade.

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