Práticas ágeis: revisão de código - falha na revisão ou levanta um problema?

are there any inherent problems or considerations with raising a ticket off of the back of a review, instead of failing it?

Não inerentemente. Por exemplo, a implementação da mudança atual pode ter desenterrado um problema que já estava lá, mas não era conhecido / aparente até agora. Não conseguir o ticket seria injusto, pois você falharia por algo não relacionado à tarefa realmente descrita.

in the review we discover a function

No entanto, suponho que a função aqui seja algo que foi adicionado pela alteração atual. Nesse caso, o ticket deve falhar, pois o código não passou no teste de cheiro.

Onde você desenharia a linha, se não onde você já desenhou? Você claramente não acha que esse código está suficientemente limpo para permanecer na base de código em sua forma atual; então por que você consideraria dar um passe para o ingresso?

Fail the code review, so that the ticket doesn't close in this sprint, and we take a little hit on morale, because we cannot pass off the ticket.

Parece-me que você está indiretamente argumentando que está tentando dar um passe para beneficiar a moral da equipe, em vez de beneficiar a qualidade da base de código.

Se for esse o caso, então você tem suas prioridades misturadas. O padrão de código limpo não deve ser alterado simplesmente porque torna a equipe mais feliz. A correção e a limpeza do código não dependem do humor da equipe.

The refactor is a small piece of work, and would get done in the next sprint (or even before it starts) as a tiny, half point story.

Se a implementação do ticket original causou o cheiro do código, ele deverá ser endereçado no ticket original. Você só deve criar um novo tíquete se o cheiro do código não puder ser atribuído diretamente ao tíquete original (por exemplo, um cenário de "palha que quebrou o fundo do camelo").

The resources I can find and have read detail code reviews as 100% or nothing, usually, but I find that is usually not realistic.

Pass / fail é inerentemente um estado binário , que é inerentemente tudo ou nada.

O que você está se referindo aqui, eu acho, é mais que você interpreta as revisões de código como exigindo código perfeito ou falhando, e esse não é o caso.

O código não deve ser imaculado, deve simplesmente obedecer ao padrão razoável de limpeza que sua equipe / empresa emprega. A adesão a esse padrão é uma escolha binária: adere (passa) ou não (falha).

Com base na sua descrição do problema, é claro que você não acredita que ele esteja de acordo com o padrão de código esperado e, portanto, não deve ser aprovado por motivos ocultos, como moral da equipe.

Otherwise the task fits the definition of done.

Se "ele fizer o trabalho" fosse o melhor benchmark para qualidade de código, então não teríamos que inventar o princípio de código limpo e boas práticas para começar - o compilador e o teste de unidade já seriam nossos testes automatizados. processo de revisão e você não precisaria de revisões de código ou argumentos de estilo.

At the end of a 2 week sprint and a task has a code review [...] Easy refactor job.

Por que isso aparece no final do sprint? Uma revisão de código deve acontecer assim que você achar que o código está pronto (ou até mesmo antes). Você deve verificar sua definição de feito com cada história que terminar.

Se você está acabando de contar histórias tão pouco tempo antes de sua revisão demo / sprint que você não consegue encaixar uma tarefa "minúscula", então você precisa melhorar a estimativa do seu trabalho. Sim, essa história não foi concluída. Não por causa de uma revisão de código, mas porque você não planejou incorporar alterações da revisão de código. Isso é como estimar "testar" para tomar o tempo zero, porque "se você programou corretamente, ele simplesmente funcionará, certo?". Não é assim que funciona. O teste encontrará erros e a revisão de código encontrará itens a serem alterados. Se não fosse, seria uma grande perda de tempo.

Então, para resumir: sim, o DoD é binário. Passar ou falhar. Uma revisão de código não é binária, deve ser mais como uma tarefa contínua. Você não pode falhar . É um processo e no final está feito. Mas se você não planejar adequadamente, você não chegará àquele estágio "concluído" no tempo e ficará preso no território "não concluído" no final do sprint. Isso não é bom para a moral, mas você precisa explicar isso no planejamento.

Simples: você revisa a alteração . Você não revisa o estado do programa de outra forma. Se eu corrigir um bug em uma função de 3.000 linhas, verifique se minhas alterações corrigem o bug e pronto. E se minha alteração corrigir o bug, você aceita a alteração.

Se você acha que a função é muito longa, coloque uma solicitação de alteração para tornar a função mais curta ou dividi-la, após minha alteração foi aceita e essa solicitação de alteração pode ser priorizada de acordo com a sua importância. Se a decisão for tomada de que a equipe tem coisas mais importantes a fazer, ela será tratada mais tarde.

Seria ridículo se você pudesse decidir prioridades de desenvolvimento durante uma revisão de código, e rejeitar minha mudança por essa razão seria uma tentativa de decidir as prioridades de desenvolvimento.

Em resumo, é absolutamente aceitável aceitar uma mudança de código e imediatamente aumentar um ticket com base nas coisas que você viu ao revisar a alteração. Em alguns casos, você fará isso mesmo se a alteração causou os problemas: Se é mais importante ter as alterações agora do que corrigir os problemas. Por exemplo, se outras pessoas foram bloqueadas, aguardando a alteração, você deseja desbloqueá-las enquanto o código pode ser melhorado.

Fail the code review, so that the ticket doesn't close in this sprint, and we take a little hit on morale, because we cannot pass off the ticket.

Este parece ser o problema.
Teoricamente, você sabe o que deve fazer, mas está próximo do prazo, portanto não quer fazer o que sabe que deve fazer.

A resposta é simples: faça o que você faria se obtivesse o mesmo código para revisão de código no primeiro dia do sprint. Se seria aceitável, então deveria agora. Se não fosse, não seria agora.

Uma grande parte do processo é decidir o que feito significa e seguir suas armas. Isso também significa não comprometer demais e fazer com que suas revisões de colegas sejam concluídas a tempo de permitir que os testes garantam que o trabalho também esteja funcionalmente completo.

Quando se trata de questões de revisão de código, há algumas maneiras de lidar com isso, e a escolha certa depende de alguns fatores.

• Você pode apenas limpar o código por conta própria e informar a pessoa sobre o que você fez. Fornece algumas oportunidades de orientação, mas isso deve ser algo bastante simples que pode ser feito em poucos minutos.
• Você pode chutar de volta com comentários sobre o que está errado. Se o tratamento de erros for mal feito, ou se o desenvolvedor continuar repetindo os mesmos erros, isso pode ser garantido.
• Você pode criar um ticket e incorrer em dívida técnica. O bilhete está lá para garantir que você pague mais tarde. Pode ser que você esteja em uma crise de tempo e, no processo de revisão das mudanças, você veja um problema maior que não esteja diretamente relacionado à mudança.

A conclusão é que quando você terminar o trabalho, precisará terminar com ele. Se houver problemas maiores do que o desenvolvedor trabalhou, levante a bandeira e siga em frente. Mas você não deve estar em uma posição onde há horas antes do final do sprint e você está apenas começando a revisão por pares. Isso cheira a excesso de comprometimento de seus recursos, ou procrastinar nas revisões por pares. (um cheiro de processo).

Não há problemas inerentes à depriorização de problemas de revisão de código, mas parece que os principais problemas que você precisa concordar, como equipe, são:

1. Qual é o objetivo da sua análise de código?
2. Como os resultados da revisão de código se relacionam com a Definição de Feito para um item de trabalho?
3. Se a revisão de código se aplica como um teste de bloqueio, quais problemas são considerados 'bloqueadores'?

Tudo se resume ao que a equipe concordou como definição de Concluído. Se passar a revisão de código com Zero Issues for a definição de feito para um item de trabalho, você não poderá fechar um item que não atendeu a esse requisito.

É o mesmo que se durante o teste de unidade um teste de unidade falhasse. Você consertaria o bug, não ignoraria o teste de unidade, se passar testes de unidade fosse um requisito para ser feito.

Se a equipe não concordar em codificar as revisões como sendo uma definição de Concluído, suas revisões de código não serão um teste de aceitação do item de trabalho. Eles são uma atividade de equipe que faz parte do processo do backlog para procurar trabalho adicional que possa ser necessário. Nesse caso, quaisquer problemas descobertos não estão relacionados aos requisitos do item de trabalho original e são novos itens de trabalho para a equipe priorizar.

Por exemplo, pode ser completamente aceitável que uma equipe não priorize a correção de erros em alguns nomes de variáveis, pois isso não afeta a funcionalidade de negócios que foi entregue, mesmo que a equipe realmente odeie ver o nome da variável "myObkect". / p>     

As respostas mais votadas aqui são muito boas; este endereça o ângulo de refatoração.

Na maioria dos casos, a maior parte do trabalho na refatoração é entender o código existente; alterá-lo depois disso é geralmente a parte menor do trabalho por uma das duas razões:

1. Se apenas tornar o código mais claro e / ou conciso, as alterações necessárias são óbvias. Muitas vezes você adquiriu sua compreensão do código experimentando mudanças que pareciam mais limpas e vendo se elas realmente funcionavam, ou se elas perdiam alguma sutileza no código mais complexo.

2. Você já tem em mente um projeto ou estrutura particular que precisa para facilitar a criação de um novo recurso. Nesse caso, o trabalho para desenvolver esse design foi parte da história que gerou a necessidade disso; é independente de você precisar fazer a refatoração para chegar a esse design.

Aprender e entender o código existente é uma boa quantidade de trabalho para um benefício não permanente (daqui a um mês é provável que alguém tenha esquecido muito sobre o código se não continuar a ler ou trabalhar com ele durante esse período), e, portanto, não faz sentido fazer isso, exceto em áreas de código que estão causando problemas ou que você está planejando alterar em um futuro próximo. Por sua vez, como esse é o principal trabalho de refatoração, você não deve refatorar o código, a menos que esteja causando problemas ou esteja planejando alterá-lo no futuro próximo.

Mas há uma exceção: se alguém tiver um bom entendimento do código que vazará ao longo do tempo, usar esse entendimento para tornar o código mais claro e mais rapidamente entendido posteriormente pode ser um bom investimento. Essa é a situação em que alguém acabou de desenvolver uma história.

The refactor is a small piece of work, and would get done in the next sprint (or even before it starts) as a tiny, half point story.

Nesse caso, você está pensando em criar uma história separada para a refatoração é um sinal de alerta em várias frentes:

1. Você não está pensando em refatorar como parte da codificação, mas como uma operação separada, o que, por sua vez, faz com que seja descartado sob pressão.

2. Você está desenvolvendo um código que será mais trabalhoso para entender da próxima vez que alguém precisar trabalhar com ele, tornando as histórias mais demoradas.

3. Você pode estar desperdiçando tempo e energia refatorando coisas das quais não obtém muito benefício. (Se uma mudança acontecer muito mais tarde, alguém ainda terá que re-entender o código, de qualquer maneira; isso é mais eficientemente combinado com o trabalho de refatoração. Se uma mudança não acontecer mais tarde, a refatoração não serviu propósito, exceto talvez uma estética.)

Portanto, a resposta aqui é falhar no item para deixar claro que algo em seu processo falhou (nesse caso, o desenvolvedor ou a equipe não está alocando tempo para revisão e implementando alterações que saem da análise) e tem o desenvolvedor continue imediatamente o trabalho no item.

Quando você vai estimar para a próxima iteração, re-estimar a história existente como qualquer quantidade de trabalho parece ser deixado para fazer a revisão passar e adicioná-la à sua próxima iteração, mas preservando a estimativa da iteração anterior. Quando a história for concluída no final da próxima iteração, defina a quantidade total histórica de trabalho como a soma da primeira e da segunda estimativas, para que você saiba quanto trabalho estimado realmente foi colocado nela. Isso ajudará a produzir estimativas mais precisas de histórias semelhantes no futuro, no estado atual do processo. (Por exemplo, não presuma que sua subestimativa aparente não acontecerá novamente; suponha que isso aconteça novamente até que você tenha completado com sucesso histórias semelhantes enquanto trabalha menos).

na revisão, descobrimos uma função que funciona, é legível, mas é bastante longa e tem alguns odores de código ...

Existe algum problema ou consideração inerente ao levantamento de um ticket da parte de trás de uma revisão, em vez de falhar?

Não há problema algum (na opinião da minha equipe). Eu suponho que o código atende aos critérios de aceitação estabelecidos no ticket (ou seja, funciona). Crie um item de lista de pendências para endereçar o comprimento, e qualquer código cheira e priorize-o como qualquer outro ticket. Se é realmente pequeno, então apenas priorize-o para o próximo sprint.

Um dos ditados que temos é "Escolha melhoria progressiva sobre a perfeição adiada".

Temos um processo muito fluido e construímos um bom número de recursos de 'prova de conceito' (1 ou 2 por sprint) que passam por dev e teste, mas nunca passam da revisão interna das partes interessadas (hmm, podemos faça isso em vez disso?), alfa ou beta ... alguns sobrevivem, outros não.

No projeto atual, perdi a conta de quantas vezes criamos um determinado recurso, o colocamos nas mãos das partes interessadas e, duas ou mais vezes, removemos totalmente o problema porque a direção do produto mudou, ou requisitos causaram uma reformulação completa de como o recurso deve ser implementado. Quaisquer tarefas restantes de "refinamento" para um recurso excluído, ou que não se encaixam nos novos requisitos, são excluídas, bem como parte da preparação do backlog.

Existem duas maneiras de analisar este problema na minha opinião:

1. A maneira acadêmica
2. A maneira do mundo real

No entanto, ninguém no mundo real segue ágil para o T como para um (de muitas razões), diferentes indústrias têm requisitos diferentes. Assim, consertar o código agora ou assumir a dívida técnica (você criaria um novo PBI com maior probabilidade) deve ser decidido por caso. Se for comprometer o sprint ou uma liberação ou introduzir uma quantidade razoável de risco, as partes interessadas do negócio devem estar envolvidas na decisão.

Estou surpreso com a falta de resposta nas respostas e comentários à noção de "falha" em uma revisão de código, porque esse não é um conceito com o qual eu, pessoalmente, esteja familiarizado. Nem me sentiria confortável com esse conceito ou com qualquer pessoa da minha equipe usando essa terminologia.

Sua pergunta chama explicitamente de "práticas ágeis", então vamos revisitar o manifesto ágil (ênfase minha):

We are uncovering better ways of developing software by doing it and helping others do it. Through this work we have come to value:

• Individuals and interactions over processes and tools
• Working software over comprehensive documentation
• Customer collaboration over contract negotiation
• Responding to change over following a plan

That is, while there is value in the items on the right, we value the items on the left more.

Fale com sua equipe. Discuta o código em questão. Avalie os custos e os benefícios e decida - como um grupo coeso de especialistas - se refatorará este código agora, mais tarde ou nunca.

Comece a colaborar. Pare de falhar nas revisões de código.

Nem . Se falhar na revisão do código, a tarefa não será concluída. Mas você não pode falhar em revisões de código na opinião pessoal. O código passa; passar para a próxima tarefa.

Deve ser uma ligação fácil, e o fato de não ser isso sugere que você não tem regras suficientemente claras por escrito para revisões de código.

1. "A função é bem longa". Anote: As funções devem ter menos de X linhas (não estou sugerindo que as regras sobre o comprimento da função sejam boas)

2. "Existem alguns cheiros de código". Anote: as funções públicas devem ter testes de unidade para funcionalidade e desempenho, tanto a CPU quanto o uso de memória devem estar sob os limites xey.