Frustrações de revisão por pares / código

Eu não me chamaria de desenvolvedor superstar, mas relativamente experiente. Eu tento manter a qualidade do código em um nível alto e estou sempre buscando fazer melhorias no meu estilo de codificação, tentando torná-lo eficiente, legível e consistente, além de incentivar a equipe a seguir padrões e metodologias para garantir a consistência. Também entendo a necessidade de equilíbrio entre qualidade e velocidade.

Para conseguir isso, apresentei à minha equipe o conceito de revisão por pares. Dois polegares para cima no pedido de pull do github para uma mesclagem. Ótimo - mas não na minha opinião sem soluços.

Costumo ver comentários de revisão por pares dos mesmos colegas como -

• Seria bom adicionar um espaço depois <INSERT SOMETHING HERE>
• Linha extra indesejada entre métodos
• O ponto final deve ser usado no final dos comentários nos docblocks.

Agora, da minha perspectiva - o revisor está analisando superficialmente a estética do código - e não está realmente realizando uma revisão do código. A revisão do código cosmético me parece uma mentalidade arrogante / elitista. Falta substância, mas você não pode discutir muito com isso porque o revisor está tecnicamente correto . Preferiria ver menos dos tipos acima de comentários e mais comentários da seguinte forma:

• Você pode reduzir a complexidade ciclomática ...
• Sair cedo e evitar if / else
• Abstraia sua consulta ao banco de dados em um repositório
• Essa lógica realmente não pertence aqui
• Não se repita - resumo e reutilização
• O que aconteceria se Xfosse passado como argumento para o método Y?
• Onde está o teste de unidade para isso?

Acho que são sempre os mesmos tipos de pessoas que dão os tipos cosméticos de revisões e os mesmos tipos de pessoas que, na minha opinião, fazem as revisões por pares "baseadas na Qualidade e na Lógica".

Qual (se houver) é a abordagem correta para a revisão por pares. E estou correto ao ficar frustrado com as mesmas pessoas, basicamente vasculhando o código procurando erros de ortografia e defeitos estéticos, em vez de defeitos reais do código?

Se eu estiver correto - como eu encorajaria os colegas a realmente procurar falhas no código em equilíbrio com a sugestão de retoques cosméticos?

Se eu estiver incorreto - por favor, me esclareça. Existem regras práticas para o que realmente constitui uma boa revisão de código? Perdi o objetivo do que são as revisões de código?

Da minha perspectiva - a revisão do código é sobre responsabilidade compartilhada pelo código. Eu não me sentiria confortável dando o polegar para cima sem codificar / verificar a lógica, a legibilidade e a funcionalidade. Eu também não me incomodaria em bloquear uma mesclagem de um pedaço sólido de código se percebesse que alguém havia omitido um ponto final em um bloco de documentos.

Ao revisar o código, gasto talvez entre 15 e 45 minutos por 500 Loc. Não consigo imaginar essas revisões superficiais levando mais de 10 minutos, se essa é a profundidade da revisão que elas estão executando. Além disso, qual é o valor do polegar para cima do revisor superficial? Certamente, isso significa que todos os polegares não têm o mesmo peso e é necessário que haja um processo de revisão em duas passagens. Um polegar para análises profundas e um segundo polegar para o "polimento"?

Tipos de críticas

Não existe uma maneira verdadeira de fazer análises por pares. Existem várias maneiras de julgar se o código é de uma qualidade suficientemente alta. Claramente, existe a questão de saber se é de buggy ou se possui soluções que não escalam ou que são quebradiças. Questões de conformidade com padrões e diretrizes locais, embora talvez não sejam tão críticas quanto algumas outras, também fazem parte do que contribui para um código de alta qualidade.

Tipos de revisores

Assim como temos critérios diferentes para julgar o software, as pessoas que fazem o julgamento também são diferentes. Todos nós temos nossas próprias habilidades e predileções. Alguns podem pensar que aderir aos padrões locais é altamente importante, assim como outros podem estar mais preocupados com o uso da memória ou com a cobertura do código de seus testes, e assim por diante. Você deseja todos esses tipos de análises, pois, como um todo, elas o ajudarão a escrever um código melhor.

Uma revisão por pares é colaboração, não um jogo de tags

Não sei se você tem o direito de dizer a eles como fazer o trabalho deles. A menos que você saiba com certeza, suponha que essa pessoa esteja tentando contribuir da maneira que achar melhor. No entanto, se você encontrar espaço para melhorias ou suspeitar que talvez eles não entendam o que é esperado em uma revisão por pares, converse com eles .

O objetivo de uma revisão por pares é envolver seus pares . O envolvimento não é jogar código por cima do muro e esperar que uma resposta seja lançada de volta. O envolvimento está trabalhando juntos para criar um código melhor. Converse com eles.

Conselhos

No final da sua pergunta, você escreveu:

como eu encorajaria os colegas a procurar falhas no código em equilíbrio com erros estéticos flagrantes?

Mais uma vez, a resposta é comunicação. Talvez você possa perguntar a eles "ei, eu aprecio que você tenha percebido esses erros. Isso me ajudaria tremendamente se você também pudesse se concentrar em algumas questões mais profundas, como se estou estruturando meu código corretamente. Eu sei que isso leva tempo, mas seria realmente Socorro."

Em uma nota mais pragmática, eu pessoalmente divido os comentários de revisão de código em dois campos e os uso de frases apropriadas: coisas que precisam ser consertadas e coisas mais cosméticas. Eu nunca impediria o código sólido e funcional de fazer check-in se houvesse muitas linhas em branco no final de um arquivo. Vou apontar isso, no entanto, mas farei isso com algo como "nossas diretrizes dizem ter uma única linha em branco no final, e você tem 20. Não é um impedimento, mas se você tiver uma chance, pode querer consertar ".

Aqui está outra coisa a considerar: pode ser uma irritação sua que eles façam uma revisão superficial do seu código. Pode muito bem ser que uma irritação deles é que você (ou algum outro colega de equipe que recebe uma análise semelhante) é desleixado com relação aos padrões de codificação da sua própria organização, e foi assim que eles escolheram comunicar isso com você.

O que fazer após a revisão

E, finalmente, um conselho após a revisão: ao confirmar o código após uma revisão, você pode considerar cuidar de todas as coisas cosméticas em um commit e as alterações funcionais em outro. A mistura dos dois pode dificultar a diferenciação de mudanças significativas de insignificantes. Faça todas as alterações cosméticas e, em seguida, confirme com uma mensagem como "cosmético; sem alterações funcionais".

As pessoas comentam sobre formatação de código e erros de digitação porque são fáceis de identificar e não exigem muito esforço delas.

Esta parte é fácil de corrigir - quase qualquer idioma possui uma ferramenta de verificação de estilo ou linter. Conecte-o ao processo de compilação, para que falhe na compilação se houver um espaço redundante. Como resultado, não haverá problemas de estilo para comentar.

Fazer com que eles encontrem problemas reais pode ser um grande desafio. Não só isso requer uma mentalidade inquisitiva e detalhista, mas também uma motivação significativa.

E essa motivação vem da experiência. Experiência em tentar trabalhar com códigos ruins criados por desenvolvedores anteriores. Os engenheiros seniores têm muito disso. Nadar no oceano de merda dá a você um grande desejo de não chegar lá novamente.

Se eu precisar escolher uma das principais coisas a verificar durante a revisão do código, seria a manutenção do código. O desenvolvedor que estiver revisando esse código deve sempre entender e estar pronto para aprimorá-lo e modificá-lo. Se ele não tem idéia do que está acontecendo aqui, ele precisa contar imediatamente, e o código precisa ser reescrito.

Aí vem a resposta prática:

Cenário 1 : Você é o membro sênior e alguém que pode decidir a prática

Convoque uma reunião e estabeleça as regras e diretrizes da Revisão de Código. Confie em mim, diretrizes claras funcionam melhor do que qualquer sistema ou treinamento de "honra". Deixe claro que os problemas de formatação de código não devem ser levantados. Alguns membros vão se opor. Ouça-os e peça-lhes que sigam as diretrizes nos primeiros meses como um experimento. Demonstre vontade de mudar se as diretrizes atuais não funcionarem.

Cenário 2 : Você não é alguém que decide a prática ou é um membro relativamente júnior da equipe

Sua melhor aposta é resolver os problemas de revisão de código até conseguir atingir o cenário 1 .

A resposta simples para impedir comentários "triviais" de revisão de código é insistir (por uma questão de eficiência) que o revisor deve ser o único a corrigi-lo. Portanto, se o revisor descobrir que você (horror!) Perdeu um ponto final ou soletrou um comentário incorretamente, deve corrigi-lo e seguir em frente.

Na minha experiência, isso significa que:

• seus revisores param de fazer esses comentários de revisão relativamente inúteis e os devolvem para serem corrigidos.
• somente a maioria dos revisores de TOC os corrigirá, o que significa que a maioria das suas avaliações será aprovada. isso tem o efeito indireto de exigir que os desenvolvedores realizem análises mais substanciais, aqueles que relatam trivialidades porque na verdade não estão analisando o código acabam precisando justificar por que todas as análises passam sem comentários.

De qualquer forma, isso é um benefício. O custo de uma revisão com falha é alto em termos de fazer um desenvolvedor parar o que está trabalhando e revisitar seu código e a revisão subsequente. Se uma revisão encontrar problemas reais de qualidade ou arquitetura do código, esse custo valerá cada centavo, mas pagar esse custo por trivialidades não será.

Revise o processo de revisão

Além das revisões de código, sugiro que também reveja o processo de revisão regularmente. Certamente também há muita coisa que as pessoas podem aprender aqui, por exemplo, como fazer análises de código apropriadas.

Normalmente, alguns dos shedders de bicicleta são apenas inexperientes e simplesmente não sabem o que procurar. Eles precisam de um pouco de orientação não apenas em relação ao seu código, mas também em relação à revisão de códigos úteis. O fornecimento de feedback sobre as revisões levará a um processo de aprendizado que (com êxito) resulta em melhores análises e um código melhor.

Em seguida, pode ser uma boa ideia formalizar (vagamente) um conjunto de valores e critérios, o que a organização ou equipe considera como Good Code ™ e quais são os antipadrões que devem ser evitados. Não se trata de definir sth. em concreto, mas sobre obter um consenso comum sobre a qualidade do código desde o início .

Como alguém que esteve nos dois lados disso (revisando o código escrito por outras pessoas, além de revisar o código que escrevi), eu diria que tenho três categorias em que qualquer feedback se enquadra. Bem, quatro, há também o delicioso caso de "tudo está bem".

"Seria bom, mas não o bloqueará" (se todo o feedback se enquadrar nessa categoria, posso enviar a solicitação de mesclagem com um "será mesclado em XX: XX, a menos que você me diga que deseja corrigi-los" , ou "com certeza, faça o check-in, qualquer bloco que o sistema lançaria foi desativado"):

• Esquecer pontos finais no final das frases (em strings de documentos ou comentários). Gramática desajeitada em qualquer coisa que não seja emitida para os usuários de forma ou formato (novamente, sequências de documentos e comentários)
• Código que tem uma maneira mais elegante, mas o que é compreensível e idiomático (provavelmente vou colocar minha sugestão, então é fácil sair ou consertar)

"Coisas que precisam ser consertadas, mas eu confio em você para fazer isso" (se todas as coisas se encaixarem nessa categoria ou na categoria anterior, responderei com "conserte estas, mesclarei quando você me disser que ' está pronto "(e nesse momento provavelmente irei pesquisar rapidamente para ver se nada mais apareceu)):

• Pequenas reclamações ("você está comparando um booleano true, isso é um pouco paranóico ...", ...)
• Violações menores do estilo ("o guia de estilo diz X, o que você fez está fora disso; eu faria Y ou Z, dependendo do caminho que você deseja seguir")
• Alguns casos de teste ausentes, que não devem ser difíceis de escrever

E, finalmente, "coisas problemáticas, precisarei revisar seu código novamente depois que você corrigir esses problemas" (se houver algum nessa categoria, será necessário fazer outra rodada de revisão, por isso, coloque um comentário dizendo " há também algumas coisas menores, seria bom ver algumas delas corrigidas enquanto você está nisso ", se houver alguma coisa das duas primeiras categorias presentes):

• Isso seria algo como "não, existe realmente uma maneira MUITO melhor de escrever isso", "você não está computando o que espera, porque seu teste de unidade perde esses casos extremos" e todos os outros problemas graves.

Parece que algumas pessoas esqueceram a pergunta mais importante: o que você realmente deseja obter com as revisões de código?

O objetivo das revisões de código é obter um código livre de erros e de manutenção mais rápido. As revisões de código conseguem isso de várias maneiras: os desenvolvedores escrevem melhor o código em primeiro lugar porque sabem que será revisado, o conhecimento é compartilhado como parte do processo de revisão, os bugs serão encontrados porque o revisor não está cego para seus próprios erros como desenvolvedores pode ser.

Se você vê o processo de revisão como uma chance de demitir seus colegas ou criar trabalho para eles, está fazendo algo errado.