Como geralmente ocorre uma revisão de código? Você envia uma solicitação de pool, recebe feedback, faz correções, envia correções para uma segunda revisão, obtém aprovação e mesclagem. Parece simples, mas, na realidade, o processo de revisão pode levar muito tempo.
Imagine que você tenha uma solicitação de pool com centenas de linhas de mudança. O revisor deve gastar muito tempo para ler completamente o código e entender as alterações propostas. Como resultado, todo o processo, desde a criação de uma solicitação de pool até sua aprovação, pode levar vários dias - isso não é muito agradável para o revisor e o autor das alterações. E as chances são boas de que, no final, o revisor ainda perca alguma coisa. Ou a verificação pode ser muito superficial e, na pior das hipóteses, a solicitação de pool pode ser rejeitada imediatamente.
Acontece que, quanto maior a solicitação de pool, menor será o benefício de verificá-la.
Como evitar tais situações? Como tornar a solicitação de pool mais fácil e compreensível para o revisor e otimizar todo o processo?
Estamos traduzindo um artigo do nosso desenvolvedor de back-end Sergey Zhuk sobre como funciona o processo de revisão de código da equipe de aplicativos móveis Skyeng.
Alterar categorias
Vamos imaginar que você tenha uma tarefa - implementar novas funcionalidades no projeto. A solicitação de pool em que você está trabalhando pode conter diferentes categorias de alterações. Claro que haverá algum código novo nele. Porém, no decorrer do trabalho, você pode perceber que algum código precisa ser refatorado previamente, para que contribua para a adição de novas funcionalidades. Ou com essa nova funcionalidade, a duplicação apareceu no código que você deseja eliminar. Ou, de repente, você encontrou um erro e deseja corrigi-lo. Como deve ser a solicitação final do pool?
Primeiro, vamos ver quais categorias de alterações podem ocorrer com o código.
- Mudanças funcionais.
- Refatoração estrutural - alterações em classes, interfaces, métodos, movimento entre classes.
- Refatoração simples - pode ser executada usando o IDE (por exemplo, extraindo variáveis / métodos / constantes, simplificando condições).
- Renomeando e movendo classes - reorganizando o espaço para nome.
- Removendo código (inoperante) não utilizado.
- Estilo de código de correções.
Revisão da Estratégia
É muito importante entender que cada uma dessas categorias é revisada de maneira diferente e, ao considerá-las, o revisor deve se concentrar em coisas diferentes. Pode-se dizer que cada categoria de mudança envolve sua própria estratégia de revisão.
- Mudança funcional: solução de problemas de negócios e design de sistemas.
- Refatoração estrutural: compatibilidade com versões anteriores e aprimoramento do projeto.
- Refatoração primitiva: melhor legibilidade. Essas alterações podem ser feitas principalmente usando o IDE (por exemplo, extraindo variáveis / métodos / constantes etc.).
- Renomeando / movendo classes: a estrutura do namespace melhorou?
- Remoção de código não utilizado: compatível com versões anteriores.
- Estilo de código de correções: na maioria das vezes, a solicitação de mesclagem de pool acontece imediatamente.
Diferentes abordagens são usadas para verificar alterações em diferentes categorias; portanto, a quantidade de tempo e esforço gastos em sua revisão também varia.
Mudanças funcionais. Esse é o processo mais longo, pois envolve a alteração da lógica do domínio. O revisor procura verificar se o problema foi resolvido e verifica se a solução proposta é a mais adequada ou pode ser melhorada.
Refatoração estrutural. Esse processo requer muito menos tempo que as alterações funcionais. Mas pode haver sugestões e desacordos sobre como exatamente o código deve ser organizado.
Ao verificar as categorias restantes em 99% dos casos, a mesclagem ocorre imediatamente.
- Refatoração simples. O código ficou mais legível? - Merzhim.
- Renomeando / movendo classes. A classe foi movida para um espaço de nome melhor? - Merzh.
- A remoção de código não utilizado (inativo) é merzhim.
- Correções de estilo ou formatação do código - merzh. Seus colegas não devem verificar isso durante a revisão do código, esta é a tarefa do linter.
Por que as alterações devem ser categorizadas?
Já discutimos que diferentes categorias de alterações são revisadas de maneira diferente. Por exemplo, verificamos alterações funcionais com base nos requisitos de negócios e, na refatoração estrutural, verificamos a compatibilidade com versões anteriores. E se misturarmos várias categorias, será difícil para o revisor manter várias estratégias de revisão em mente ao mesmo tempo. E, provavelmente, o revisor passará mais tempo na solicitação de pool do que o necessário e, por causa disso, poderá perder alguma coisa. Além disso, se a solicitação de pool contiver vários tipos de alterações, com qualquer correção, o revisor terá que revisar todas essas categorias novamente. Por exemplo, você misturou refatoração estrutural e alterações funcionais. Mesmo se a refatoração for executada bem, mas houver um problema com a implementação do funcional, após as correções, o revisor terá que examinar toda a solicitação de pool desde o início. Ou seja, verifique novamente a refatoração e as alterações funcionais. Portanto, o revisor gasta mais tempo na solicitação de pool. Em vez de contemplar imediatamente uma solicitação de pool separada com refatoração, o revisor deve revisar novamente o código inteiro.
O que exatamente não vale a pena misturar
Renomear / excluir uma classe e sua refatoração. Aqui nos deparamos com o Git, que nem sempre entende corretamente essas mudanças. Quero dizer mudanças em larga escala quando muitas linhas mudam. Quando você refatora uma classe e depois a move para algum lugar, o Git não a percebe como móvel. Em vez disso, o Git interpreta essas alterações como removendo uma classe e criando outra. Isso leva a várias perguntas durante a revisão do código. E o autor do código é perguntado por que ele escreveu esse código feio, embora na verdade esse código tenha sido simplesmente movido de um lugar para outro com pequenas alterações.
Qualquer alteração funcional + qualquer refatoração. Já discutimos esse caso acima. Isso faz com que o revisor tenha em mente duas estratégias de revisão ao mesmo tempo. Mesmo se a refatoração for bem-sucedida, não poderemos adiar essas alterações até que as alterações funcionais sejam aprovadas.
Quaisquer alterações mecânicas + quaisquer alterações feitas pelo homem. Por "alterações mecânicas", quero dizer qualquer formatação feita usando o IDE ou geração de código. Por exemplo, aplicamos o novo estilo de código e obtemos 3000 alterações de linha. E se misturarmos essas alterações com quaisquer alterações funcionais ou outras feitas por uma pessoa, forçaremos o revisor a classificar mentalmente essas alterações e o motivo: é uma alteração feita por um computador - pode ser ignorada e é necessária uma alteração necessária. confira. Assim, o revisor passa muito tempo verificando.
Exemplo
Aqui está uma solicitação de pool com uma função de método que fecha suavemente a conexão do cliente com o Memcached:
Mesmo que a solicitação de pool seja pequena e contenha apenas cem linhas de código, ainda é difícil verificar. Como você pode ver nos commits, ele contém várias categorias de mudanças:
- funcional (novo código),
- refatoração (criação / movimentação de classes),
- estilo de código de correções (remoção de excesso de blocos de encaixe).
Ao mesmo tempo, a nova funcionalidade em si é de apenas 10 linhas:
Como resultado, o revisor deve revisar todo o código e
- Verifique se a refatoração está OK.
- verifique se a nova funcionalidade está implementada corretamente;
- determine se essa alteração foi gerada automaticamente pelo IDE ou pela pessoa.
Portanto, é difícil revisar essa solicitação de pool. Para corrigir a situação, você pode dividir essas confirmações em ramificações separadas e criar um conjunto de solicitações para cada uma delas.
1. Refatoração: extração de classe
Existem apenas dois arquivos. O revisor deve verificar apenas o novo design. Se tudo estiver em ordem - merzhim.
2. O próximo passo também é refatoração, apenas movemos as duas classes para um novo espaço para nome
Essa solicitação de pool é bastante simples de verificar; pode ser instanciada imediatamente.
3. Removendo o excesso de blocos de encaixe
Nada interessante aqui. Merzhim.
4. O funcional em si
E agora a solicitação de pool com alterações funcionais contém apenas o código desejado. Portanto, seu revisor pode se concentrar apenas nessa tarefa. O pedido de piscina é pequeno e fácil de verificar.
Conclusão
Regra prática:Não crie solicitações de pool enormes com categorias mistas de alterações.
Quanto maior a solicitação de pool, mais difícil é para o revisor entender as alterações que você propôs. Provavelmente, uma enorme solicitação de pool com centenas de linhas de código será rejeitada. Em vez disso, divida-o em pequenas partes lógicas. Se a sua refatoração estiver em ordem, mas as alterações funcionais contiverem erros, a refatoração poderá ser facilmente retida e, assim, você e seu revisor poderão se concentrar na funcionalidade sem examinar todo o código desde o início.
E sempre siga estas etapas antes de enviar a solicitação de pool:
- Otimize seu código para leitura. O código é muito mais legível do que escrito;
- Descreva as alterações propostas para fornecer o contexto necessário para a sua compreensão;
- sempre verifique seu código antes de criar uma solicitação de pool. E faça isso como se fosse o código de outra pessoa. Às vezes, ajuda a encontrar algo que você perdeu. Isso reduzirá a probabilidade de rejeição da sua solicitação de pool e o número de correções.
Meu colega
Oleg Antonevich me contou sobre a ideia de dividir as mudanças em categorias.
Do editor: Sergey escreve muitas coisas interessantes sobre programação e PHP, e às vezes traduzimos algo: um servidor de streaming de vídeo , renderizando arquivos HTML . Sinta-se à vontade para fazer perguntas nos comentários deste artigo - ele responderá!
Bem, lembramos também que sempre temos muitas vagas interessantes para desenvolvedores!