Code Review Best Practices

Mar 4, 2018 – 12 min read

/div>

A Internet fornece uma riqueza de material sobre revisões de códigos: sobre o efeito das revisões de códigos na cultura da empresa, sobre revisões formais de segurança, guias mais curtos, listas de verificação mais longas, revisões humanizadas, razões para fazer revisões de códigos em primeiro lugar, melhores práticas, mais melhores práticas, estatísticas sobre a eficácia das revisões de códigos para apanhar bugs, e exemplos de revisões de códigos que correram mal. Ah, e claro que também existem livros. Resumindo, este post no blogue apresenta a tomada de Palantir das resenhas de código. As organizações com profunda relutância cultural às revisões por pares podem querer consultar o excelente ensaio de Karl E. Wiegers sobre Humanizar as Revisões por Pares antes de tentarem seguir este guia.

Este post é copiado dos guias de melhores práticas da nossa cadeia de ferramentas Java Code Quality, Baseline, e cobre os seguintes tópicos:

• Porquê, o quê, e quando fazer revisões de código
• Preparar código para revisão
• Realizar revisões de código
• Exemplos de revisão de código

Realizamos revisões de código (CRs) a fim de melhorar a qualidade do código e beneficiar de efeitos positivos na cultura da equipa e da empresa. Por exemplo:

• Os committers são motivados pela noção de um conjunto de revisores que irão analisar o pedido de mudança: o committer tende a limpar as pontas soltas, consolidar os TODOs, e geralmente melhorar o commit. O reconhecimento da perícia de codificação através dos pares é uma fonte de orgulho para muitos programadores.
• A partilha de conhecimentos ajuda as equipas de desenvolvimento de várias maneiras:
  – Um CR comunica explicitamente a funcionalidade adicionada/alterada/removida aos membros da equipa que podem subsequentemente desenvolver o trabalho feito.
  – O comitente pode utilizar uma técnica ou algoritmo com o qual os revisores podem aprender. Mais geralmente, as revisões de código ajudam a elevar a barra de qualidade em toda a organização.
  – Os revisores podem possuir conhecimentos sobre técnicas de programação ou a base de código que podem ajudar a melhorar ou consolidar a mudança; por exemplo, outra pessoa pode estar simultaneamente a trabalhar numa característica ou correcção semelhante.
  – A interacção positiva e a comunicação fortalecem os laços sociais entre os membros da equipa.
• Consistência numa base de código torna o código mais fácil de ler e compreender, ajuda a prevenir bugs, e facilita a colaboração entre espécies regulares e migratórias de desenvolvedores.
• A legibilidade dos fragmentos de código é difícil de julgar para o autor cujo cérebro é filho, e fácil de julgar para um revisor que não tem o contexto completo. O código legível é mais reutilizável, sem bugs, e à prova de futuro.
• Erros acidentais (por exemplo, erros de digitação) bem como erros estruturais (por exemplo, código morto, bugs lógicos ou de algoritmos, preocupações de desempenho ou arquitectura) são muitas vezes muito mais fáceis de detectar para revisores críticos com uma perspectiva externa. Estudos descobriram que mesmo revisões de código curtas e informais têm um impacto significativo na qualidade do código e frequência de bugs.
• Os ambientes de conformidade e regulamentação exigem frequentemente revisões. Os CR são uma óptima forma de evitar armadilhas de segurança comuns. Se a sua característica ou ambiente tiver requisitos de segurança significativos, beneficiará de (e provavelmente exigirá) revisão por parte dos seus curmudgeons de segurança locais (o guia do OWASP é um bom exemplo do processo).

O que rever

Não há uma resposta eternamente verdadeira a esta questão e cada equipa de desenvolvimento deve concordar na sua própria abordagem. Algumas equipas preferem rever cada alteração fundida no ramo principal, enquanto outras terão um limiar de “trivialidade” abaixo do qual não é necessária uma revisão. O compromisso é entre a utilização eficaz do tempo dos engenheiros (tanto dos autores como dos revisores) e a manutenção da qualidade do código. Em certos ambientes regulamentares, a revisão de código pode ser necessária mesmo para alterações triviais.

As revisões de código são sem classe: ser a pessoa mais sénior da equipa não implica que o seu código não precise de revisão. Mesmo que, no caso raro, o código seja impecável, a revisão proporciona uma oportunidade de mentoria e colaboração, e, minimamente, diversifica a compreensão do código na base do código.

Quando rever

As revisões de código devem acontecer após verificações automatizadas (testes, estilo, outros IC) terem sido concluídas com sucesso, mas antes do código se fundir no ramo principal do repositório. Geralmente não realizamos revisões formais de código de alterações agregadas desde a última versão.

Para alterações complexas que devem fundir-se no ramo da linha principal como uma única unidade, mas que são demasiado grandes para caberem num RC razoável, considere um modelo de RC empilhado: Criar um ramo primário feature/big-feature e um número de ramos secundários (por exemplo, feature/big-feature-apifeature/big-feature-testing, etc.) que cada um encapsula um subconjunto da funcionalidade e que são revistos individualmente em relação ao ramo feature/big-feature. Uma vez todos os ramos secundários fundidos em feature/big-feature, criar um CR para fundir este último no ramo principal.

Preparar código para revisão

É da responsabilidade do autor submeter CR fáceis de rever para não desperdiçar o tempo e motivação dos revisores:

• Âmbito e tamanho. As alterações devem ter um âmbito restrito, bem definido e auto-contido que abranja exaustivamente. Por exemplo, uma alteração pode implementar uma nova funcionalidade ou corrigir um bug. As alterações mais curtas são preferidas às mais longas. Se uma CR fizer alterações substanciais a mais de ~5 ficheiros, ou demorar mais de 1-2 dias a escrever, ou demorar mais de 20 minutos a rever, considere dividi-la em múltiplas CR auto-contidas. Por exemplo, um programador pode submeter uma alteração que define a API para uma nova funcionalidade em termos de interfaces e documentação, e uma segunda alteração que acrescenta implementações para essas interfaces.

li>Apenas submeter CR completos, auto-revisados (por dif), e CR auto-testados. A fim de poupar tempo aos revisores, testar as alterações submetidas (isto é, executar o conjunto de testes) e certificar-se de que passam todas as compilações, bem como todos os testes e verificações de qualidade do código, tanto localmente como nos servidores de IC, antes de atribuir revisores.

• As alterações de refactorização não devem alterar o comportamento; inversamente, uma alteração de comportamento deve evitar alterações de refactorização e de formatação do código. Há várias boas razões para isto:
  – As alterações de refactoring tocam frequentemente em muitas linhas e ficheiros e, consequentemente, serão revistas com menos atenção. Alterações involuntárias de comportamento podem vazar para a base de código sem que ninguém se aperceba.
  – Grandes alterações de refactoring quebram a selecção de cereja, rebase, e outra magia de controlo de fontes. É muito oneroso desfazer uma mudança de comportamento que foi introduzida como parte de um compromisso de refactoring em todo o repositório.
  – Tempo dispendioso de revisão humana deve ser gasto na lógica do programa em vez de debates sobre estilo, sintaxe, ou formatação. Preferimos resolver aqueles com ferramentas automatizadas como Checkstyle, TSLint, Baseline, Prettier, etc.

Comprometer mensagens

O seguinte é um exemplo de uma boa mensagem de commit seguindo um padrão amplamente citado:

Capitalized, short (80 chars or less) summaryMore detailed explanatory text, if necessary. Wrap it to about 120 characters or so. In some contexts, the first
line is treated as the subject of an email and the rest of the text as the body. The blank line separating the
summary from the body is critical (unless you omit the body entirely); tools like rebase can get confused if you run
the two together.Write your commit message in the imperative: "Fix bug" and not "Fixed bug" or "Fixes bug." This convention matches
up with commit messages generated by commands like git merge and git revert.Further paragraphs come after blank lines.- Bullet points are okay, too

Tenta descrever tanto o que o commit muda como a forma como o faz:

> BAD. Don't do this.
Make compile again> Good.
Add jcsv dependency to fix IntelliJ compilation

Encontrar revisores

É costume o committer propor um ou dois revisores que estejam familiarizados com a base de código. Muitas vezes, um dos revisores é o chefe do projecto ou um engenheiro sénior. Os proprietários dos projectos devem considerar subscrever os seus projectos a fim de serem notificados de novos CR. As revisões de código entre mais de três partes são frequentemente improdutivas ou mesmo contraproducentes, uma vez que diferentes revisores podem propor alterações contraditórias. Isto pode indicar desacordo fundamental sobre a implementação correcta e deve ser resolvido fora de uma revisão de código num fórum de maior largura de banda, por exemplo pessoalmente ou numa videoconferência com todas as partes envolvidas.

Execução de revisões de código

Uma revisão de código é um ponto de sincronização entre diferentes membros da equipa e, portanto, tem o potencial de bloquear o progresso. Consequentemente, as revisões de código precisam de ser rápidas (na ordem de horas, não de dias), e os membros da equipa e os líderes precisam de estar conscientes do compromisso de tempo e priorizar o tempo de revisão em conformidade. Se achar que não consegue completar uma revisão a tempo, por favor informe imediatamente o revisor para que possa encontrar outra pessoa.

Uma revisão deve ser suficientemente completa para que o revisor possa explicar a mudança a um nível razoável de detalhe a outro programador. Isto assegura que os detalhes da base de código são conhecidos por mais do que uma única pessoa.

Como revisor, é da sua responsabilidade fazer cumprir as normas de codificação e manter a barra de qualidade elevada. A revisão do código é mais uma arte do que uma ciência. A única forma de o aprender é fazê-lo; um revisor experiente deve considerar colocar outros revisores menos experientes nas suas mudanças e pedir-lhes que façam uma revisão em primeiro lugar. Assumindo que o autor seguiu as directrizes acima (especialmente no que diz respeito à auto-revisão e à garantia da execução do código), eis uma lista de coisas a que um revisor deve prestar atenção numa revisão de código:

Propósito

• Este código cumpre o propósito do autor? Cada alteração deve ter uma razão específica (nova funcionalidade, refactor, correcção de bugs, etc.). O código submetido cumpre realmente este objectivo?
• Faz perguntas. As funções e classes devem existir por uma razão. Quando a razão não é clara para o revisor, isto pode ser uma indicação de que o código precisa de ser reescrito ou suportado com comentários ou testes.

Implementação

• Pense em como teria resolvido o problema. Se é diferente, porquê? O seu código lida com mais casos (de borda)? É mais curto/mais fácil/mais rápido/mais rápido/mais rápido/ferramentas equivalentes em termos funcionais? Existe algum padrão subjacente que tenha detectado que não seja capturado pelo código actual?
• Vê potencial para abstracções úteis? O código parcialmente duplicado indica frequentemente que uma funcionalidade mais abstracta ou geral pode ser extraída e depois reutilizada em contextos diferentes.
• Pense como um adversário, mas seja simpático com isso. Tente “apanhar” os autores a tomar atalhos ou casos em falta, apresentando configurações problemáticas/dados de entrada que quebram o seu código.
• Pense em bibliotecas ou código de produto existente. Quando alguém reimplementa a funcionalidade existente, na maioria das vezes é simplesmente porque não sabe que ela já existe. Por vezes, o código ou funcionalidade é duplicado de propósito, por exemplo, a fim de evitar dependências. Nesses casos, um comentário de código pode clarificar a intenção. A funcionalidade introduzida já é fornecida por uma biblioteca existente?
• A alteração segue os padrões padrão? As bases de códigos estabelecidas exibem frequentemente padrões em torno de convenções de nomenclatura, decomposição lógica de programas, definições de tipos de dados, etc. É normalmente desejável que as alterações sejam implementadas de acordo com padrões existentes.
• A alteração acrescenta dependências de tempo de compilação ou de tempo de execução (especialmente entre sub-projectos)? Pretendemos manter os nossos produtos pouco acoplados, com o menor número possível de dependências. As alterações às dependências e ao sistema de compilação devem ser objecto de um grande escrutínio.

Legibilidade e estilo

• Pense na sua experiência de leitura. Compreendeu os conceitos num período de tempo razoável? O fluxo era são e os nomes das variáveis e métodos eram fáceis de seguir? Foi capaz de acompanhar através de múltiplos ficheiros ou funções? Foi adiado por nomes inconsistentes?
• O código adere às directrizes de codificação e estilo de código? O código é consistente com o projecto em termos de estilo, convenções API, etc.? Como mencionado acima, preferimos resolver debates de estilo com ferramentas automatizadas.
• Este código tem TODOs? Os TODOs acumulam-se em código, e tornam-se obsoletos com o tempo. Peça ao autor que apresente um bilhete em GitHub Issues ou JIRA e anexe o número de emissão ao TODO. A alteração de código proposta não deve conter código comentado.

Manutenção

• Leia os testes. Se não houver testes e estes devem existir, peça ao autor para escrever alguns. As características verdadeiramente não testadas são raras, enquanto que as implementações de características não testadas são infelizmente comuns. Verifique os próprios testes: estão eles a cobrir casos interessantes? São legíveis? A CR reduz a cobertura global dos testes? Pense em formas como este código pode quebrar. Os padrões de estilo para testes são frequentemente diferentes do código principal, mas ainda assim importantes.
• Este CR introduz o risco de quebrar o código de teste, de encenação de pilhas, ou de testes de integração? Estes não são frequentemente verificados como parte das verificações pré-compromisso/fusão, mas fazê-los descer é doloroso para todos. Coisas específicas a procurar são: remoção de utilitários ou modos de teste, alterações na configuração, e alterações na disposição/estrutura do artefacto.
• Esta alteração quebra a compatibilidade com o passado? Em caso afirmativo, é correcto fundir a alteração neste ponto ou deve ser empurrada para uma versão posterior? As quebras podem incluir alterações na base de dados ou no esquema, alterações na API pública, alterações no fluxo de trabalho do utilizador, etc.
• Este código precisa de testes de integração? Por vezes, o código não pode ser adequadamente testado apenas com testes unitários, especialmente se o código interage com sistemas ou configurações externas.
• Deixe feedback sobre documentação de nível de código, comentários, e mensagens de compromisso. Comentários redundantes desordenam o código, e mensagens de commit terse mistificam os futuros contribuidores. Isto nem sempre é aplicável, mas os comentários de qualidade e as mensagens de submissão pagar-se-ão a si próprios ao longo da linha. (Pense numa altura em que viu uma excelente, ou verdadeiramente terrível, mensagem ou comentário de commit.)
• A documentação externa foi actualizada? Se o seu projecto mantém um README, CHANGELOG, ou outra documentação, foi actualizado para reflectir as alterações? A documentação desactualizada pode ser mais confusa do que nenhuma, e será mais dispendioso corrigi-la no futuro do que actualizá-la agora.

Não se esqueça de elogiar código conciso/leitura/eficiente/elegante. Inversamente, declinar ou desaprovar uma CR não é rude. Se a alteração for redundante ou irrelevante, decline-a com uma explicação. Se a considerar inaceitável devido a uma ou mais falhas fatais, desaprová-la, mais uma vez com uma explicação. Por vezes o resultado correcto de uma RE é “façamos isto de uma forma totalmente diferente” ou mesmo “não façamos isto de todo”

Seja respeitoso para com os revisores. Embora o pensamento contraditório seja útil, não é a sua característica e não pode tomar todas as decisões. Se não conseguir chegar a um acordo com o seu revisor com o código tal como está, mude para comunicação em tempo real ou procure uma terceira opinião.

Segurança

Verifiquem se os parâmetros API executam a autorização e autenticação adequadas, consistentes com o resto da base de código. Verificar outros pontos fracos comuns, por exemplo, configuração fraca, entrada de utilizadores maliciosos, falta de eventos de registo, etc. Em caso de dúvida, encaminhar o CR para um especialista em segurança da aplicação.

Comentários: conciso, amigável, accionável

Reevisões devem ser concisas e escritas em linguagem neutra. Critique o código, não o autor. Quando algo não estiver claro, pedir esclarecimentos em vez de assumir a ignorância. Evite pronomes possessivos, em particular em conjunção com avaliações: “o meu código funcionou antes da sua mudança”, “o seu método tem um erro”, etc. Evite julgamentos absolutos: “isto nunca pode funcionar”, “o resultado está sempre errado”.

Tente diferenciar entre sugestões (por exemplo, “Sugestão: extrair método para melhorar a legibilidade”), alterações necessárias (por exemplo, “Adicionar @Override”), e pontos que necessitam de discussão ou esclarecimento (por exemplo, “Será este realmente o comportamento correcto? Em caso afirmativo, queira acrescentar um comentário que explique a lógica”). Pense em fornecer ligações ou indicações para explicações aprofundadas de um problema.

Quando terminar uma revisão do código, indique em que medida espera que o autor responda aos seus comentários e se gostaria de rever a CR depois de as alterações terem sido implementadas (por exemplo, “Isto é realmente o comportamento correcto”), “Sinta-se à vontade para se fundir depois de responder às poucas sugestões menores” vs. “Por favor, considere as minhas sugestões e avise-me quando puder dar outra vista de olhos”).

Responder às revisões

Parte do objectivo da revisão do código é melhorar o pedido de alteração do autor; consequentemente, não se ofenda com as sugestões do seu revisor e leve-as a sério mesmo que não concorde. Responda a cada comentário, mesmo que seja apenas um simples “ACK” ou “feito”. Explique porque tomou certas decisões, porque existe alguma função, etc. Se não conseguir chegar a um acordo com o revisor, mudar para comunicação em tempo real ou procurar uma opinião externa.

Fixes devem ser empurrados para o mesmo ramo, mas num compromisso separado. O esmagamento dos commits durante o processo de revisão torna difícil para o revisor acompanhar as alterações.

Equipas diferentes têm diferentes políticas de fusão: algumas equipas permitem apenas a fusão dos proprietários do projecto, enquanto outras equipas permitem que o contribuinte se funda após uma revisão positiva do código.

Revisões de código pessoalmente

Para a maioria das revisões de código, as ferramentas assíncronas baseadas na difusão, tais como Reviewable, Gerrit ou, GitHub são uma grande escolha. Alterações complexas, ou revisões entre partes com conhecimentos ou experiência muito diferentes podem ser mais eficientes quando realizadas pessoalmente, quer em frente do mesmo ecrã ou projector, ou remotamente via VTC ou ferramentas de partilha de ecrã.

Exemplos

Nos exemplos seguintes, os comentários de revisão sugeridos são indicados por //R: ... comentários nos blocos de código.

Nomeação inconsistente

class MyClass {
private int countTotalPageVisits; //R: name variables consistently
private int uniqueUsersCount;
}

Assinaturas do método inconsistente

interface MyInterface {
/** Returns {@link Optional#empty} if s cannot be extracted. */
public Optional<String> extractString(String s); /** Returns null if {@code s} cannot be rewritten. */
//R: should harmonize return values: use Optional<> here, too
public String rewriteString(String s);
}

O uso da biblioteca

//R: remove and replace by Guava's MapJoiner
String joinAndConcatenate(Map<String, String> map, String keyValueSeparator, String keySeparator);

Sabor pessoal

int dayCount; //R: nit: I usually prefer numFoo over fooCount; up to you, but we should keep it consistent in this project

Bugs

//R: This performs numIterations+1 iterations, is that intentional?
// If it is, consider changing the numIterations semantics?
for (int i = 0; i <= numIterations; ++i) {
...
}

Problemas arquitectónicos