- Revisão de código parece uma boa ideia, certo?
- Com a revisão de código, dois desenvolvedores olham o código e têm a chance de encontrar problemas e compartilhar entendimento sobre o processo de evolução do projeto
- O revisor pode aprender truques úteis ao olhar o código do autor em detalhe, ou encontrar oportunidades de ensinar truques úteis ao autor
- Mas essa é a forma como os revisores de código do "lado luz" a utilizam. Eles usam a revisão de código para melhorar o código e elevar coletivamente a capacidade técnica dos desenvolvedores
- A revisão de código também pode ser uma ferramenta poderosa para objetivos completamente diferentes. Se o revisor migrar para o "lado sombrio", pode usar vários métodos para atrapalhar ou atrasar a melhoria do código
- Também pode perseguir outros objetivos pessoais, como atormentar o autor do patch ou deixá-lo completamente frustrado
- Se você se converteu recentemente ao "lado sombrio", talvez ainda não tenha pensado em todas as possibilidades
- Por isso, este texto apresenta uma lista de antipadrões para revisores de código do lado sombrio
[Antipadrões]
The Death of a Thousand Round Trips
- Ao ler o código, assim que encontra algo trivial, aponta isso em um comentário de revisão e interrompe a leitura imediatamente
- O desenvolvedor corrige diligentemente essa trivialidade e envia o patch revisado
- O revisor começa a ler essa versão e encontra outra trivialidade. Poderia ter mencionado isso na primeira revisão, mas não o fez. Aponta a trivialidade e interrompe a leitura de novo
- Repita isso até que o desenvolvedor perca as esperanças
The Ransom Note
- Este patch parece ser especialmente importante para o desenvolvedor
- Mas para o revisor ele não é tão importante. Portanto, o revisor está em uma posição de poder
- É possível manter a mudança desejada pelo desenvolvedor como refém até que trabalhos adicionais importantes para o revisor sejam concluídos
- Esses trabalhos extras não precisam de fato estar no mesmo commit, mas são importantes para o revisor
The Double Team
- Um patch, dois revisores
- Sempre que um revisor exige uma mudança, o desenvolvedor a faz obedientemente. Então chega a vez do outro revisor reclamar
- Os revisores se alternam apresentando exigências conflitantes
- Mas sempre comentam direcionando-se ao desenvolvedor. Evitam discutir diretamente entre si na thread da revisão
- A ideia é ver quantas vezes os revisores conseguem jogar o desenvolvedor de um lado para o outro até ele desistir
The Guessing Game
- Existe um problema, e há várias maneiras de resolvê-lo
- O desenvolvedor escolhe uma solução e envia o patch
- O revisor critica essa solução específica por motivos que não têm relação com ela resolver ou não o problema
- Usa como justificativa uma suposta violação de princípios vagos de design ou incompatibilidade com planos futuros de desenvolvimento
- Se a crítica for vaga o suficiente, o desenvolvedor não consegue saber como deveria alterar o patch atual para resolvê-la
- Nesse caso, a melhor saída para o desenvolvedor é escolher uma solução totalmente diferente para o problema original e implementá-la no lugar
- Então o revisor rejeita isso novamente da mesma maneira inútil
The Priority Inversion
- Na primeira revisão de código, aponta coisas triviais e simples
- Espera o desenvolvedor corrigi-las e então levanta um problema grave
- Há um problema fundamental que exige reescrever completamente uma parte substancial do patch. Isso significa descartar grande parte do trabalho trivial de correção que já foi feito
- Poucas coisas transmitem tão bem a mensagem "não queremos o seu trabalho, e o seu tempo não tem valor" quanto fazer alguém trabalhar bastante e depois jogar isso fora
- Só isso já pode ser suficiente para o desenvolvedor desistir
The Late-Breaking Design Review
- Um trabalho extremamente complexo vem sendo desenvolvido ao longo de semanas ou meses em muitos patches separados
- O revisor não concorda com o design desse trabalho como um todo, mas não participou da discussão original ou estava do lado que perdeu no debate
- Aí pedem ao revisor que examine uma parte pequena, mas importante, desse trabalho em andamento, como uma correção fácil para um bug que está bloqueando muitos desenvolvedores. Isso é uma oportunidade para o revisor
- O revisor passa a exigir uma justificativa para o design de todo o trabalho feito até ali
- Se o desenvolvedor for responsável apenas por parte do trabalho total e não souber responder, isso não é problema do revisor. Ele não vai apertar o botão de "aprovar" até ficar satisfeito
The Catch-22
- Se for um patch grande, é difícil demais de ler. Exige que ele seja dividido em partes menores autocontidas
- Por outro lado, se houver muitos patches pequenos, alguns deles não fazem sentido por si só. Exige que sejam juntados de novo
- Sempre que algum tipo de trade-off parecer relevante para um caso específico, isso pode ser explorado como motivo para reclamar
- Por exemplo, se o código estiver escrito de forma fácil de entender, então o desempenho será ruim; se estiver bem otimizado, será difícil de ler e de manter
The Flip Flop
- Existe um tipo de mudança que as pessoas normalmente fazem na mesma parte do código
- O revisor já revisou esse tipo de mudança várias vezes antes
- Chega mais uma mudança. O autor examinou cuidadosamente as alterações anteriores, seguiu com atenção o padrão existente e escolheu um revisor que parece familiarizado com essa área
- O revisor de repente questiona um aspecto da mudança que nunca havia sido problema antes. Seguir o padrão existente já não basta
- O que acontece se o autor mostrar mudanças anteriores iguais e apontar a inconsistência do revisor?
- O revisor diz: "Sim. Isso também deveria ser alterado"
- O revisor deve tomar cuidado para não se oferecer para corrigir todos os casos existentes. Com sorte, o desenvolvedor vai interpretar isso como uma instrução para alterar ele mesmo todos os casos anteriores, poupando muito esforço ao revisor
Mas, falando sério...
- Até aqui, este texto foi uma piada. Também não estou sugerindo que revisores ajam assim de propósito
- A maior parte das descrições é um exagero do que revisores realmente fazem, ou um exagero de como um autor frustrado de patch pode se sentir
- Na prática, a ideia é: não faça essas coisas!
- Tente minimizar os round trips, peça reescritas importantes antes de implicar com detalhes pequenos e, ao criticar um patch, faça sugestões construtivas sobre que versão você aceitaria
- Se você tiver opiniões sobre o codebase como um todo, em vez de implicar com o patch de um único desenvolvedor durante a revisão, discuta isso de forma mais ampla com todos os desenvolvedores
- Se você fez esse tipo de coisa sem querer, tenha consciência disso. Reconheça que tornou a vida de um desenvolvedor mais difícil sem intenção, peça desculpas e se esforce para ser mais útil
- O problema fundamental é o abuso de poder
- Quando um desenvolvedor se torna revisor do patch de outro, ele passa a ter um poder temporário. O revisor tem autoridade para impedir que aquele patch seja commitado
- Poder vem com responsabilidade. Ele deve ser usado para o propósito pretendido e apenas na medida necessária
- A maioria dos antipadrões (ou dos comportamentos leves satirizados aqui) é abuso de poder. O revisor usa sua autoridade temporária sobre o desenvolvedor como alavanca para perseguir agendas pessoais não relacionadas à melhoria do patch, ou até contrárias a ela
- As agendas pessoais variam conforme o antipadrão: tarefas sem relação, preferências pessoais de estilo, preguiça, resistência a mudanças, antipatia pessoal pelo autor do patch etc.
- Se o autor do patch já agiu assim no passado quando era revisor de código, talvez a antipatia seja justificada. Por isso, o poder do revisor de código deve ser usado com cuidado
- Se os desenvolvedores entrarem em um ciclo de vingança uns contra os outros, o projeto de software estará condenado
Revisão de código como gatekeeping
- Até aqui, o foco foi revisão de código entre pares
- O revisor de código e o autor do patch são colegas; nenhum dos dois responde ao outro nem tem decisão final sobre o codebase
- Por isso, o poder obtido numa revisão de código é temporário
- Em uma situação de revisão entre pares, revisor e autor deveriam ter basicamente os mesmos objetivos
- Se houver divergências sérias sobre quais funcionalidades incluir, quanto polimento é necessário antes da aprovação ou qual estilo de código é aceitável, isso deve ser tratado fora do contexto da revisão de código
- Mas em outros tipos de revisão de código, não é assim. Isso é especialmente diferente quando alguém de fora do projeto envia um patch não solicitado
- Esse cenário normalmente acontece em software livre
- Porque qualquer pessoa no mundo pode modificar o código-fonte e algumas tentarão mandar essas mudanças de volta
- Mas isso também pode acontecer em outras situações
- Dentro de uma empresa que desenvolve código proprietário, um desenvolvedor de uma equipe pode enviar um patch para o projeto de outra equipe e torcer para ser aceito
- Nessas situações, é muito mais provável que quem recebe o patch simplesmente não queira aquela mudança desde o início, ou discorde completamente de como ela foi feita, e portanto talvez não a aceite de forma alguma
- Nesse caso, não se trata de abuso de um poder temporário concedido ao revisor como colega, mas do exercício legítimo de um poder permanente como responsável pelo projeto
- Eu decido a direção do meu projeto de software
- Quando o revisor atua nesse papel de "gatekeeping", nem sempre é errado criticar um patch por violar princípios gerais de design ou requisitos já existentes
- Talvez ele realmente não saiba como resolver aquele problema de um jeito compatível com os requisitos
- Na verdade, essa pode ser justamente a parte difícil, e talvez o único motivo de eu mesmo ainda não ter feito a mesma mudança
- Porém, mesmo no contexto de gatekeeping, aplicar "The Guessing Game" sem explicação é falta de educação
- Em geral, tento explicar ao desenvolvedor qual parte difícil ele deixou passar e, se nem eu souber a resposta, digo isso claramente
- É claro que não há desculpa para obstrução passivo-agressiva como "The Death of a Thousand Round Trips"
- Se você realmente não quer aquele patch no código de jeito nenhum e está numa função de gatekeeping com autoridade legítima para tomar essa decisão, pode simplesmente dizer isso em palavras para que o autor não perca mais tempo
Disclaimer
- Ao longo de anos, fui reunindo anotações para este artigo a partir de revisões de código das quais participei (dos dois lados), revisões que observei entre outras pessoas e revisões sobre as quais só ouvi falar em conversas
- Portanto, isto não é direcionado a nenhuma pessoa específica que tenha revisado meu código recentemente
13 comentários
Surpreendentemente, talvez nem seja exagero....
Isso eu realmente vivi, e quase desisti de ser desenvolvedor; foi muito difícil me reerguer de verdade.
Quase tive PTSD lendo o texto
Parece que você vinha reunindo muito bem as anotações para este artigo até agora!!
Só de ler já parece abuso psicológico...
Então a essência está na última linha, né? kkk.....
Uau... achei que fosse ter um troço;;
Esse tipo de coisa, se você for a lugares que fazem SI + SM, é algo que dá para ver com frequência também no Brasil. Costumam chamar isso de panelinha. Tem muita gente mesquinha fazendo de tudo para proteger o próprio ganha-pão.
Há muitos jeitos perversos.
No longo prazo, quem faz esse tipo de coisa sem um motivo razoável acaba, ou 1) sendo excluído cedo da rede de contatos entre desenvolvedores, ou 2) mantendo esse estado porque, apesar da personalidade péssima, é tão competente que fica com uma fatia grande das responsabilidades e é difícil de excluir, então alguém assume o papel de adaptador e vira a ponte de comunicação para manter a conexão; se essa pessoa intermediária desaparece por qualquer motivo, não demora muito para ser excluído. Por mais que alguém se esforce ao máximo, no fim das contas o trabalho e o dinheiro só circulam quando as pessoas se juntam para fazer alguma coisa, e, se o dinheiro circula, as pessoas também circulam; por isso, quem não consegue se dar bem com os outros acaba sendo excluído do grupo e ficando para trás.
Em geral, quem sobrevive por muito tempo dentro de um grupo apesar de ter um caráter ruim costuma se enganar achando que continua ali porque é algum tipo de figura extraordinária, como um sociopata de alta funcionalidade saído de um drama tipo Sherlock, mas na prática os outros só suportam e usam essa pessoa porque ela ainda tem utilidade; quando essa utilidade desaparece, a relação vira algo como "foi péssimo conviver com você e espero nunca mais te ver". Até o Sherlock protagonizado pelo Cumberbatch é, para nós que assistimos de fora, apenas um sociopata carismático; se não houvesse gente ao redor que se recusasse a desistir dele e continuasse se importando com ele, simplesmente não haveria história alguma.
As pessoas que sobreviveram por muito tempo apesar de terem um péssimo caráter costumam cair na ilusão de que sobreviveram porque são algo grandioso, tipo um Sherlock de drama, um sociopata de alta funcionalidade; mas, na verdade, é só que ao redor aguentam em silêncio e as usam porque ainda têm utilidade. Quando essa utilidade acaba, a relação vira um "foi horrível trabalhar com você e espero nunca mais te ver" ==> É uma frase impressionante. Preciso guardar isso.
Lembra assédio no trabalho, ou melhor, pawahara.
Dizem que é humor, mas lendo o texto a irritação vem com tudo..