Como lidar com discordâncias em uma revisão de código em relação a um caso improvável?

Estou trabalhando em uma startup de robótica em uma equipe de cobertura de caminho e, depois de enviar uma solicitação pull, meu código é revisado.

Meu colega de equipe, que está na equipe há mais de um ano, fez alguns comentários ao meu código que sugerem que eu trabalhe muito mais do que acredito ser necessário. Não, eu não sou um desenvolvedor preguiçoso. Eu amo código elegante que tem bons comentários, nomes de variáveis, indentação e lida com os casos corretamente. No entanto, ele tem um tipo diferente de organização em mente com a qual não concordo.

Vou dar um exemplo:

Passei um dia escrevendo casos de teste para alterar um algoritmo de descoberta de transição que eu fiz. Ele sugeriu que eu lidasse com um caso obscuro que é extremamente improvável de acontecer - na verdade, não tenho certeza de que seja possível. O código que eu criei já funciona em todos os nossos casos de teste originais e em alguns novos que encontrei. O código que eu criei já passa de mais de 300 simulações que são executadas todas as noites. No entanto, para lidar com esse caso obscuro, levaria 13 horas que poderiam ser gastas melhor tentando melhorar o desempenho do robô. Para deixar claro, o algoritmo anterior que estávamos usando até agora também não lidou com esse caso obscuro e nem uma vez, nos 40 mil relatórios gerados, já ocorreu. Somos uma startup e precisamos desenvolver o produto.

Eu nunca tive uma revisão de código antes e não tenho certeza se estou sendo muito argumentativo; devo ficar quieto e fazer o que ele diz? Decidi manter a cabeça baixa e apenas fazer a mudança, embora discorde totalmente que foi um bom uso do tempo.

Eu respeito meu colega de trabalho e o reconheço como um programador inteligente. Eu apenas discordo dele em um ponto e não sei como lidar com discordâncias em uma revisão de código.

Eu sinto que a resposta que escolhi atende a este critério de explicar como um desenvolvedor júnior pode lidar com discordâncias em uma revisão de código.

um caso obscuro que é extremamente improvável de acontecer - na verdade, não tenho certeza de que seja possível

Não ter comportamentos não testados no código pode ser muito importante. Se um trecho de código for executado, por exemplo, 50 vezes por segundo, uma chance em um milhão ocorrerá aproximadamente a cada 5,5 horas de tempo de execução. (No seu caso, as probabilidades parecem mais baixas.)

Você pode conversar sobre as prioridades com seu gerente (ou com quem seja a pessoa mais experiente responsável pela unidade em que trabalha). Você entenderá melhor se, por exemplo, trabalhar no desempenho do código ou à prova de balas é a principal prioridade e quão improvável essa caixa de canto pode ser. Seu revisor também pode ter uma ideia distorcida de prioridades. Depois de conversar com o responsável, você terá mais facilidade em (des) concordar com as sugestões do revisor e terá algo a que se referir.

É sempre uma boa ideia ter mais de um revisor. Se seu código for revisado apenas por um colega, peça a alguém que conheça esse código, ou a base de código em geral, para dar uma olhada. Uma segunda opinião, novamente, ajudará você a (des) concordar mais facilmente com as sugestões do revisor.

Ter vários comentários recorrentes durante várias revisões de código geralmente indica que algo maior não está sendo claramente comunicado, e os mesmos problemas surgem repetidamente. Tente descobrir essa coisa maior e discuta-a diretamente com o revisor. Pergunte o suficiente por que perguntas. Isso me ajudou muito quando comecei a prática de revisões de código.

Posso dar um exemplo de um caso de esquina que nunca poderia ocorrer e causou um desastre.

Quando o Ariane 4 estava sendo desenvolvido, os valores dos acelerômetros laterais foram dimensionados para caberem em um número inteiro assinado de 16 bits e porque a saída máxima possível dos acelerômetros, quando dimensionada, nunca poderia exceder exceder 32767 e o mínimo nunca poderia cair abaixo - 32768 "não havia necessidade de sobrecarga na verificação de alcance". Em geral, todas as entradas devem ser verificadas no intervalo antes de qualquer conversão, mas, nesse caso, isso tentaria detectar um caso de canto impossível.

Vários anos depois, o Ariane 5 estava sendo desenvolvido e o código para escalar os acelerômetros laterais foi reutilizado com testes mínimos, uma vez que foi “comprovado em uso”. Infelizmente, o foguete maior poderia esperar maiores acelerações laterais, para que os acelerômetros fossem atualizados e pudessem produzir valores maiores de flutuação de 64 bits .

Esses valores maiores "empacotados" no código de conversão, não lembram de verificação de intervalo, e os resultados do primeiro lançamento em 1996 não foram bons. Custou milhões à empresa e causou um grande hiato no programa.

O ponto que estou tentando ressaltar é que você não deve ignorar os casos de teste como nunca acontecendo, extremamente improvável, etc .: os padrões para os quais eles estavam codificando exigiam a verificação do intervalo de todos os valores de entrada externos. Se isso tivesse sido testado e tratado, o desastre poderia ter sido evitado.

Observe que no Ariane 4 isso não foi um bug (como tudo funcionou bem para todos os valores possíveis) - foi uma falha em seguir os padrões. Quando o código foi reutilizado em um cenário diferente, ele falhou catastroficamente, enquanto se o intervalo de valores tivesse sido cortado, provavelmente teria falhado normalmente, e a existência de um caso de teste para isso poderia ter acionado uma revisão dos valores. Também é importante notar que, enquanto os codificadores e testadores foram submetidos a algumas críticas dos investigadores após a explosão, a gerência, o controle de qualidade e a liderança ficaram com a maior parte da culpa.

Esclarecimento

Embora nem todo software seja crítico para a segurança, nem tão espetacular quando falha, minha intenção era destacar que testes "impossíveis" ainda podem ter valor. Este é o caso mais dramático que eu conheço, mas a robótica também pode produzir alguns resultados desastrosos.

Pessoalmente, eu diria que uma vez que alguém tenha destacado uma caixa de canto para a equipe de teste, um teste deve ser colocado em prática para verificá-la. O líder da equipe de implementação ou o gerente de projeto pode decidir não tentar solucionar as falhas encontradas, mas deve estar ciente de que existem deficiências. Como alternativa, se o teste for muito complexo ou caro para implementar, um problema poderá ser levantado em qualquer rastreador em uso e / ou no registro de riscos, para deixar claro que esse é um caso não testado - que pode precisar ser resolvido. antes de uma mudança de uso ou impedir um uso inadequado.

Como não foi tratado antes, está fora do escopo do seu esforço. Você ou seu colega podem perguntar ao seu gerente se vale a pena o esforço para cobrir este caso.

Com algoritmos complexos, é muito difícil provar que você pensou em todos os casos de teste que surgirão no mundo real. Quando você intencionalmente deixa um caso de teste quebrado porque ele não aparece no mundo real, você está potencialmente deixando outros casos de teste em que ainda nem pensou.

O outro efeito que costuma ocorrer é quando você lida com casos de teste adicionais, seu algoritmo necessariamente se torna mais geral e, por isso, você encontra maneiras de simplificá-lo e torná-lo mais robusto para todos os seus casos de teste. Isso economiza seu tempo em manutenção e solução de problemas no futuro.

Além disso, existem muitos casos de bugs "isso nunca deve acontecer" acontecendo na natureza. Isso ocorre porque seu algoritmo pode não mudar, mas suas entradas podem mudar anos depois, quando ninguém se lembra desse caso de uso não tratado. É por isso que desenvolvedores experientes lidam com esse tipo de coisa quando estão frescos em suas mentes. Ele volta para mordê-lo mais tarde, se não o fizer.

Esta não é uma questão técnica, mas uma decisão de estratégia de negócios. Você percebe que a correção sugerida é para um caso muito obscuro que quase nunca acontecerá. Aqui faz uma grande diferença se você estiver programando um brinquedo ou se estiver programando, digamos, equipamento médico ou um drone armado. As consequências de um mal funcionamento raro serão muito diferentes.

Ao fazer revisões de código, você deve aplicar um entendimento básico das prioridades de negócios ao decidir quanto investir no tratamento de casos raros. Se você não concordar com o seu colega na sua interpretação das prioridades da empresa, convém conversar com alguém do lado comercial.

As revisões de código não se limitam apenas à correção do código. Na realidade, isso está bem abaixo da lista de benefícios, por trás do compartilhamento de conhecimento e por ser um processo lento, mas constante, para criar um consenso de estilo / projeto de equipe.

Como você está experimentando, o que conta como "correto" geralmente é discutível, e todos têm seu próprio ângulo sobre o que é isso. Na minha experiência, tempo e atenção limitados também podem tornar as revisões de código altamente inconsistentes - o mesmo problema pode ser detectado ou não, dependendo de diferentes desenvolvedores e em momentos diferentes pelo mesmo desenvolvedor. Revisores na mentalidade de "o que melhoraria esse código?" geralmente sugerem mudanças que eles não adicionariam aos seus próprios esforços naturalmente.

Como codificador experiente (mais de 15 anos como desenvolvedor), sou frequentemente revisado por codificadores com menos anos de experiência do que eu. Às vezes, eles me pedem mudanças das quais eu discordo levemente ou que considero sem importância. No entanto, eu ainda faço essas alterações. Eu travo batalhas por mudanças apenas quando sinto que o resultado final causaria uma fraqueza no produto, onde o custo do tempo é muito alto ou onde um ponto de vista "correto" pode ser objetivo (por exemplo, a alteração solicitada " t funciona no idioma que estamos usando ou uma referência mostra que uma melhoria de desempenho alegada não é uma).

Então, sugiro escolher suas batalhas com cuidado. Dois dias codificando um caso de teste que você acha que não é necessário provavelmente não valem o tempo / esforço para lutar. Se você estiver usando uma ferramenta de revisão, como solicitações de recebimento do GitHub, talvez faça um comentário sobre o custo / benefício que você percebe, para observar sua objeção, mas concorda em continuar o trabalho. Isso conta como um leve empurrão para trás, para que o revisor saiba que eles estão atingindo um limite e, mais importante, inclua sua lógica para que casos como esse possam ser escalados se entrarem em conflito. Você deseja evitar uma escalada de discordâncias por escrito - não deseja ter um argumento no estilo de fórum da Internet sobre diferenças de trabalho -, portanto, pode ser útil discutir a questão primeiro e registrar um resumo justo do resultado da discussão,discussão amigável decidirá para vocês de qualquer maneira).

Após o evento, este é um bom tópico para reuniões de planejamento de equipe de desenvolvimento ou revisão de sprint, etc. Apresente-o da maneira mais neutra possível, por exemplo: "Na revisão de código, o Desenvolvedor A identificou esse caso de teste adicional, que levou mais dois dias para ser escrito. a equipe acha que a cobertura extra se justifica a esse custo? " - essa abordagem funciona muito melhor se você realmente faz o trabalho, como mostra de maneira positiva; você fez o trabalho e deseja pesquisar a equipe sobre aversão ao risco e desenvolvimento de recursos.

Eu aconselho você a pelo menos reivindicar o caso obscuro. Dessa forma, não apenas os futuros desenvolvedores verão que você decidiu ativamente contra o caso, mas com um bom tratamento de falhas, que já deveria estar no local, isso também traria surpresas.

E então, faça um caso de teste que afirme essa falha. Dessa forma, o comportamento é melhor documentado e será exibido em testes de unidade.

Esta resposta obviamente pressupõe que o seu julgamento do fato de ser "extremamente improvável se possível" está correto e não podemos julgar isso. Mas se for, e seu colega concorda, uma afirmação explícita contra o evento deve ser uma solução satisfatória para vocês dois.

Como você parece ser novo por lá, há apenas uma coisa que você pode fazer - verifique com o líder da equipe (ou líder do projeto). 13 horas é uma decisão de negócios; para algumas empresas / equipes, muito; para alguns, nada. Não é sua decisão, ainda não.

Se o lead indicar "cubra esse caso", tudo bem; se ele disser "não, que se dane", tudo bem - sua decisão, sua responsabilidade.

Quanto às revisões de código em geral, relaxe. Ter uma tarefa retornada para você uma ou duas vezes é perfeitamente normal.

Uma coisa que acho que não vi abordada em espécie, embora tenha sido levantada na resposta @SteveBarnes:

Quais são as repercussões de uma falha?

Em alguns campos, uma falha é um erro em uma página da web. Um PC é exibido em azul e reinicializado.

Em outros campos, é vida ou morte - o carro autônomo trava. O marcapasso médico para de funcionar. Ou na resposta de Steve: as coisas explodem, resultando na perda de milhões de dólares.

Há um mundo de diferença nesses extremos.

Se vale ou não 13 horas para cobrir uma "falha", não cabe a você. Deve ser responsabilidade da gerência e dos proprietários. Eles devem ter uma ideia do quadro geral.

Você deve dar um bom palpite sobre o que valerá a pena. Seu robô simplesmente diminuirá a velocidade ou parará? Desempenho degradado? Ou uma falha do robô causará danos monetários? Perda de vida?

A resposta para essa pergunta deve direcionar a resposta para "vale 13 horas do tempo da empresa". Aviso: Eu disse que o tempo das empresas. Eles pagam as contas e finalmente decidem o que vale a pena. Sua gerência deve ter a palavra final de qualquer maneira.

Talvez fale com a pessoa responsável por priorizar o trabalho? Na inicialização, pode haver CTO ou proprietário do produto. Ele poderia ajudar a descobrir se esse trabalho extra é necessário e por quê. Além disso, você pode trazer suas preocupações durante situações de emergência diárias (se as tiver).

Se não houver uma responsabilidade clara (por exemplo, o proprietário do produto) pelo planejamento do trabalho, tente conversar com as pessoas ao seu redor. Mais tarde, pode se tornar um problema que todos estão empurrando o produto para uma direção oposta.

A melhor maneira de lidar com discordâncias é a mesma, independentemente de você ser um desenvolvedor júnior ou um desenvolvedor sênior ou mesmo um CEO.

Aja como Columbo .

Se você nunca assistiu a nenhum Columbo, foi um show fantástico. Columbo era esse personagem despretensioso - a maioria das pessoas pensava que ele era um pouco louco e não valia a pena se preocupar. Mas, parecendo humilde, e apenas pedindo às pessoas que expliquem, ele conseguiu o homem.

Eu acho que também está relacionado ao método socrático .

Em geral, você deseja fazer perguntas a si mesmo e aos outros para se certificar de que está fazendo as escolhas certas. Não de uma posição de "Estou certo, você está errado", mas de uma posição de descoberta honesta. Ou pelo menos o melhor que puder.

No seu caso, você tem duas idéias aqui, mas elas têm basicamente o mesmo objetivo: melhorar o código.

Você tem a impressão de que poupar na cobertura do código para um caso potencialmente improvável (impossível?) Em favor do desenvolvimento de outros recursos é a melhor maneira de fazer isso.

Seu colega de trabalho tem a impressão de que ter mais cuidado com as caixas de canto é mais valioso.

O que eles veem que você não vê? O que você vê que eles não vêem? Como desenvolvedor júnior, você está realmente em uma ótima posição, porque naturalmente deve estar fazendo perguntas. Em outra resposta, alguém menciona como é surpreendentemente provável um caso de canto. Para começar, "Ajude-me a entender - eu estava com a impressão de que X, Y e Z - o que estou perdendo? Por que o widget framboozle? Eu estava com a impressão de que ele seria instável em circunstâncias cromulentas. o swizzle stick realmente embiggen os pincéis ANZA? "

Ao questionar suas suposições e suas descobertas, você irá procurar, descobrir preconceitos e, eventualmente, descobrir qual é o curso de ação correto.

Comece com a suposição de que todos na sua equipe são perfeitamente racionais e têm em mente os melhores interesses da equipe e do produto, assim como você. Se eles estão fazendo algo que não faz sentido, então você precisa descobrir o que você não sabe que eles fazem ou o que você sabe que eles não sabem.

13 horas não é grande coisa, eu faria. Lembre-se de que você está sendo pago por isso. Apenas giz como "segurança no emprego". Também é melhor manter um bom karma entre a equipe. Agora, se fosse algo que levaria uma semana ou mais, você poderia envolver seu gerente e perguntar se esse é o melhor uso do seu tempo, especialmente se você não concordar.

No entanto, parece que você precisa de alavancagem no seu grupo. Aqui está como você obtém vantagem: peça perdão, não peça permissão. Adicione coisas ao programa como achar melhor (no escopo do curso, ou seja, verifique se ele resolve completamente o problema que o chefe deseja ..) e informe o gerente ou seus colegas após o fato. Não pergunte a eles: "Tudo bem se eu adicionar o recurso X". Em vez disso, basta adicionar os recursos que você deseja pessoalmente no programa. Se eles ficarem chateados com um novo recurso ou não concordarem, não deixe de removê-lo. Se eles gostarem, guarde.

Além disso, sempre que eles pedirem para você fazer algo, vá além e adicione muitas coisas que eles esqueceram de mencionar ou que funcionariam melhor do que disseram. Mas não pergunte se não há problema em percorrer uma milha extra. Basta fazê-lo e, ocasionalmente, contar a eles sobre isso depois que terminar. O que você está fazendo é treiná-los ..

O que acontecerá será que seu gerente o classificará como um "empreendedor" e começará a confiar em você, e seus colegas começarão a vê-lo como o líder em que você está começando a ser o dono do programa. E então, quando coisas como o que você mencionar acontecerem no futuro, você terá mais palavras a dizer, porque você é essencialmente a estrela do time e os colegas de time recuarão se você não concordar com eles.

A revisão de código serve a vários propósitos. Um de que você obviamente está ciente é: " Esse código é adequado ao objetivo? " Em outras palavras, ele é funcionalmente correto; é adequadamente testado; as partes não óbvias são adequadamente comentadas; está de acordo com as convenções do projeto?

Outra parte da revisão de código é o compartilhamento de conhecimento sobre o sistema. É uma oportunidade para o autor e o revisor aprenderem sobre o código alterado e como ele interage com o restante do sistema.

Um terceiro aspecto é que ele pode fornecer uma revisão dos problemas que existiam antes que qualquer alteração fosse feita. Freqüentemente, ao revisar as alterações de outra pessoa, identificarei algo que perdi em uma iteração anterior (muitas vezes algo próprio). Uma declaração como "Aqui está uma oportunidade de tornar isso mais robusto do que era", não é uma crítica e não a tome como uma!

Minha equipe atual considera a revisão de código não apenas como um gateway ou obstáculo que o código deve passar ileso antes da confirmação, mas principalmente como uma oportunidade para uma discussão um pouco estruturada de uma área específica de funcionalidade. É uma das ocasiões mais produtivas para o compartilhamento de informações. (E esse é um bom motivo para compartilhar a revisão com a equipe, em vez de sempre o mesmo revisor).

Se você perceber as revisões de código como uma atividade de confronto, é uma profecia que se realiza. Se você os considerar como a parte mais colaborativa do seu trabalho, eles estimularão melhorias contínuas no seu produto e na forma como você trabalha em conjunto. Ajuda se uma revisão puder ser clara sobre as prioridades relativas de suas sugestões - há uma milha de diferença entre "Eu gostaria de um comentário útil aqui" e "Isso interrompe se xfor negativo", por exemplo.

Tendo feito muitas declarações gerais acima, como isso se aplica à sua situação específica? Espero que agora seja óbvio que meu conselho é responder à revisão com perguntas abertas e negociar qual abordagem tem mais valor. Para o seu exemplo de caso em que um teste extra é sugerido, algo como "Sim, podemos testar isso; eu estimo que levará <tempo> para ser implementado. Você acha que o benefício vale a pena? E há algo mais que nós pode fazer para garantir que o teste não é necessário? "

Uma coisa que me impressiona quando leio sua pergunta: se são necessários dois dias de esforço para escrever um novo caso de teste, então o novo teste é um cenário muito diferente dos testes existentes (nesse caso, provavelmente há muitos valor) ou você identificou a necessidade de reutilização aprimorada de código no conjunto de testes.

Finalmente, como um comentário geral sobre o valor das revisões de código (e como um resumo conciso do que eu disse acima), eu gosto desta declaração, em Maintainers Don't Scale, de Daniel Vetter :

Pelo menos para mim, a revisão não é apenas garantir uma boa qualidade do código, mas também difundir conhecimento e melhorar o entendimento. No início, talvez haja uma pessoa, o autor (e isso não é um dado), entendendo o código. Após uma boa revisão, deve haver pelo menos duas pessoas que entendam completamente, incluindo casos de canto.

Código SEMPRE pode ser melhor.

Se você está em uma revisão de código e não vê nada que possa ser melhor ou um teste de unidade que pode detectar um bug, não é o código perfeito, mas o revisor que não está fazendo seu trabalho. Se você escolheu mencionar a melhoria é uma escolha pessoal. Mas quase sempre que sua equipe faz uma revisão de código, deve haver coisas que alguém percebe que poderiam ser melhores ou que todo mundo provavelmente desperdiçou seu tempo.

Dito isto, se você age nos comentários ou não, cabe a sua equipe. Se suas alterações resolverem o problema ou agregarem valor suficiente, sem alterações, para que sua equipe as aceite, junte-as e registre seus comentários no backlog para que alguém possa resolver posteriormente. Se a equipe descobrir que suas alterações adicionam mais risco ou complexidade do que valor, é necessário resolver os problemas de acordo.

Lembre-se de que qualquer código possui pelo menos mais um caso de borda que pode ser testado e pode usar pelo menos mais uma refatoração. É por isso que as revisões de código são feitas melhor como um grupo, com todos olhando para o mesmo código ao mesmo tempo. Para que, no final, todos cheguem a um consenso sobre se o código em revisão é aceitável (como é) e agregue valor suficiente para mesclar-se à base da comunidade ou se certas coisas devem ser feitas antes que haja valor suficiente para mesclar .

Como você está fazendo essa pergunta, presumo que você não esteja realmente fazendo "revisões de código", mas criando uma solicitação de recebimento ou outro mecanismo de envio para que outros comentem de maneira não determinística. Então agora você está em um problema de gerenciamento e uma definição de pronto. Eu acho que seu gerenciamento é indeciso e realmente não entende o processo e a finalidade das revisões de código e provavelmente não possui uma "definição de feito" (DOD). Porque se o fizessem, o seu DOD responderia explicitamente a essa pergunta e você não precisaria vir aqui e fazer sua pergunta.

Como você conserta isso? Bem, peça ao seu gerente que lhe dê um DOD e diga se você deve sempre implementar todos os comentários. Se a pessoa em questão é seu gerente, a resposta é evidente.

Esta questão não se refere às virtudes da programação defensiva, aos perigos dos casos extremos ou aos riscos catastróficos de bugs em produtos físicos. De fato, nem sequer se trata de engenharia de software .

O que realmente interessa é como um praticante júnior lida com uma instrução de um praticante sênior quando o júnior não pode concordar ou apreciar isso.

Há duas coisas que você precisa apreciar sobre ser um desenvolvedor júnior. Em primeiro lugar, significa que, embora seja possível que você esteja certo, e ele errado, isso não é provável - no balanço das probabilidades. Se o seu colega de trabalho está fazendo uma sugestão da qual você não vê o valor, você precisa considerar seriamente a possibilidade de não ter experiência suficiente para entendê-la. Eu não entendo esse sentido neste post.

A segunda coisa a apreciar é que o seu parceiro sênior é chamado porque ele tem mais responsabilidade. Se um júnior quebra algo importante, não terá problemas se seguir as instruções. Se um veterano os permitisse quebrá-lo, no entanto - não levantando problemas na revisão de código, por exemplo -, eles com toda a razão se incomodariam.

Por fim, é um requisito implícito do seu trabalho que você observe instruções daqueles em que a empresa confia para liderar seus projetos. Você geralmente não consegue adiar os idosos quando há boas razões para valorizar a experiência deles? Você pretende seguir nenhuma instrução que não possa entender? Você acha que o mundo deve parar até que você esteja convencido? Esses valores são incompatíveis com o trabalho em equipe.

Um ponto final. Pense nos projetos que você escreveu seis meses atrás. Pense nos projetos que você escreveu na universidade. Vê como eles agora parecem ruins - todos os erros e o design invertido, os pontos cegos e as abstrações equivocadas? E se eu lhe dissesse que daqui a seis meses você contará as mesmas falhas no trabalho que está fazendo hoje? Isso ajuda a demonstrar quantos pontos cegos existem na sua abordagem atual? Porque essa é a diferença que a experiência faz.

constantemente faz comentários ao meu código que sugerem que eu trabalhe muito mais do que o necessário.

Você pode fazer recomendações, mas, em última análise, não é sua função decidir o que é necessário. É seu trabalho implementar o que a gerência ou (neste caso, seu revisor) decide que é necessário. Se você não concorda com o que é necessário demais ou com muita força, provavelmente ficará sem emprego. Consequentemente, faz parte do seu profissionalismo aceitar isso e estar em paz com ele.

Ele sugeriu que eu lidasse com um caso obscuro que é extremamente improvável que aconteça

Existem outros grandes respostas aqui que mostram como mesmo os não-bugs (ie. Algo que comprovadamente não pode falhar sempre ) ainda deve ser re-trabalhado, às vezes. (por exemplo, no caso de criar uma segurança futura do produto, seguir os padrões etc.) Parte do papel de um grande desenvolvedor é ter o máximo de confiança possível para que seu código seja robusto em todas as situações concebíveis a qualquer momento e no futuro à prova de umidade, não apenas trabalhando em situações testadas nas condições atuais na maioria das vezes

Sugestões para revisores de código para aumentar a utilidade comercial de sua revisão de código (você, como OP, deve propor uma alteração):

• Marque seus comentários por tipo. "Crítico" / "Imprescindível" / "Opcional" / "Melhorias sugeridas" / "bom ter" / "Estou pensando".

  Se isso parecer muito CDO / anal / complicado, use pelo menos 2 níveis: "É necessário corrigir a aprovação na revisão e permitir a fusão de suas alterações" / "Todas as outras".

Sugestões para lidar com sugestões de revisão de código que parecem menos críticas:

• Crie um ticket aberto no seu sistema de emissão de ingressos preferido (sua equipe usa um, espero?), Acompanhando a sugestão

• Coloque o número do ticket como um comentário de resposta ao item de revisão de código se o seu processo permitir respostas a comentários como o Fisheye ou as revisões por email.

• Entre em contato com o revisor e pergunte explicitamente se esse item é do tipo "deve corrigir ou não será mesclado / liberado".

  • Se a resposta for "Sim", mas você não concorda, deixe a pessoa responsável pelo gerenciamento do projeto (PM, seu gerente, etc ...) tomar uma decisão - apresente a discordância total e honestamente. Não é sobre qual de vocês está "certo", mas sobre o que é melhor para o projeto, portanto, o trabalho do gerente / gerente de projetos.

Agora, trate esse ticket como qualquer outra solicitação de desenvolvimento.

1. Se for decidido ser urgente após a escalação, trate-o como qualquer solicitação urgente de desenvolvedor. Despriorize outro trabalho e trabalhe nisso.

2. Caso contrário, trabalhe nele de acordo com a prioridade que lhe foi atribuída e seu ROI (que pode diferir com base na sua linha de negócios, conforme explicado em outras respostas).

Você não deve encaminhar isso para a gerência.

Na maioria das empresas, o gerente sempre escolhe não escrever esse teste extra, não perder tempo melhorando marginalmente a qualidade do código, para não perder tempo refatorando.

Em muitos casos, a qualidade do código depende das regras não escritas na equipe de desenvolvimento e do esforço extra que os programadores fazem.

Você é um desenvolvedor júnior e esta é sua primeira revisão de código . Você deve aceitar o conselho e fazer o trabalho. Você só pode melhorar o fluxo de trabalho e as regras da sua equipe se os conhecer e respeitar por um tempo, para entender por que eles estão lá. Caso contrário, você será o cara novo que não respeita as regras e se torna o lobo solitário da equipe.

Você é novo na equipe, segue os conselhos que recebe por um tempo, descobre por que eles estão lá, não traga o primeiro conselho que você colocar em questão na reunião do scrum. As prioridades reais dos negócios ficarão evidentes para você depois de um tempo sem perguntar (e pode não ser o que o gerente vai dizer cara a cara).

É muito importante que você crie um código que atenda à sua solicitação de lead / gerenciamento. Qualquer outro detalhe seria apenas "um prazer ter". Se você é um especialista (leia o seguinte: "se você não é um desenvolvedor júnior") em seu campo, você é "elegível" para resolver os problemas menores encontrados ao longo do caminho sem consultar seus líderes todas as vezes.

Se você acha que algo está errado e você é relativamente especialista em seu campo, as chances são altas de que você está certo .

Aqui estão algumas instruções que podem ser úteis para você:

• Me pediram para fazer o X, o revisor de código sugere que você também faça Y, devo fazê-lo ou devo passar para coisas mais importantes?
• Você está sugerindo Y para mim, então você pode descobrir pelo menos um caso de teste que capte esse comportamento para que eu possa testá-lo? Eu acredito que esse código não será chamado.
• Talvez não devamos desenvolver um substituto seguro para casos descobertos primeiro? Então, nós os pegamos cedo e nos fixamos em qualquer lugar para que possamos nos concentrar em coisas mais importantes.
• OK, enquanto estou implementando o Y, você poderia escrever alguns casos de teste para fazermos isso de uma vez por todas ?
• Fui convidado a fazer o X e acho que poderia fazer o Y, a menos que haja outras prioridades. Da próxima vez, por que você não arquiva uma solicitação de recurso em vez de colocá-la como um comentário de revisão no meu código ? Pelo menos, podemos ouvir outras opiniões de outros membros da equipe sobre esse recurso antes de implementá-lo (geralmente qualquer material importante deve ser um recurso e deve ser tratado por mais de uma pessoa; geralmente a revisão de código deve ser sobre a revisão de abordagens de código e soluções; o restante é correção de bugs e recursos).

Você acha seriamente que a atitude do seu revisor está prejudicando o projeto ou você acha que ele está certo na maioria das vezes (exceto, talvez, às vezes, ele apenas comete pequenos erros na avaliação e exagera isso)?

Para mim, parece mais que ele está escrevendo coisas que não pertencem a uma revisão de código, o que é uma prática ruim porque torna mais difícil para todos rastrear coisas. No entanto, não sei que outros comentários de revisão ele fez, por isso não posso dizer nada em outros casos.

Em geral, tente evitar o seguinte:

• Você não está fazendo o que foi solicitado
• Você faz seu revisor parecer burro

Se ele está revisando seu código, é porque a gerência confia nele mais do que em você.

Quando houver discordância durante a revisão de código sobre o escopo:

1. Documente o escopo realmente coberto pelo código. Ninguém gosta de surpresas desagradáveis.
2. Perceba que o escopo é uma decisão de negócios. O escopo já deve ser conhecido quando você começar a trabalhar em um recurso, mas se não for, sempre poderá pedir esclarecimentos mais tarde.

Se o revisor de código é quem toma as decisões de negócios, ele pode alterar o escopo a qualquer momento, mesmo durante a revisão de código, mas ele não o faz em sua função de revisor de código.

Se você não pode provar que o caso de borda é impossível, você deve assumir que é possível. Se for possível, é inevitável que isso acabe ocorrendo, e mais cedo ou mais tarde. Se o caso extremo não ocorreu nos testes, isso pode indicar que a cobertura do teste está incompleta.

1. Aceite o feedback.
2. Antes de fazer alterações no seu código, tente o seu melhor para criar um teste para o caso de ponta e veja se você pode obter uma falha no teste (prova de que o problema existe). Se for impossível criar um caso de teste e ocorrer uma falha no teste, você poderá concluir que o caso de borda é realmente impossível (embora eu hesite em tirar essa conclusão).
3. Se você conseguir uma falha no teste, aplique a alteração de código apropriada para passar no teste.

Para o bem do produto, você provavelmente deseja produzir o estojo de ponta e induzir uma falha, para poder aplicar a correção e ter certeza de que um possível problema foi evitado.

O ponto principal das revisões de código é colocar outros olhos no código. Nenhum de nós está imune a erros ou negligências. É muito comum olhar para um pedaço de código várias vezes e não perceber um erro óbvio, onde um par de olhos novo pode pegá-lo imediatamente.

Como você disse, você implementou um novo algoritmo. Seria imprudente tirar conclusões sobre o comportamento do seu novo algoritmo com base no comportamento ou nas observações sobre seu antecessor.

Existem revisores de código que sabem o que estão fazendo e, se dizem que algo precisa ser alterado, ele precisa ser alterado e, quando dizem que algo precisa ser testado, ele precisa ser testado.

Existem revisores de código que precisam justificar sua própria existência, criando um trabalho inútil para os outros.

Qual é qual é a sua decisão e como lidar com o segundo tipo é mais uma questão para o local de trabalho.

Se você usa o scrum, então a questão é se o seu trabalho faz o que deveria (aparentemente) e você pode lidar com o caso extremamente raro e talvez impossível na lista de pendências, onde será priorizado e se entrar em um sprint, o revisor poderá ficar à vontade para buscá-lo e realizar as 13 horas de trabalho. Se você realiza o trabalho X e, como realiza o trabalho X, percebe que o trabalho Y também precisa ser executado, o trabalho Y não se torna parte do trabalho X, é seu próprio trabalho independente.