O objetivo do review de um Staff Engineer é o desenvolvimento do time. Achar bug é a base da atividade, e ferramenta já pega parte disso antes de o PR abrir. O que separa esse review de um review comum é o que você escreve no comentário e o que você escolhe deixar passar.

Quatro anti-patterns que enfraquecem o review

Nitpick de estilo. Espaço, aspa, ordem de import: você tem linter e CI reclamando daquilo a cada push. O padrão do reviewer no Google Engineering Practices coloca estilo e convenção de código automática abaixo de correção e design. Comentário de estilo gasta a atenção do autor para resolver o que ferramenta resolve sozinha.

Reescrever o código no comentário. "Eu faria assim:" seguido de vinte linhas impõe a sua solução e ensina a pessoa a copiar. Um comentário com o problema e a restrição leva o autor a produzir a solução dele.

Aprovar sem ler. LGTM em trinta segundos protege o prazo e transfere o risco para produção. Dizer "esta semana não consigo revisar" é mais honesto e mais rápido para quem espera.

Bloquear por gosto. "Isso viola nosso padrão de tratamento de erro" é princípio com razão registrada em algum lugar do time. "Eu prefiro Promise a async/await" é preferência. O mesmo documento do Google separa os dois e pede que comentário bloqueante aponte risco concreto ou violação de convenção acordada.

O comentário que transfere raciocínio

Comentário curto obriga o autor a adivinhar o motivo. Comentário com contexto devolve o motivo junto com o pedido, e o próximo leitor do PR aprende o raciocínio também.

// fraco
"Use memoização aqui"

// forte
"Esse componente re-renderiza a cada mudança de estado do pai, mesmo quando
a prop `items` não muda. useMemo nesse cálculo evita re-render em lista
grande. Contexto: discutimos isso na issue #847, tela de pedidos."

A versão forte entrega causa, efeito e referência histórica. O guia de comentários do Google pede isso: explique o porquê e sugira o caminho, em vez de apontar o defeito e sumir.

Peso explícito de cada comentário

  • Bloqueador: imped merge. Segurança, correção, quebra de contrato de API. Nomeie o risco na frase.
  • Sugestão: recomenda e não trava. "Extrair isso para helper deixaria teste mais fácil, mas manter inline também resolve."
  • Nit: opinião leve, marcada com o prefixo nit: que o guia do Google adotou. Autor ignora sem discussão.
  • Pergunta: você quer entender a decisão. "Por que Map aqui e não objeto literal? Quero seguir o mesmo critério no módulo de billing."

A categoria no começo da frase elimina a maior parte dos mal-entendidos. Autor que recebe um "nit:" sabe que pode seguir sem responder.

Elogio com nome do que funcionou

Quando o código traz uma escolha boa, diga qual é a escolha. "Gostei" não diz nada para o próximo leitor; "discriminated union aqui torna estado inválido inrepresentável, vou usar no módulo de pagamento" ensina dois engenheiros ao mesmo tempo.

// "O discriminated union aqui torna impossível representar estado inválido.
// Vou usar a mesma ideia no módulo de pagamento."

Feedback específico nos dois sentidos cria um ambiente em que o PR deixa de ser fila de obstáculo.

Quando a thread passa de quatro respostas

Discussão assíncrona com mais de quatro respostas custa mais caro que a chamada de quinze minutos que resolve. Chame a pessoa, alinhe, e escreva a decisão no PR para quem chegar depois. O capítulo 9 de Software Engineering at Google trata review como conversa de engenharia e recomenda CL pequeno justamente para a discussão caber no texto.

Arquitetura entra antes do código. Um design doc curto, comentável enquanto trocar é barato, evita o PR de oitocentas linhas que ninguém consegue revisar de verdade.

O que você deixa passar de propósito

Código que funciona, é mantível e não viola princípio nenhum recebe seu OK mesmo sendo diferente do que você escreveria. Você tem contexto que o autor não tem; o autor tem contexto que você não tem. Bloquear PR acima do necessário cobra velocidade do time e devolve pouca coisa em troca.

O documento do Google sobre o que procurar em uma review lista design, funcionalidade, complexidade, testes, nomes e comentários nessa ordem. Bug entra em funcionalidade; desenvolver time entra no comentário que você escreve.

Referências