Revisão de Código: Experiência de Sucesso

Há muitas informações na Internet sobre revisões de código:

• como uma revisão do código de outra pessoa afeta a cultura da empresa
• Verificações de segurança regulamentares
• manuais resumidos
• longas listas de recomendações
• revisão de código de uma perspectiva interpessoal
• por que preciso de uma revisão de código
• métodos comprovados
• mais sobre métodos comprovados
• statistics: quanto a revisão de código ajuda a detectar bugs
• exemplos de erros cometidos durante a revisão de código

Sim, é claro, existem livros sobre esse assunto. Em uma palavra, este artigo descreve como a revisão de código é organizada pela Palantir. Nas organizações cuja cultura não aceita essa revisão por pares, pode ser útil ler primeiro o brilhante ensaio de Karl E. Wiegers, The Code Revision with Human Face, e depois tentar seguir este guia.

Este texto foi retirado das recomendações de melhoria da qualidade com base no trabalho com o Baseline , nossa ferramenta para controle de qualidade do código Java. Abrange os seguintes tópicos:

• Por que, o que e quando tentamos obter a revisão de código
  
• Preparando o código para revisão
  
• Execução de revisão de código
  
• Exemplos de revisão de código
  

Traduzido para Alconost


XKCD Comic 'Code Quality', copiado sob licença CC BY-NC 2.5

Motivação

Estamos envolvidos na revisão de código (RC) para melhorar a qualidade do código e, assim, queremos influenciar positivamente a equipe e a cultura corporativa. Por exemplo:

• Os autores do código estão motivados porque sabem que sua equipe de revisores considerará sua solicitação de alterações: o autor tenta limpar a aspereza, seguir o plano de ação e geralmente melhorar todo o comprometimento. Quando os colegas reconhecem seu profissionalismo ao escrever código - para muitos programadores, essa é uma ocasião para se orgulhar.
  
• Compartilhar conhecimentos ajuda a equipe de desenvolvimento de várias maneiras:
  

- No RK, é claramente relatado qual parte da funcionalidade foi adicionada / alterada / excluída para que, no futuro, os membros da equipe possam confiar no trabalho já realizado.

- O autor do código pode usar uma técnica ou algoritmo no qual os próprios revisores aprenderão algo. No geral, uma revisão de código ajuda a elevar os padrões de qualidade em toda a organização .

- Os revisores podem saber algo sobre técnicas de programação ou a própria base de código que ajudará a melhorar ou consolidar as alterações feitas; por exemplo, alguém pode ao mesmo tempo desenvolver ou modificar exatamente a mesma oportunidade.

- O contato e a comunicação positivos reforçam os laços sociais entre os membros da equipe.

• Devido à consistência na base de código, o código em si é muito mais fácil de ler e entender. É mais fácil evitar bugs e promover a colaboração entre programadores estabelecidos e nômades.
  
• É difícil para o autor do código julgar a legibilidade de sua própria criação, e para o revisor isso é fácil, porque ele não vê em que contexto esse código está localizado. O código legível por humanos é mais fácil de reutilizar, possui menos erros e é mais durável.
  
• Erros aleatórios (por exemplo, erros de digitação), bem como erros estruturais (por exemplo, código não executável, erros de lógica ou algoritmos, problemas de desempenho e arquitetura) geralmente são muito mais fáceis de serem detectados por um revisor exigente que olha o código de lado. Segundo a pesquisa, mesmo uma revisão curta e não oficial do código afeta significativamente a qualidade do código e a ocorrência de bugs .
  
• Os requisitos de conformidade e as estruturas regulatórias geralmente exigem revisões obrigatórias. Uma revisão de código é uma ótima maneira de evitar problemas de segurança comuns. Se os requisitos de segurança neste ambiente ou em relação a esta oportunidade forem aumentados, a revisão será recomendada (ou mesmo solicitada) por satraps das autoridades reguladoras (um bom exemplo é o guia da OWASP ).

O que procurar

Não há uma resposta clara para essa pergunta e cada equipe de desenvolvimento precisa concordar com sua própria abordagem. Algumas equipes preferem verificar as alterações feitas no ramo principal de desenvolvimento, enquanto outras levam em consideração o "limiar de trivialidade", além do qual a verificação é obrigatória. Nesse caso, é necessário encontrar um equilíbrio entre o uso eficiente do tempo do programador (o autor e o revisor do código) e a manutenção da qualidade do código. Em alguns contextos estritos, às vezes é necessário verificar tudo, mesmo as alterações mais triviais, no código.

As regras de revisão de código são as mesmas para todos: mesmo se você for o desenvolvedor mais sênior da equipe, isso não significa que seu código não exija uma revisão. Mesmo que (às vezes isso aconteça) o código seja perfeito, a revisão abre oportunidades para orientação e cooperação e, pelo menos, ajuda a analisar a base de código sob diferentes pontos de vista.

Quando verificar

Uma revisão de código deve ocorrer após a conclusão bem-sucedida das verificações automáticas (testes, design, outros estágios de integração contínua), mas antes da mesclagem do novo código com a ramificação do repositório principal. Normalmente, não recorremos à verificação formal do total de alterações feitas no código desde a última versão.

Para alterações complexas que devem ser adicionadas à ramificação principal do código como uma única unidade, mas tão extensas que não podem ser realmente cobertas em uma verificação, tente uma revisão em fases. Criamos o principal recurso / ramificação de grande recurso e vários secundários (por exemplo, feature / big-feature-api, feature / big-feature-testing, etc.), cada qual encapsula um subconjunto da funcionalidade comum e cada uma dessas ramificações verificado no ramo principal do recurso / recurso grande. Depois que todas as ramificações secundárias forem mescladas em feature / big-feature, crie uma revisão de código para injetar a última na ramificação principal.

Preparando o código para revisão

O autor do código é obrigado a fornecer o código para a revisão de forma digerível, para não dedicar mais tempo aos revisores e não desmotivá-los:

• Escopo e tamanho . As mudanças devem estar relacionadas a um escopo estreito, bem definido e auto-suficiente, totalmente coberto pela revisão. Por exemplo, as alterações podem estar relacionadas à implementação de algum recurso ou correção de bug. Mudanças breves são preferíveis às longas. Se a revisão contiver material que inclui alterações em mais de ~ 5 arquivos ou exigir mais de um a dois dias para gravar, ou se a revisão em si demorar mais de 20 minutos, tente dividir o material em vários fragmentos independentes e verificar cada um separadamente. Por exemplo, um desenvolvedor pode enviar uma alteração que define a API para um novo recurso em termos de interfaces e documentação e, em seguida, adicionar uma segunda alteração que descreve as implementações para essas interfaces.
  
• Você precisa enviar apenas materiais completos, testados independentemente (use diff) e testados independentemente . Para economizar o tempo do revisor, teste as alterações enviadas (ou seja, execute o conjunto de testes) e verifique se o código passa em todos os assemblies, assim como em todos os testes e todos os estágios do controle de qualidade, localmente e em servidores de integração contínua, e somente então selecione revisores .
  
• As mudanças na refatoração não devem afetar o comportamento e vice-versa; alterações que afetam o comportamento não devem envolver refatoração ou reformatação do código. Existem várias boas razões para isso:
  

- As alterações relacionadas à refatoração geralmente afetam muitas linhas de código em muitos arquivos - portanto, esse código não será verificado com o mesmo cuidado . Alterações não planejadas no comportamento podem vazar para a base de código e ninguém nem notará.

- As principais mudanças associadas à refatoração violam os mecanismos de movimento, seleção e outras "mágicas" associadas ao controle de origem. É muito difícil desfazer uma alteração no comportamento que foi feita após a conclusão da refatoração que cobria todo o repositório.

- O tempo de trabalho caro que uma pessoa gasta na revisão do código deve verificar a lógica do programa, e não em disputas sobre o estilo, sintaxe ou formatação do código. Preferimos resolver esses problemas usando ferramentas automatizadas, em particular, Checkstyle , TSLint , Baseline , Prettier , etc.

Confirmar mensagens

A seguir, é apresentado um exemplo de uma mensagem de confirmação competente, projetada de acordo com o padrão amplamente utilizado:

 ( 80 )    ,  ,   .   72 .          ,    —   .   ,       (   );     ,  rebase,    .  -   : « »,   « »   « ».      -,  , ,  git merge  git revert.      .    . 

Tente descrever as alterações feitas durante a confirmação e como exatamente essas alterações são feitas:

 > .   .    . > .  jcsv-     IntelliJ 

Pesquisar revisores

Normalmente, o autor de uma confirmação procura um ou dois revisores familiarizados com a base de código. Freqüentemente nessa capacidade estão o gerente de projeto ou o engenheiro sênior. É recomendável que o proprietário do projeto assine seu próprio projeto para receber notificações de novas revisões de código. Com a participação de três ou mais revisores, uma revisão de código geralmente se mostra improdutiva ou contraproducente, pois diferentes revisores podem propor alterações mutuamente conflitantes. Isso pode indicar desacordo fundamental sobre a implementação correta, e esses problemas não devem ser resolvidos durante a revisão do código, mas em uma reunião prolongada em que todas as partes interessadas participem pessoalmente ou no modo de videoconferência.

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

Uma revisão de código é um ponto de sincronização entre diferentes membros da equipe, portanto, potencialmente, pode parar o trabalho. Portanto, a revisão do código deve estar operacional (leva várias horas, não dias), e os membros e líderes da equipe devem estar cientes de quanto tempo levará para verificar e priorizar o tempo alocado de acordo. Se lhe parecer que você não tem tempo para concluir a revisão a tempo, informe imediatamente o autor do commit para que ele possa encontrar um substituto para você.

A revisão deve ser bastante completa, para que o revisor possa explicar a alteração para outro desenvolvedor com detalhes suficientes. Portanto, garantiremos que os detalhes da base de código sejam conhecidos por pelo menos dois, e não um.

Você, como revisor, deve aderir aos padrões de qualidade de código e mantê-lo em um nível alto. Uma revisão de código é mais uma arte do que uma ciência. Isso só pode ser aprendido na prática. Um revisor experiente deve tentar primeiro permitir que colegas menos experientes mudem e que eles sejam os primeiros a revisar. Se o autor seguiu as regras acima (especialmente aquelas relacionadas a autoteste e execução preliminar de código), é nisso que o revisor deve prestar atenção ao revisar:

Finalidade

• O código alcança a meta estabelecida pelo autor? Qualquer alteração deve ter um motivo específico (novo recurso, refatoração, correção de bug etc.). O código proposto realmente nos leva a esse objetivo?
  
• Faça perguntas. Funções e classes devem ser justificadas. Se sua atribuição ao revisor não for clara, talvez isso signifique que o código deve ser reescrito ou suportado por comentários ou testes.
  

Implementação

• Pense em como você resolveria esse problema. Se não, então por quê? Seu código lida com mais casos (borderline)? Talvez seja mais curto / mais fácil / mais limpo / mais rápido / mais seguro e funcionalmente não seja pior? Talvez você tenha percebido alguma regularidade profunda que não é coberta pelo código existente?
  
• Você vê o potencial para criar abstrações úteis? Se o código for parcialmente duplicado, isso geralmente significa que um elemento mais abstrato ou geral do funcional pode ser extraído dele, que pode ser reutilizado em outros contextos.
  
• Pense como um oponente, no entanto, fique bem. Tente "capturar" os autores que cortam os cantos ou perdem casos específicos, lançando configurações problemáticas / dados de entrada que quebram seu código.
  
• Pense em bibliotecas ou código de trabalho pronto. Se alguém reimplementar a funcionalidade existente - isso não é apenas porque ele não sabe sobre a existência de uma solução pronta. Às vezes, o código ou a funcionalidade são intencionalmente reinventados - por exemplo, para evitar dependências. Nesse caso, isso deve ser claramente comentado no código. Essa funcionalidade já é fornecida em alguma biblioteca existente?
  
• A mudança se encaixa nos padrões padrão? Nas bases de código existentes, as regularidades são frequentemente rastreadas relacionadas a convenções de nomenclatura, decomposição da lógica do programa, definições de tipos de dados etc. É geralmente desejável que todas as alterações sejam implementadas de acordo com os padrões existentes.
  
• As dependências que ocorrem durante a compilação ou no tempo de execução (especialmente entre subprojetos) são adicionadas durante uma alteração? Nós nos esforçamos pela fraca coerência do código de nossos produtos, ou seja, tentamos permitir um mínimo de dependências. As alterações relacionadas às dependências e ao sistema de compilação devem ser verificadas especialmente com cuidado.
  

Legibilidade e estilo

• Pense em como você lê este código. Você pode entender seus conceitos com rapidez suficiente? É razoavelmente definido, é fácil acompanhar os nomes de variáveis ​​e métodos? Posso rastrear código em vários arquivos ou funções? Você já se incomodou com nomes inconsistentes em algum lugar?
  
• O código está em conformidade com as conhecidas diretrizes de programação e estilo? O código está quebrando todo o projeto por estilo, convenções de nomenclatura de API etc.? Como mencionado acima, tentamos resolver disputas estilísticas usando ferramentas automáticas.
  
• O código possui listas de tarefas (TODOs)? As listas de tarefas simplesmente se acumulam no código e eventualmente se transformam em lastro. Atribua ao autor o envio de um ticket para o GitHub Issues ou JIRA e anexe um número de tarefa ao TODO. Não deve haver código comentado na alteração proposta.
  

Suporte de usabilidade

• Leia os testes. Se não houver testes no código, mas eles devem estar lá, peça ao autor para escrever alguns. Recursos verdadeiramente não testados são raros, e implementações de recursos não testadas - infelizmente, frequentemente. Verifique os próprios testes: eles cobrem casos interessantes? É conveniente lê-los? A cobertura geral do teste diminui após este teste? Pense em como esse código pode falhar. Os padrões de design de teste e o código principal geralmente são diferentes, mas os padrões de teste também são importantes.
  
• A revisão desse fragmento representa um risco de quebrar o código de teste, o código de invasão ou os testes de integração? Freqüentemente, essa revisão não é feita antes da consolidação / mesclagem, mas se tais casos forem negligenciados, todos sofrerão. Preste atenção às seguintes coisas: excluir utilitários ou modos de teste, alterar a configuração, alterações no layout / estrutura do artefato.
  
• Essa alteração viola a compatibilidade com versões anteriores? Em caso afirmativo, é possível fazer essa alteração no release neste estágio ou deve ser adiado para um release posterior? As violações podem incluir alterações no banco de dados ou em seu esquema, alterações nas APIs públicas, alterações no fluxo de tarefas no nível do usuário etc.
  
• Esse código requer testes de integração? Às vezes, o código falha na verificação adequada usando apenas testes de unidade, especialmente se ele interage com sistemas ou configurações externas.
  
• Deixe um feedback sobre a documentação deste código, faça comentários e envie mensagens. Comentários extensos obstruem o código e significam que as mensagens de confirmação confundem aqueles que mais tarde precisam trabalhar com o código. Nem sempre é esse o caso, mas os comentários de qualidade e as mensagens de confirmação ao longo do tempo certamente serão úteis. (Lembre-se de quando você viu um comentário maravilhoso ou simplesmente horrível ou enviou uma mensagem.)
  
• A documentação externa foi atualizada? Se o README, CHANGELOG ou outra documentação for mantida para o seu projeto, ela será atualizada para refletir as alterações feitas? A documentação desatualizada pode ser mais prejudicial do que nenhuma, e será mais caro corrigir erros no futuro do que fazer alterações agora.
  

Não se esqueça de elogiar o código conciso / legível / eficiente / bonito. Pelo contrário, rejeitar ou rejeitar um código proposto para revisão não é rude. Se a mudança for excessiva ou insignificante - rejeite-a, explicando o motivo. Se a alteração lhe parecer inaceitável devido a um ou vários erros fatais - rejeite-a novamente com razão. Às vezes, o melhor resultado de uma revisão de código é dizer: "Vamos fazer isso de maneira completamente diferente" ou "Não vamos fazer nada".

Respeito pelos pares. Embora a posição do oponente seja sólida, você não percebeu essa oportunidade e não pode decidir tudo por ele. Se você não conseguir obter uma opinião revisada por pares sobre o código, organize uma consulta em tempo real e ouça a opinião de uma terceira pessoa.

Segurança

Verifique se todos os pontos de extremidade da API estão adequadamente autorizados e autenticados, de acordo com as regras adotadas no restante da base de códigos. Procure outras falhas comuns, por exemplo: configuração fraca, entrada maliciosa do usuário, entradas de log ausentes etc. Em caso de dúvida, mostre o código revisado por um profissional de segurança.

Comentários: conciso, educado, incentivo

A revisão deve ser concisa, sustentada em um estilo neutro. Critique o código, não o autor. Se algo não estiver claro - peça esclarecimentos e não assuma que tudo esteja na ignorância do autor. Evite pronomes possessivos, especialmente em julgamentos de valor: “ meu código funcionou antes de suas alterações”, “existe um erro no seu método” etc. Não corte o ombro: “não vai funcionar”, “o resultado está sempre incorreto”.

Tente distinguir entre recomendações (por exemplo, “Recomendação: extraia o método, isso tornará o código mais claro”), alterações necessárias (por exemplo, “Adicionar substituição ”) e opiniões que precisam de esclarecimento ou discussão (por exemplo, “Esse comportamento é verdadeiro? ? Se sim, por favor, adicione um comentário explicando a lógica. ”) Tente colocar links ou ponteiros para uma descrição detalhada do problema.

Ao concluir a revisão do código, indique o quão detalhada a reação aos seus comentários você espera do autor, você gostaria de repetir a revisão depois que as alterações forem feitas, por exemplo: “Você pode fazer o upload do código com segurança para o ramo principal após essas pequenas correções” ou “Observe meus comentários e me avise quando eu puder ver o código novamente. ”

Rever resposta

A revisão do código, em particular, visa melhorar a solicitação de alterações proposta pelo autor; portanto, não leve hostilidade às recomendações do revisor, leve-as a sério, mesmo que não concorde com elas. Responda a qualquer comentário, mesmo ao "ACK" usual (aprovado) ou "pronto" (pronto). Explique por que você tomou essa ou aquela decisão, por que você tem certas funções em seu código, etc. Se você não conseguir uma opinião comum com o revisor, agende uma consulta em tempo real e ouça a opinião de um especialista externo.

As correções devem ser corrigidas na mesma ramificação, mas em uma confirmação separada. Se você manter as confirmações na fase de revisão, será mais difícil para o revisor acompanhar as alterações.

A política de mesclagem de código é diferente para equipes diferentes. Em algumas empresas, a mesclagem de código é confiável apenas pelo proprietário do projeto; em outras, um participante pode fazer isso após a revisão e aprovação do código.

Revisão do código Tête-à-tête

Como regra, ferramentas diff-assíncronas, como Reviewable, Gerrit ou GitHub, são ótimas para revisões de código. Se estivermos falando de uma revisão complexa ou de uma revisão, cujos participantes diferem muito em termos de experiência ou profissionalismo, pode ser mais eficaz conduzir uma revisão pessoalmente - sentados juntos em frente a um monitor ou projetor ou remotamente, por meio de ferramentas de videoconferência ou compartilhamento de tela.

Exemplos

Nos exemplos a seguir, os comentários do revisor nas listagens são inseridos usando

 //R: ... 

Nomenclatura inconsistente

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

Assinaturas de método inconsistentes

 interface MyInterface { /**  {@link Optional#empty},  s   . */ public Optional<String> extractString(String s); /**  null,  {@code s}   . */ //R:    :    // Optional<> public String rewriteString(String s); } 

Uso da biblioteca

 //R:     MapJoiner   Guava String joinAndConcatenate(Map<String, String> map, String keyValueSeparator, String keySeparator); 

Preferências pessoais

 int dayCount; //R: :   numFoo   fooCount;   ,          

Bugs

 //R:    numIterations+1 –    ? //    –     numIterations? for (int i = 0; i <= numIterations; ++i) { ... } 

Considerações arquitetônicas

 otherService.call(); //R: ,      OtherService.    ? 

Sobre o tradutor

O artigo foi traduzido por Alconost.

A Alconost localiza jogos , aplicativos e sites em 70 idiomas. Tradutores em idioma nativo, teste linguístico, plataforma em nuvem com API, localização contínua, gerentes de projeto 24/7, qualquer formato de recursos de string.

Também criamos vídeos de publicidade e treinamento - para sites que vendem, imagem, publicidade, treinamento, teasers, exploradores, trailers do Google Play e da App Store.

Mais detalhes