Como rejeitar uma revisão de código que você acredita ser desnecessária?

Estou em uma posição em que me pediram para revisar algum código que corrige um problema que não acredito que exista.

O fixador, que é mais experiente que eu, insiste que sua correção é necessária, mas parece não ser mais que sofisma em C ++ para mim. Parte do nosso processo de implantação é uma revisão de código e, como o segundo maior engenheiro de uma pequena empresa, devo revisar as alterações.

Acredito que os revisores são tão responsáveis ​​por alterações de código quanto o codificador original, e não estou disposto a aceitar a responsabilidade por essa alteração. Como você rejeitaria esta revisão?

Peça um caso de teste que falhe sem a alteração que obtiver sucesso com a alteração.

Se ele não pode produzir um, você usa isso como justificativa.

Se ele pode produzir um, você precisa explicar por que o teste é inválido.

Os revisores devem ser objetivos.

É claro que você formou uma opinião sobre o código em questão antes mesmo de revisá-lo, e parece que você e o consertador assumiram posições. Se for assim, você terá dificuldade em parecer objetivo e ainda mais difícil em ser objetivo. Nada disso ajuda no processo, e pode ser que a melhor e mais objetiva coisa que você possa fazer seja se curvar, alegando que está muito próximo do problema.

Considere uma abordagem de equipe.

Se não for possível remover a si mesmo, talvez você possa ter vários outros engenheiros revisando o código ao mesmo tempo. Ou eles concordam com você que o código deve ser rejeitado ou não. Se eles concordarem com você, não será mais apenas você e o consertador, e você poderá argumentar com mais força que a equipe examinou a correção objetivamente e decidiu não aceitá-la. Por outro lado, se eles decidirem aceitar a correção, isso também será uma decisão da equipe. Não é preciso dizer que você deve participar com a mente mais aberta possível e que não deve tentar influenciar as opiniões dos outros membros da equipe por qualquer coisa que não seja uma discussão racional. Importante: se houver um resultado ruim mais tarde, não jogue a equipe embaixo do ônibus dizendo "Bem, eu sempre disse que era um código incorreto, mas eu estava em menor número que os outros membros da equipe ".

Rejeições são uma parte natural do processo de revisão de código.

O processo de revisão de código não existe para correções de carimbo de pessoas mais velhas; existe para proteger e melhorar a qualidade do código. Não há nada de errado em rejeitar uma correção, desde que você a faça pelo motivo certo, ou seja, que a correção não melhore o código. Se, após uma revisão aberta do código, você ainda achar que a correção não reduz o risco e / ou a magnitude de um problema demonstrável, você deve rejeitá-la. Não é pessoal, apenas sua opinião honesta. Se o fixador não concordar, tudo bem também e, nesse ponto, torna-se um problema para a gerência descobrir. Apenas certifique-se de permanecer honesto, aberto e profissional.

A responsabilidade corta nos dois sentidos.

Você disse que não quer ser responsável por essa mudança, aparentemente porque não acredita que haja um problema. No entanto, é preciso perceber que, se você está errado e não é um problema, então você pode acabar sendo responsável por rejeitar o código que teria evitado o problema.

Faça anotações.

Manter um registro escrito do processo de revisão ajudará você a manter seus fatos corretos. Anote seus pensamentos e preocupações ao revisar, descrever e obter os resultados de quaisquer testes que você possa executar para medir o suposto problema e a correção etc. Se o problema persistir, você terá um registro do que fez para dar suporte ao seu problema. posição. Se o problema surgir novamente no futuro (provavelmente ocorrerá se o fixador estiver anexado à sua própria visão), você terá algo para movimentar sua memória.

Anote seus motivos de rejeição e defesas para prováveis ​​contra-argumentos. Em seguida, discuta racionalmente com o fixador e, se necessário, passe para o gerenciamento.

Se você tiver documentação suficiente (incluindo listagens de código, resultados de testes e argumentos objetivos ), estará bem coberto.

Você causará alguma má vontade com o fixador, portanto, verifique se o problema vale a pena (ou seja, a não correção causa dano?)

Além disso, se o fixador estiver relacionado aos proprietários, esqueça esta resposta.

Recentemente, nas notícias do LinkedIn, havia um artigo sobre resolução de conflitos no local de trabalho e ser humilde, em vez de parecer arrogante. Infelizmente, não consigo encontrar o link agora, mas o ponto principal era tentar fazer perguntas de uma maneira não conflituosa. Por favor, não tome isso como eu sugerindo que você é arrogante, é exatamente o modo como o artigo foi escrito.

De qualquer forma, dizer ao programador sênior que ele está errado em sua suposição apenas o levará a ter uma abordagem defensiva e a atacá-lo de volta em sua resposta. No entanto, se você colocar a questão de por que ele acredita que o problema existe, do ponto de vista de sua falta de entendimento, é provável que ele seja mais aberto a discutir o assunto.

Então, em vez de dizer "O problema não existe, então eu não deveria ter que revisar isso", talvez pergunte algo como "Para revisar isso corretamente, eu preciso entender o problema. Você pode, por favor, explicar ponto de vista?"

Como exemplo, o artigo descreveu um teste para crianças em que um adulto segurava um cubo cujos rostos eram todos de uma cor, exceto o que estava voltado para o adulto. Quando as crianças eram questionadas sobre a cor da face que o adulto estava vendo, as crianças com menos de 5 anos quase sempre diziam a cor que podiam ver, pois não entendiam que o adulto poderia ter uma visão diferente da sua.

C / C ++ pode estar cheio de comportamentos indefinidos. Por um lado, é ruim, pois pode levar a comportamentos inesperados. Por outro lado, permite otimização agressiva e, geralmente, quando você está usando C / C ++, está interessado em velocidade.

Escrever caso de teste com quebras pode ser difícil - pode envolver uma arquitetura ou compilador estranho que não existe mais. Também pode parecer que em qualquer arquitetura sensata "não deve causar problemas".

No entanto, em um ponto ou outro, você mudará de plataforma (digamos - você quer se tornar móvel para fazer a porta para o ARM ou deseja acelerar as coisas e usar a GPU). Nesse ponto, as coisas podem começar a quebrar e você precisará depurá-lo. Pode ser tão trivial quanto atualizar o compilador para uma versão mais recente (e você pode querer / precisar).

O código problemático era:

int d[16];

int SATD (void)
{
   int satd = 0, dd, k;
   for (dd=d[k=0]; k<16; dd=d[++k]) {
     satd += (dd < 0 ? -dd : dd);
   }
   return satd;
 }

Durante a última iteração d[++k] => d[++15] => d[16]é acessada. Como geralmente o próximo elemento era a memória legal (em termos de paginação, não modelo de memória C), mesmo os compiladores triviais não produziram nenhum executável com comportamento estranho. A única maneira de escrever o caso de teste era encontrar uma plataforma com exatamente o mesmo modelo de memória que o C (provavelmente VM).

No entanto, o gcc 4.8 pré -elas visto que d[++k]é executado em cada loop. Então, k < 16caso contrário, o acesso seria ilegal e a legalidade do programa alimentado para compilador faz parte do contrato. Portanto, a condição do loop é sempre verdadeira, considerando as suposições, portanto é um loop infinito. Isso pode parecer estranho, mas era uma otimização perfeitamente legal - a emissão system("dd if=/dev/zero of=/dev/sda"); system("format c:")também seria a substituição correta do loop. Existem maneiras mais sutis de escolher comportamentos. Por exemplo, durante uma palestra sobre Memória Transacional , lembro que o apresentador tentou várias vezes obter valor errado quando 2 threads aumentaram o mesmo valor.

No entanto, o C / C ++, ao contrário de algumas linguagens, é padronizado para que essa disputa possa ser feita com referência à fonte objetiva:

• Se você acha que isso não é um problema, tente prová-lo usando o padrão C / C ++ (ou seja, que leva a um valor e comportamento definidos)
• Se ele acha que isso é um problema, peça que ele o prove usando o padrão C / C ++ (isto é, leva a um valor ou comportamento indefinido)

Em geral, se sua equipe está escrevendo em C / C ++, é útil ter um padrão à mão - mesmo os especialistas da equipe podem encontrar algo estranho lá de vez em quando.

Isso parece mais um problema com a política da sua equipe do que com esta revisão específica. Quando eu trabalhava em uma grande loja de software em uma equipe de 9 pessoas, um revisor tinha poder de veto sobre o código, e esse era um padrão que todos respeitávamos. Nós éramos talentosos e experientes o suficiente - viz. "maduro", mas mais no sentido de "não infantil" do que "experiente" - que o líder da nossa equipe poderia razoavelmente esperar que pudéssemos sempre chegar a um acordo sem recorrer a discussões em um período de tempo prudente.

Simplesmente com base no seu idioma no seu post, posso avisá-lo que parece que você abordará essa situação de uma maneira que pode levar a um argumento desconcentrado. Talvez seja "masturbação intelectual", mas provavelmente existe uma razão pela qual ele fez isso, e o ônus sobre você será o de encontrar uma maneira mais simples de resolver esse problema - e, se não existir, documente no comentário o motivo. o código masturbatório era, de fato, necessário.

Faça com que o remetente mostre a prova de que o código dele resolve o problema. Se ele pode testar e mostrar provas, é você quem está errado e deve parar de reclamar e lidar com isso. Se ele não puder mostrar provas para apoiar sua reivindicação, há um problema e mostra que seu patch corrige o problema, então eu o enviava de volta ao tabuleiro.

É claro que poderia haver uma política interna sensível em torno disso. Mas é assim que eu iria (deliciosamente).