Cultura de revisão de código: por que você precisa ter uma

Quantas vezes você cruza os dedos ao abrir uma solicitação de pull na esperança de que ela não seja atribuída àquele desenvolvedor em sua equipe que sempre deixa um punhado de comentários sobre tudo e qualquer coisa? Não estou falando daqueles muito razoáveis que revelam erros cometidos ou, por favor, não, code smell.

Esses são comentários sobre seu estilo de código ou algumas pequenas coisas subjetivas que não visam melhorar a base de código no final, mas sim dar a sensação de "Eu teria feito de outra forma porque simplesmente gosto mais". Como um colega de equipe responsável, é provável que você os aborde.

Não importa qual opção você decida seguir — apenas aplicar todas as mudanças ou recuar com longas explicações sobre o porquê de você não concordar com isso e aquilo — você provavelmente acabará se sentindo agitado, exausto e frustrado com o tempo gasto em coisas não essenciais. E realmente valeu a pena? Essa pergunta surgirá na sua cabeça toda vez que você lidar com tais situações.

Ou às vezes, você tem o outro lado da moeda — o insta aprova sem comentários e sem sinais de que um revisor realmente verificou cuidadosamente suas alterações. Se eles revisaram, e não há entradas objetivas, ou não, você fica questionando a si mesmo e a esse colega de equipe.

Certo, mesmo que tudo esteja bem, não seria legal incluir uma rápida menção ao autor e garantir que vocês estejam na mesma página, sem deixar dúvidas?

Se ambas as histórias acima parecem muito familiares, este artigo é para você. O que sua equipe sente falta é de uma cultura de pull request ou, em outras palavras, um conjunto de diretrizes sobre como se comunicar em pull requests para dar suporte a um processo de colaboração amigável e altamente eficiente. Abordarei as peças essenciais que os revisores gostariam de procurar e também exemplos de comentários questionáveis — mais simples, picuinhas — que criam atrito ou, às vezes, até mesmo problemas além de seus sentimentos pessoais.

Como desenvolvedor iOS, usarei Swift em meus exemplos de código, mas, em geral, esse tópico é importante para qualquer desenvolvedor, não importa a plataforma ou linguagem que você usa diariamente.

No entanto, acredito que a cultura de revisão de código é ainda mais relevante para plataformas Apple porque, na minha experiência, tendemos a ser mais exigentes em muitos aspectos. Provavelmente vem de uma mentalidade perfeccionista herdada da visão de Steve nos velhos tempos.

Lição aprendida

Pull request ou merge request é uma maneira de verificar alterações de código com seus colegas e verificar se há erros, avaliar sua prontidão antes de ir para os próximos estágios e, finalmente, para os usuários finais. É também um dos principais canais de comunicação entre desenvolvedores. Às vezes, podemos nem conhecer uma pessoa fora dos threads em PRs.

Muitas vezes, especialmente em estágios iniciais de carreira, os desenvolvedores não prestam atenção à sua comunicação de RP. Especificamente, como seus comentários podem ser percebidos por outras pessoas, se os pontos levantados são claros e corretamente escritos em inglês, e assim por diante.

Acredite, eu já passei por isso e também cometi erros no passado. Uma experiência em particular ficou comigo até hoje. Eu a menciono quase sempre durante as discussões sobre cultura de RP porque ela destaca perfeitamente a importância deste tópico.

Anos atrás, quando eu estava procurando um novo emprego, fui desafiado com uma solicitação de pull simulada que eu precisava revisar. Não pensei em tudo cuidadosamente naquela época porque estava animado por ter recebido uma tarefa fácil em vez de outra tortura do Leetcode. Claro, não foi tão fácil assim. Foi um teste sobre cultura de solicitação de pull em que falhei drasticamente.

Dei uma olhada rápida no PR e deixei um monte de comentários, em grande parte inúteis, por exemplo:

 import UIKit import AVFoundation // It would be nice to list your imports alphabetically, so that it's easier to read.

Ou outro:

 func someMethod() { // about 30 lines of code } // What do you think about splitting this method in half?

Em outros comentários raros onde eu identifiquei um problema, eu não fui claro o suficiente sobre ele e não forneci nenhuma solução razoável. Mas a principal falha desse exercício foi que eu perdi alguns problemas de desempenho porque meu foco não estava no lugar certo. No final, recebi o seguinte feedback:

Senti que você deixou passar muitos detalhes importantes, como correção do código, desempenho e melhorias gerais (com algumas pequenas exceções).

Você focou muito da revisão na formatação, que deve ser uma discussão em equipe e/ou fazer uso de ferramentas de formatação. Isso também permite que os desenvolvedores se concentrem nas coisas certas em uma revisão.

Desde então, percebi que minha abordagem para revisões de código estava longe de ser perfeita e carecia de muitas coisas importantes.

Este exemplo é bastante primitivo de certa forma, mas mostra claramente como a falta de uma cultura de RP pode facilmente se tornar um caminho perigoso que leva diretamente a bugs e problemas na produção.

Vamos nos aprofundar nos detalhes e começar com algo que você deve evitar como revisor de código: escrever comentários minuciosos.

O que é um comentário chato?

Há muitas maneiras de expressar sua lógica e pensamentos em código. Linguagens de programação modernas oferecem versatilidade suficiente para isso e, frequentemente, não há certo ou errado entre várias abordagens. No entanto, não queremos que nossos projetos pareçam monstros de Frankenstein — cada equipe deve ter um estilo de código decente com diretrizes.

Ao mesmo tempo, não podemos e não devemos controlar cada linha de código dos nossos companheiros de equipe. Acredito que encontrar um bom equilíbrio entre os dois é uma virtude pela qual idealmente queremos lutar.

Um comentário nitpick é uma solicitação para fazer uma alteração, geralmente pequena, que não tem razões objetivas para fazê-la. Frequentemente, é uma projeção de uma preferência pessoal de um revisor de código no código revisado. Deixe-me mostrar alguns exemplos disso:

 // Original guard let newValue, newValue != oldValue else { return } // Change request guard let newValue, newValue != oldValue else { return }

Se você me perguntar, eu sempre escolheria a segunda opção porque é mais fácil, na minha opinião, depurar esse código e colocar um ponto de interrupção bem na linha de retorno. Mas você pode argumentar que a primeira variante economiza 2 linhas, além de poder ver esse estilo dos engenheiros da Apple também.

 // Original func handleErrorIfNeeded(_ error: SomeError) { if error == .failedRequest { return } if error == .validationFailed { state.errorMessage = “Data input is incorrect” return } // … other cases } // Change request func handleErrorIfNeeded(_ error: SomeError) { guard error != .failedRequest else { return } guard error != .validationFailed { state.errorMessage = “Data input is incorrect” return } // … other cases }

Em muitas situações lógicas, guard e if em Swift podem ser usados de forma intercambiável. No entanto, eu diria que eles não têm a mesma semântica de linguagem. guard , como o nome indica, quer “proteger” seu fluxo de código de um resultado indesejado, enquanto if é neutro em sua natureza e permite uma chance de 50-50% de diferentes caminhos de código acontecerem. Ambas as variantes tecnicamente não estão erradas e são perfeitamente legíveis, no entanto.

E a última:

 // Original, the constant value is not being repeated anywhere else func calculateDuration(endDuration: Duration) -> Duration { let startDuration = Duration.milliseconds(130) // … calculation return finalDuration } // Change request func calculateDuration(endDuration: Duration) -> Duration { let startDuration = Duration.milliseconds(Constant.startDuration) // … calculation return finalDuration } enum Constant { static let startDuration = Double(130) }

Você pode argumentar sobre a necessidade de extrair qualquer constante em um namespace separado se ela for usada apenas uma vez, e quando não for uma constante mágica. Na primeira variante, você sabe claramente qual é esse número de 130. No entanto, alguém dirá para você extrair qualquer constante como essa, não importa o que.

Todos os exemplos acima são mudanças de nitpick. Ambas as variantes são perfeitamente boas, legíveis por qualquer um e não quebram seu código. Elas são meramente alternativas com o mesmo peso em uso.

Então, o que você quer fazer com comentários como esses? Da perspectiva de um revisor, alguns dirão algo assim:

Bem, é apenas uma sugestão. Não é obrigatório aplicá-la.

Verdade, mas ao mesmo tempo é importante deixar o autor saber disso, por exemplo:

É apenas uma sugestão, fique à vontade para descartá-la, mas o que você acha dessa abordagem: … ?

No entanto, minha opinião sobre nitpicks é não postá-los de jeito nenhum . Por quê?

Digamos que você enviou um comentário de nitpick, até mesmo destacou que é uma sugestão de baixa prioridade. O que acontece depois?

• 1ª opção: o autor aplica a mudança. O que isso realmente significa nos bastidores?
  • Eles precisam salvar as alterações atuais que estão fazendo.
  • Eles precisam mudar para o ramo de RP em seguida.
  • Eles precisam aplicar a mudança.
  • Eles podem resolver conflitos.
  • Eles precisam forçar a mudança.
  • Então, ele acionará mais uma rodada de testes de CI por causa do novo código enviado.
  • Eles precisam responder ao comentário com texto ou emoji.

De um lado, é um conjunto muito simples de tarefas que fazemos todos os dias, como desenvolvedores. Mas imagine só, não é um comentário de nitpick, mas 10. Ou você tem 5 pull requests abertas ao mesmo tempo com nitpicks de quantidades diferentes. Realmente valeu a pena seu tempo e o incômodo? Quando sabemos o que é um nitpick, a resposta é não. Eu preferiria entregar minhas alterações mais rápido e economizar tempo.

• 2ª opção: o autor não aplica a alteração.

  • Em muitas situações, é simplesmente rude ignorar o comentário do seu colega de equipe sem responder nada. E pode parecer rude também responder com:

    
    

    Obrigado pela sugestão, mas não gosto dela. Não vou aplicá-la, pois não acho necessário.

  
  

  • Em vez disso, como autor, você provavelmente irá se aprofundar mais na explicação dos detalhes do porquê você não aplicará as mudanças. Por exemplo, sua visão sobre guard vs let como acima.

    
    

  • Então, seria ótimo se o revisor simplesmente aceitasse sua opinião e seguisse em frente. Mas eles também podem sentir a resistência e responder porque realmente sentem que o jeito deles é melhor.

    
    

  • Tais tópicos podem facilmente sair da proporção com longos comentários de um lado para o outro, o que não é produtivo de forma alguma. E não vai a lugar nenhum no final. Vocês dois podem concordar em discordar, não há "vencedor", mas a pergunta final é a mesma. Realmente valeu a pena seu tempo e o incômodo?

  
  

Minha prática como revisor é filtrar essas pequenas críticas nas revisões de código antes mesmo de postar qualquer coisa, me perguntando estas perguntas:

É realmente importante mudar?

Há alguma razão objetiva para isso? Se sim, cite-a.

Eu quero postar esse comentário só porque eu teria feito diferente? Ou há uma preocupação real?

Dessa forma, você pode remover o “ruído” de comentários minuciosos da sua revisão de código e deixar apenas as partes importantes, aquelas partes em que todos vocês precisam se concentrar em escalabilidade, robustez, segurança de threads, etc.

Mas por que tais comentários aparecem? Geralmente, é um sinal de que os revisores não têm um entendimento claro do que focar ou procurar em pull requests. Na próxima seção, discutiremos exatamente isso.

Como criar uma cultura de revisão de código

Vamos ver como podemos criar um modelo de um “padrão ouro” de RP. Vou listar um conjunto de valores e diretrizes que são essenciais na minha opinião sobre a maneira de melhorar suas revisões de código:

1. Para todos: Defina um conjunto de buckets ou categorias para seus comentários de revisão de código e atribua a eles diferentes níveis de gravidade. Sempre mostre exemplos para seus colegas, para que todos estejam na mesma página. Exemplo:

…E assim por diante. Pode ser diferente na sua equipe, mas a ideia aqui é ter uma estrutura e classificação. Trazer valores de severidade nos permite priorizá-los melhor e focar no essencial.

2. Revisor : Tente alocar um intervalo de tempo específico para suas revisões de código. Não apresse as coisas e permita-se mergulhar mais fundo no significado das mudanças de código, não na sintaxe. Isso deve melhorar a qualidade e a clareza dos seus comentários.

   
   

3. Autor: Ajude seus colegas. Se você sabe que há algumas partes que podem não estar claras por causa da falta de contexto ou porque seu PR é apenas uma parte de uma série com coisas por vir, deixe um comentário com uma explicação sobre sua própria solicitação de pull com antecedência. Isso economizará tempo para ambos os lados!

   
   

4. Revisor: releia seus comentários antes de postar. Coloque-se no lugar do outro lado. Está tudo claro? Tente manter a seguinte estrutura:

   1. Descreva a sua preocupação.
   2. Por que isso é importante.
   3. O que você sugere fazer em vez disso?

   
   

5. Para todos: Verifique sua gramática e ortografia em inglês. Sim, para muitos de nós, o inglês não é nossa língua nativa, e às vezes é algo difícil de fazer. Mas se não for bem escrito, pode levar a todos os tipos de confusão e desalinhamentos que você não quer ter.

   
   

6. Para todos: Como seu comentário é percebido? Ele parece amigável? Este ponto é bem parecido com o anterior, mas a ideia é manter uma abordagem saudável e amigável na forma escrita. Por exemplo:

   
   

   Você deve extrair essa lógica em um método separado.

   
   

   Embora não haja nada de errado gramaticalmente com esta frase, a palavra “must” raramente é usada neste contexto e pode ser tratada como um comando para um leitor. Autoridades do governo dirão a você o que você deve fazer porque há leis. Mas não é o tipo de fraseado que você quer usar com seus colegas quando vocês estão no mesmo time. Tente isto em vez disso:

   
   

   O que você acha de extrair essa lógica em um método separado?

   
   

7. Revisor: Equilibre o negativo com o positivo. Se você deixou um punhado de comentários críticos, encontre alguns bons e destaque-os também. É uma coisa pequena, mas melhora a percepção geral aos olhos do autor.

   
   

8. Revisor: Se tudo estiver bem nas mudanças de código, não tenha medo de postar elogios. Aprovar sem sinais no Insta não é um bom comportamento e deixa muito a ser questionado. Poste um grito com as coisas exatas que você achou ótimas no trabalho do seu colega. Um simples “LGTM” é muito genérico e pode ser percebido como desdenhoso.

Acho que esses são bons pontos para começar com sua equipe para trabalhar em direção a uma comunicação melhor e mais saudável em suas solicitações de pull. A parte mais difícil depois disso é continuar seguindo essas diretrizes. Às vezes, pode ser dois passos para frente e um passo para trás.

Conclusão

No final, a codificação nem sempre é a parte mais difícil. Comunicação e colaboração podem ser mais desafiadoras, especialmente quando todos vocês vêm de vários países e origens com suas próprias experiências e pensamentos.

Espero que esta visão sobre a cultura de revisão de código tenha sido útil e que talvez você tire algo útil dela.

Boa sorte e revisões de código amigáveis para todos vocês!