Peça um caso de teste que falhe sem a alteração que é bem-sucedida na alteração.
Se ele não pode produzir um, você usa isso como justificativa.
Se ele puder produzir um, você precisará explicar por que o teste é inválido.
Estou em uma posição em que me pediram para revisar algum código que corrige um problema que não acredito existir.
O corretor, que é mais antigo que eu, insiste que sua correção é necessária, mas parece não ser mais do que sofisma de 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, espera-se que você revise as alterações.
Acredito que os revisores são tão responsáveis pelas alterações no código quanto o codificador original, e não estou disposto a aceitar a responsabilidade por essa alteração. Como você rejeitaria essa revisão?
Peça um caso de teste que falhe sem a alteração que é bem-sucedida na alteração.
Se ele não pode produzir um, você usa isso como justificativa.
Se ele puder produzir um, você precisará 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 analisá-lo, e parece que você e o técnico fixaram as posições. Se é assim, então você vai ter um tempo difícil aparecendo objetivo, e um tempo ainda mais difícil sendo objetivo. Nada disso ajuda o processo, e pode ser que a melhor e mais objetiva coisa que você pode fazer seja desistir com o argumento de que você está muito próximo da questão.
Considere uma abordagem de equipe.
Se não for possível remover você mesmo, talvez você possa ter vários outros engenheiros revisando o código ao mesmo tempo. Ou eles concordarão com você que o código deve ser rejeitado ou não. Se eles concordarem com você, então não será mais apenas você contra o corretor, e você será capaz de fazer um argumento mais strong de que a equipe olhou para 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 aberta, e que não deve tentar influenciar as opiniões dos outros membros da equipe por outra coisa que não seja a discussão racional. Importante: se houver um resultado ruim depois, não jogue o time embaixo do ônibus dizendo "Bem eu sempre disse que era um código ruim, mas eu estava em desvantagem em relação aos outros membros da equipe". p>
As 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 carimbar correções de pessoas mais experientes; está lá para proteger e melhorar a qualidade do código. Não há nada de errado em rejeitar uma correção, desde que você faça isso pelo motivo certo, ou seja, que a correção não aprimore 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 magnitude de um problema demonstrável, você deve rejeitá-lo. Não é pessoal, apenas a sua opinião sincera. Se o fixador não concordar, tudo bem também, e nesse momento se torna um problema para a gerência descobrir. Apenas certifique-se de permanecer honesto, aberto e profissional.
A responsabilidade divide os dois lados.
Você disse que não quer ser responsável por essa mudança, aparentemente porque não acredita que haja um problema. No entanto, você precisa perceber que, se estiver errado e houver um problema, você pode acabar sendo responsável por rejeitar o código que teria evitado o problema.
Tome notas.
Manter um registro por escrito do processo de revisão ajudará você a manter seus fatos corretos. Anote seus pensamentos e preocupações durante a revisão, a descrição e os resultados de quaisquer testes que você possa realizar para avaliar o problema alegado e a correção, etc. Se o problema aumentar, você terá um registro do que fez para dar suporte ao seu problema. posição. Se a questão surgir novamente no futuro (provavelmente, se o fixador estiver ligado à sua opinião), você terá algo para despertar sua memória.
Anote suas razões para rejeição e defesas para possíveis contra-argumentos. Em seguida, discutir racionalmente com o fixador e, se necessário, escalar para o gerenciamento.
Se você tem documentação suficiente (incluindo listagens de código, resultados de testes e argumentos objetivo ), você estará bem coberto.
Você causará má vontade com o corretor, por isso, certifique-se de que o problema vale a pena (ou seja, a não-correção causa danos?)
Além disso, se o fixador estiver de alguma forma relacionado aos proprietários, esqueça essa resposta.
De qualquer forma, ao dizer ao programador sênior que ele está errado em sua suposição, ele apenas levará uma abordagem defensiva e o atacará 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 compreensão, então ele / ela provavelmente estará mais aberto para discutir o assunto.
Então, ao invés de dizer "O problema não existe, então eu não deveria ter que revisar isso", talvez pergunte algo como "A fim de revisar isso corretamente, eu preciso entender o problema. Você pode por favor explicar do seu ponto de vista? "
Como exemplo, o artigo descreveu um teste dado a crianças em que um adulto segurava um cubo cujos rostos eram todos de uma cor, exceto aquele voltado para o adulto. Quando as crianças foram perguntadas sobre a cor do rosto que o adulto estava vendo, aquelas com menos de 5 anos quase sempre diziam a cor que podiam ver, pois não podiam compreender que o adulto pudesse 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 uma otimização agressiva e, geralmente, quando você está usando C / C ++, está interessado em velocidade.
Escrever caso de teste que quebra pode ser difícil - pode envolver arquitetura ou compilador estranho que não existe mais. Também pode parecer como em qualquer arquitetura sensata "não deve causar nenhum problema".
No entanto, em um ponto ou outro, você mudará de plataforma (digamos - você quer se tornar móvel, então você pode portar para o ARM ou você quer acelerar as coisas e usar o GPU). Neste ponto, as coisas podem começar a quebrar e você precisará depurá-lo. Pode ser tão trivial quanto atualizar o compilador para a versão mais recente (e você pode querer / precisar dele).
O código problemático foi:
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] é acessado. Como geralmente o próximo elemento era a memória legal (em termos de paginação, não o modelo de memória C), mesmo os compiladores triviais não produziam nenhum executável com comportamento estranho. A única maneira de escrever o caso de teste foi encontrar a plataforma com exatamente o mesmo modelo de memória que o C (provavelmente VM).
No entanto, o gcc 4.8 pré-correia visto que d[++k] é executado em cada loop. Então k < 16 caso contrário o acesso seria ilegal e a legalidade do programa alimentado ao compilador é parte do contrato. Portanto, a condição de loop é sempre verdadeira, dadas as suposições, de modo que é loop infinito. Isso pode parecer estranho, mas era uma otimização perfeitamente legal - emitir system("dd if=/dev/zero of=/dev/sda"); system("format c:") também seria uma substituição correta para o loop. Há maneiras mais sutis que você pode escolher para exibir comportamentos. Por exemplo, durante a palestra sobre Memória Transacional , lembro-me de que o apresentador tentou várias vezes obter um valor errado quando 2 threads incrementaram o mesmo valor.
No entanto, C / C ++, como oposto a algumas linguagens, é padronizado para que tal disputa possa ser feita com referência à fonte objetiva:
Em geral, se sua equipe está escrevendo em C / C ++, é útil ter o padrão em mãos - até mesmo especialistas em equipe podem encontrar algo estranho lá de vez em quando.
Isso soa mais como um problema com sua política de equipe do que com essa revisão específica. Quando eu trabalhava em uma grande loja de software em uma equipe de nove pessoas, um revisor tinha o poder de veto sobre o código, e esse era um padrão que todos nós respeitávamos. Nós éramos talentosos e espertos o bastante - viz. "maduro", mas mais no sentido de "não infantil" do que "experiente" - que o líder de nossa equipe poderia razoavelmente esperar que sempre pudéssemos chegar a um acordo sem cair em argumentos em um período de tempo prudente.
Simplesmente com base no seu idioma em seu post, eu poderia avisá-lo que parece que você vai abordar essa situação de uma forma que pode levar a um argumento descentralizado. Talvez seja "masturbação intelectual", mas há provavelmente uma razão pela qual ele ou ela fez isso, e o fardo para você será criar uma maneira mais simples de resolver esse problema - e, se não existe esse caminho, documente no comentário por que o código masturbatório era, de fato, necessário.
Faça o remetente mostrar uma prova de que o código dele resolve o problema dele. Se ele puder testar, e lhe mostrar a prova, então você é aquele que está errado e deve simplesmente desistir de sua queixa e lidar com isso. Se ele não puder mostrar provas para apoiar sua afirmação, há um problema, e mostrar a prova de que sua correção corrige o problema, então eu o enviaria de volta para o quadro de anotações.
É claro que pode haver sensibilidade política interna em torno disso. Mas é assim que eu vou (deliciosamente).
Tags code-reviews