Code review em entrevista: priorize bugs e escreva comentários úteis

Em uma entrevista de code review, mostre como o bug muda o resultado para o usuário. Você também precisa mostrar uma entrada que o reproduz, explicar o impacto e escrever um comentário que permita ao autor agir. Uma lista de preferências de estilo pode ocupar a conversa enquanto um erro de isolamento passa despercebido.

Revise este catálogo privado de fornecedores, com paginação e cache em memória. O PR parece curto, mas mistura três problemas: uma chave que ignora o contexto da consulta, uma ordenação incompleta e um try/catch que não intercepta a rejeição assíncrona esperada. Comece por How do you perform a thorough code review? e organize sua resposta por risco demonstrável.

Limite da evidência: Google Engineering Practices fundamenta a forma de revisar e comentar; PostgreSQL e Node.js sustentam comportamentos técnicos relevantes. O PR, os dados e os comentários são exercícios originais, não material de uma empresa ou relatos de candidatos. Executamos treze checks em Node.js 26.9.0 com um repositório falso. Não executamos banco, servidor HTTP, autenticação ou teste de carga.

Defina o contrato antes de avaliar o diff

O catálogo tem produtos privados por tenant. A consulta recebe um tenant já autorizado por uma fronteira confiável; o exemplo não implementa essa autenticação. A categoria é uma string validada, sendo a string vazia o pedido de todas as categorias. Página começa em um, tamanho deve ser inteiro entre um e cinquenta e a ordenação é por preço crescente.

Os IDs são únicos dentro de cada tenant. A resposta inclui os itens da página e o total de itens compatíveis com o filtro. O total não é apenas a quantidade retornada naquela página. Os dados permanecem estáticos durante nossos testes; invalidação de cache após alterações e consistência entre páginas em momentos distintos ainda precisam de uma política.

Leia o trecho problemático sob esse contrato:

export async function oldList(db, cache, q) {
  const key = `${q.page}:${q.size}`;
  if (cache.has(key)) return cache.get(key);
  try {
    const result = db.query({...q, order:['price']});
    cache.set(key, result);
    return result;
  } catch (error) {
    return {items:[], total:0};
  }
}

db.query devolve uma Promise. O cache é compartilhado entre chamadas ao serviço. Esse escopo permite a colisão: se o cache fosse recriado para cada chamada, haveria outros problemas, mas não o reaproveitamento entre tenants demonstrado abaixo. Não atribua ao diff uma propriedade que você ainda não confirmou no contexto do projeto.

A orientação de Google Engineering Practices inclui funcionalidade, design, complexidade e testes. Aqui, a primeira pergunta é se o comportamento atende ao contrato e preserva os usuários envolvidos. Renomear q pode ajudar a leitura, mas não corrige o resultado devolvido à pessoa errada.

Priorize a colisão de cache pelo impacto

Considere quatro produtos sintéticos: A e B pertencem a t1, categoria books, preço 10; C pertence a t1, categoria tools, preço 20; D pertence a t2, categoria books, preço 30. A consulta t1/books/página 1/tamanho 1 retorna A e grava a chave 1:1. A consulta seguinte t1/tools/1/1 encontra essa chave e recebe A, embora A pertença a books.

O filtro tools deixa de ser aplicado porque o cache responde antes da consulta. A falha não depende de preço igual ou concorrência. Duas chamadas sequenciais bastam. Esse é um contraexemplo melhor que “a chave parece incompleta”: mostra uma entrada, o estado criado e a resposta incompatível com o pedido.

O mesmo cache responde A para uma consulta t2/books. Sob o contrato privado do exercício, isso cruza uma fronteira de dados entre tenants. Trate a correção como necessária antes da aprovação. A severidade exata depende da matriz da organização e do conteúdo exposto; não use um rótulo universal sem conhecer essas condições.

Um comentário útil na linha da chave seria: “Obrigatório — isolamento: depois de t1/books/1/1, t2/books/1/1 recebe A, que pertence a t1, porque ambas usam 1:1. Inclua o tenant autorizado e os campos que determinam a resposta na identidade do cache. Acrescente um teste que aqueça o cache com t1 e consulte t2.”

Para a categoria, aproveite a mesma causa sem abrir cinco comentários repetidos. Diga que books e tools também colidem e peça os dois contraexemplos. Um comentário explica a identidade incompleta do cache; dois testes mostram a categoria errada e a exposição entre tenants. O pedido de correção pode se limitar à chave e aos testes de regressão.

Uma representação explícita pode ser JSON.stringify([tenant, category, page, size, 'price-id-v1']). No contrato limitado do exercício, ela evita a ambiguidade de concatenar strings com separadores. Não transforma qualquer objeto arbitrário em uma chave canônica universal. Se existirem moeda, permissões por usuário, filtros adicionais ou locale que mudem a resposta, eles também entram na análise de identidade.

Demonstre por que ordenar só por preço não basta

A e B têm o mesmo preço. Ordenar apenas por price não define qual dos dois vem primeiro. Em nosso repositório falso, a primeira consulta devolve A antes de B e a segunda permite B antes de A. Com tamanho um, a primeira página contém A e a segunda também contém A; B desaparece da travessia.

Essa alternância foi construída para demonstrar uma ordem de empate permitida pelo contrato antigo. Não afirmamos que PostgreSQL alterna obrigatoriamente a cada chamada. A documentação de LIMIT e OFFSET pede uma ordenação única para obter subconjuntos previsíveis. Um empate sem critério adicional deixa a fronteira entre páginas incompleta.

No exercício, price, id fornece esse desempate porque ID é único dentro do tenant filtrado. O teste corrigido observa A na primeira página e B na segunda. Verifique também a direção da ordenação e como valores nulos seriam tratados se o domínio os permitisse. Nosso pacote contém preços presentes e IDs simples; não resolve esses contratos adicionais.

O comentário na consulta pode dizer: “Obrigatório — paginação: A e B custam 10; duas ordens válidas de empate produzem A nas páginas 1 e 2. Acrescente um desempate único compatível com o tenant, como price, id, e teste a travessia com preços iguais. Isso estabiliza o conjunto estático, mas não oferece um snapshot entre requisições.”

A última frase evita uma promessa excessiva. Com inserções ou alterações entre páginas, mesmo uma ordenação total pode deslocar offsets. Cursor e snapshot são decisões adicionais, com contratos e custos próprios. Não expanda automaticamente um PR pequeno para uma nova arquitetura; registre a limitação de dados mutáveis e confirme se o requisito exige resolvê-la neste PR.

Observe a rejeição onde ela realmente acontece

A documentação de erros do Node.js diferencia mecanismos de propagação e mostra tratamento de operações baseadas em Promise com await. No trecho antigo, db.query retorna uma Promise rejeitada e o try termina sem uma exceção síncrona. A rejeição chega ao chamador; o catch local não produz a lista vazia pretendida.

O cache ainda guarda essa Promise rejeitada. Na segunda chamada com a mesma chave, o serviço reutiliza o erro sem consultar o repositório novamente. Nossos checks observaram duas rejeições e somente uma chamada ao repositório. Esse comportamento é diferente de cachear uma resposta de sucesso e precisa de uma política deliberada se for desejado.

Mesmo que você acrescente await, transformar qualquer falha em {items:[], total:0} continua problemático. Uma indisponibilidade passa a parecer ausência de produtos. Um bug interno também some atrás de um resultado válido. O consumidor perde a chance de mostrar uma falha, tentar novamente conforme contrato ou acionar monitoramento.

Um comentário na atribuição seria: “Obrigatório — falha assíncrona: db.query devolve Promise; a rejeição não entra neste catch sem espera dentro do bloco. Além disso, a Promise rejeitada fica no cache. Aguarde a consulta, armazene apenas sucesso e preserve uma resposta de falha distinta de catálogo vazio. Cubra rejeição e a próxima tentativa.”

Nossa correção de demonstração mapeia Unavailable para status lógico 503 e outros erros para 500, sem devolver a mensagem interna. Esses números são um contrato do exercício, não respostas HTTP emitidas por um servidor. Registramos o tipo do erro para o teste; uma implementação real precisa de logs estruturados, correlação e proteção de dados adequados.

Peça o menor patch que preserva o comportamento necessário

O núcleo da alteração usa a identidade completa da consulta, aguarda o resultado e registra apenas sucesso no cache:

const key = JSON.stringify([
  q.tenant, q.category, q.page, q.size, 'price-id-v1'
]);
if (cache.has(key)) return {status:200, ...cache.get(key)};
try {
  const result = await db.query({...q, order:['price','id']});
  cache.set(key, result);
  return {status:200, ...result};
} catch (error) {
  log(error instanceof Error ? error.name : 'NonError');
  return {
    status:error instanceof Unavailable ? 503 : 500,
    error:'catalog_unavailable'
  };
}

Antes desse bloco, a função corrigida valida página e tamanho como inteiros nos limites combinados. Unavailable é uma classe de erro do exercício e log é uma dependência injetada. Tenant e categoria continuam sendo precondições validadas pelo chamador. O trecho não é um controlador HTTP completo nem deve ser apresentado como proteção de autenticação.

Os treze checks locais incluem cinco observações do comportamento antigo: colisão de categoria, colisão de tenant, repetição entre páginas, rejeição que escapa do catch e reutilização da Promise rejeitada. Oito verificações da versão corrigida cobrem filtros, tenant, desempate, hit de sucesso, paginação inválida, ausência de erro no cache e distinção de falha interna e rejeição com valor que não é Error.

Para uma indisponibilidade, duas chamadas à versão corrigida fazem duas consultas, porque não armazenamos o erro. Isso evita o erro persistente do exemplo, mas não controla uma tempestade de tentativas. Limites, backoff ou coordenação de chamadas simultâneas exigem outra análise. Não transforme um contraexemplo resolvido em uma garantia geral de resiliência.

O cache também armazena objetos mutáveis sem cópia e não tem TTL ou invalidação. Mantivemos dados estáticos e consumidores que não alteram a resposta para isolar os três achados. Se o produto permitir mudanças, documente a política de atualização e investigue mutação compartilhada. A revisão deve delimitar o que o patch resolveu e o que continua como requisito aberto.

Escreva o resumo e responda à discordância

A orientação de Google sobre comentários de review recomenda explicar o motivo e distinguir severidade. Nosso resumo pode dizer: “Solicito alterações por isolamento de tenant, paginação com empates e tratamento/cache de rejeições. Os contraexemplos usam quatro produtos e duas chamadas. Depois do patch, quero rever esses testes e confirmar o contrato de atualização do cache. Renomear q é sugestão de leitura, não bloqueio.”

Se o autor responder “isso nunca acontece em produção”, pergunte qual invariante impede a sequência demonstrada. Um cache realmente isolado por tenant mudaria a avaliação do primeiro achado; a chave ainda precisaria representar categoria e demais filtros. Atualize o comentário quando surgir contexto válido, em vez de defender a primeira opinião como posição pessoal.

Se pedirem uma aprovação parcial, nomeie o escopo revisado e as dependências pendentes. “Revisei a identidade do cache e os testes locais; autenticação e invalidação não estão cobertas” comunica mais que um “LGTM” abrangente. Evite sugerir que passar testes com fakes confirma constraints, planos de consulta ou comportamento distribuído de um banco real.

Estas questões ajudam a variar o contexto da revisão:

Continue com Review getEvents endpoint for readability, performance, scalability, security. Entregue três comentários com entrada reproduzível, impacto e pedido de mudança, seguidos de um resumo que diga o que impede a aprovação.

Sources and Further Reading

• Google Engineering Practices — What to look for in a code review.
• Google Engineering Practices — How to write code review comments.
• PostgreSQL — LIMIT and OFFSET.
• Node.js — Errors.