Revise antes ou depois da confirmação do código, o que é melhor?

Tradicionalmente, realizávamos a revisão de código antes do commit, tive uma discussão hoje com meu colega, que preferia a revisão do código após o commit.

Primeiro, aqui estão alguns antecedentes,

1. Temos alguns desenvolvedores experientes e também temos novas contratações com quase zero de experiência em programação.
2. Gostaríamos de executar iterações rápidas e curtas para lançar nosso produto.
3. Todos os membros da equipe estão localizados no mesmo site.

As vantagens da revisão de código antes de confirmar eu aprendi:

1. Mentor de novas contratações
2. Tente evitar erros, falhas, designs ruins no início do ciclo de desenvolvimento
3. Aprenda com os outros
4. Backup de conhecimento se alguém sair

Mas também tive algumas experiências ruins:

1. Baixa eficiência, algumas alterações podem ser revisadas ao longo de dias
2. Difícil de equilibrar velocidade e qualidade, especialmente para iniciantes
3. Um membro da equipe sentiu desconfiança

Quanto à revisão pós-confirmação, sei pouco sobre isso, mas o que mais me preocupa é o risco de perder o controle devido à falta de revisão. Alguma opinião?

ATUALIZAR:

1. Estamos usando o Perforce for VCS
2. Codificamos e confirmamos nos mesmos ramos (ramos de correção de troncos ou bugs)
3. Para melhorar a eficiência, tentamos dividir o código em pequenas alterações. Também tentamos algumas revisões de diálogos ao vivo, mas nem todos seguiram a regra. Este é outro problema, no entanto.

Como Simon Whitehead menciona em seu comentário , isso depende da sua estratégia de ramificação.

Se os desenvolvedores tiverem sua própria ramificação privada para desenvolvimento (o que eu recomendaria na maioria das situações), eu executaria a revisão do código antes de mesclar com o tronco ou o repositório principal. Isso permitirá que os desenvolvedores tenham a liberdade de fazer o check-in com a freqüência que desejar durante seu ciclo de desenvolvimento / teste, mas a qualquer momento que o código entrar no ramo que contém o código entregue, ele será revisado.

Geralmente, suas experiências ruins com as revisões de código parecem mais um problema com o processo de revisão que possui soluções. Ao revisar o código em blocos menores e individuais, você pode garantir que não demore muito. Um bom número é que 150 linhas de código podem ser revisadas em uma hora, mas a taxa será mais lenta para pessoas não familiarizadas com a linguagem de programação, o sistema em desenvolvimento ou a criticidade do sistema (um crítico de segurança requer mais tempo) - essas informações podem ser úteis para melhorar a eficiência e decidir quem participa das revisões de código.

Existe um mantra que ninguém parece ter citado ainda: Faça check-in antecipado e frequente :

Os desenvolvedores que trabalham por longos períodos - e, por muito tempo, quero dizer mais de um dia - sem verificar nada no controle de origem estão se preparando para algumas sérias dores de cabeça de integração na linha. Damon Poole concorda :

Os desenvolvedores costumam adiar o check-in. Eles adiam porque não querem afetar outras pessoas muito cedo e não querem ser responsabilizados por quebrar a compilação. Mas isso leva a outros problemas, como a perda de trabalho ou a impossibilidade de voltar às versões anteriores.

Minha regra geral é "fazer check-in antecipado e frequente", mas com a ressalva de que você tem acesso a versões particulares. Se um check-in for imediatamente visível para outros usuários, você corre o risco de introduzir alterações imaturas e / ou interromper a compilação.

Prefiro que pequenos fragmentos sejam verificados periodicamente do que passar longos períodos sem ter idéia do que meus colegas de trabalho estão escrevendo. No que me diz respeito, se o código não estiver verificado no controle de origem, ele não existe . Suponho que essa seja mais uma forma de Don't Go Dark ; o código fica invisível até que exista no repositório de alguma forma.

... Se você aprender a fazer o check-in antecipado e o check-in com frequência, terá tempo suficiente para feedback, integração e revisão ao longo do caminho ...

Eu trabalhei para algumas empresas que tinham abordagens diferentes para isso. Um permitiu, desde que não impedisse a compilação. O outro ficaria assustado se você verificasse algum erro. O primeiro é muito preferido. Você deve estar se desenvolvendo em um tipo de ambiente que permita colaborar com outras pessoas em coisas que ainda estão em andamento, com o entendimento de que tudo será testado mais tarde.

Há também a afirmação de Jeff Atwood: Não tenha medo de quebrar as coisas :

A maneira mais direta de melhorar como desenvolvedor de software é não ter medo de mudar seu código. Desenvolvedores que têm medo de código quebrado são desenvolvedores que nunca amadurecerão em profissionais.

Eu também acrescentaria que, para revisões por pares, se alguém quiser que você mude alguma coisa, ter o histórico da sua versão original na fonte é uma ferramenta de aprendizado muito valiosa.

Recentemente, comecei a fazer revisões de pré-confirmação em um projeto em que estou e devo dizer que estou agradavelmente surpreso com o quão problemático é.

A maior desvantagem das revisões pós-confirmação que vejo é que geralmente é um caso de uma única pessoa: alguém analisa o código para correção, mas o autor geralmente não está envolvido, a menos que haja um erro grave. Pequenos problemas, sugestões ou sugestões geralmente não chegam ao autor.

Isso também muda o resultado percebido das revisões de código: é visto como algo que apenas produz trabalho adicional, em oposição a algo em que todos (o revisor e o autor do código) podem aprender coisas novas sempre.

As revisões de código pré-confirmação parecem tão naturais que nunca me ocorreu que as revisões pudessem ser feitas deliberadamente após a confirmação. Do ponto de vista da integração contínua, você deseja confirmar seu código quando ele terminar, não em um estado de trabalho, mas possivelmente mal escrito, certo?

Talvez seja porque a maneira como sempre fizemos em minhas equipes é o diálogo ao vivo iniciado pelo desenvolvedor original, mas não as avaliações assíncronas, controladas por controle e baseadas em documentos.

Atualmente, a maioria dos repositórios suporta um commit em duas fases ou um shelveset (ramificação privada, solicitação pull, envio de patch ou o que você quiser chamar), que permitirá que você inspecione / revise o trabalho antes de colocá-lo na linha principal. Eu diria que o aproveitamento dessas ferramentas permitiria que você sempre faça revisões pré-confirmadas.

Além disso, você pode considerar a codificação de pares (pares sênior com junior) como outra maneira de fornecer uma revisão de código integrada. Considere isso como uma inspeção de qualidade na linha de montagem, em vez de depois que o carro decolou.

Faz ambos :

• pré-confirmação - faça esse tipo de revisão quando for algo muito importante, como um código muito reutilizável ou uma decisão importante do projeto
• post commit - faça esse tipo de revisão quando quiser obter opinião sobre um pedaço de código que pode ser aprimorado

Qualquer revisão formal deve ser realizada nos arquivos de origem que estão sob controle de configuração, e os registros de revisão indicam claramente a revisão do arquivo.

Isso evita qualquer argumento do tipo "você não possui o arquivo mais recente" e garante que todos estejam revendo a mesma cópia do código-fonte.

Isso também significa que, caso sejam necessárias correções pós-revisão, o histórico pode ser anotado com esse fato.

Para a revisão do código, meu voto é para 'durante' o commit.

Um sistema como o gerrit ou o trevo (eu acho) pode realizar uma mudança e pedir ao revisor que o comprometa com o controle de origem (push git), se for bom. Esse é o melhor do mundo.

Se isso não for prático, acho que depois do commit é o melhor compromisso. Se o design for bom, apenas os desenvolvedores mais jovens devem ter coisas ruins o suficiente para que você nunca os queira comprometidos. (Faça uma revisão prévia para eles).

O que leva à revisão do design - enquanto você pode fazer isso no momento da revisão do código (ou, nesse caso, no momento da implantação do cliente), a localização de problemas de design deve ser feita antes disso - antes que o código seja realmente escrito.

Com a revisão por pares, há um risco mínimo de perda de controle. O tempo todo, duas pessoas têm conhecimento sobre o mesmo código. Eles precisam mudar ocasionalmente, portanto precisam estar sempre atentos para acompanhar o código.

Faz sentido ter um desenvolvedor habilidoso e um novato trabalhando juntos. À primeira vista, isso parece ser ineficiente, mas não é. De fato, há menos erros e leva menos tempo para corrigi-los. Além disso, os novatos aprenderão muito mais rápido.

O que ocorre com a prevenção de um design incorreto deve ser feito antes da codificação. Se houver alguma alteração / melhoria / nova peça de design considerável, ela deverá ser revisada antes do início da codificação. Quando o design é completamente desenvolvido, não há muito o que fazer. A revisão do código será mais fácil e levará menos tempo.

Concordo que não é essencial revisar o código antes de confirmar apenas se o código for produzido por um desenvolvedor experiente, que já tenha comprovado suas habilidades. Mas se houver um novato, o código deve ser revisado antes da confirmação: o revisor deve sentar-se ao lado do desenvolvedor e explicar todas as alterações ou melhorias feitas por ele.

As análises se beneficiam de pré e pós-confirmações.

Confirmação de pré-revisão

• Dá aos revisores a confiança de que estão revisando a revisão mais recente do autor.
• Ajuda a garantir que todos analisem o mesmo código.
• Fornece uma referência para comparação quando as revisões feitas a partir dos itens de revisão estiverem concluídas.

Nenhuma execução em execução durante a revisão

Eu usei ferramentas Atlassian e vi commits em execução durante a revisão. Isso é confuso para os revisores, por isso eu recomendo.

Revisões pós-revisão

Após os revisores concluírem seus comentários, verbalmente ou por escrito, o moderador deve garantir que o autor faça as alterações solicitadas. Às vezes, os revisores ou o autor podem discordar quanto à designação de um item de revisão como uma falha, sugestão ou investigação. Para resolver desacordos e garantir que os itens da investigação sejam limpos corretamente, a equipe de revisão depende do julgamento do moderador.

Minha experiência com cerca de 100 inspeções de código é que, quando os revisores podem fazer referência a um padrão de codificação inequívoco, e para a maioria dos tipos de lógica e outras falhas de programação, os resultados da revisão geralmente são claros. Ocasionalmente, há um debate sobre a nitidez ou um ponto de estilo pode degenerar em argumento. No entanto, dar poder de decisão ao moderador evita o empate.

Compromisso pós-revisão

• Dá ao moderador e a outros revisores um ponto de dados para comparar com a confirmação da pré-revisão.
• Fornece métricas para avaliar o valor e o sucesso da revisão na remoção de defeitos e aprimoramento de código.

Depende da sua equipe. Para uma equipe relativamente experiente, que é boa em commits pequenos e frequentes, depois da revisão pós-commit apenas para obter um segundo par de olhos no código, tudo bem. Para confirmações maiores e mais complexas e / ou para desenvolvedores menos experientes, a pré-confirmação das revisões para corrigir os problemas antes que eles entrem parece mais prudente.

Nesse sentido, ter um bom processo de IC e / ou check-ins fechados diminui a necessidade de revisões antes da confirmação (e, sem dúvida, após a confirmação para muitas delas).

Eu e meus colegas fizemos algumas pesquisas científicas sobre esse tópico recentemente, então gostaria de acrescentar algumas de nossas idéias, apesar de essa ser uma pergunta bastante antiga. Criamos um modelo de simulação de um processo / equipe de desenvolvimento Kanban ágil e comparamos a análise de pré-confirmação e pós-confirmação para um grande número de situações diferentes (número diferente de membros da equipe, diferentes níveis de habilidade ...). Analisamos os resultados após 3 anos de tempo de desenvolvimento (simulado) e procuramos diferenças em relação à eficiência (pontos de história finalizados), qualidade (erros encontrados pelos clientes) e tempo de ciclo (tempo do início à entrega de uma história do usuário) . Nossas descobertas são as seguintes:

• As diferenças em termos de eficiência e qualidade são insignificantes em muitos casos. Quando não estão, a revisão pós-confirmação traz alguns benefícios em relação à qualidade (outros desenvolvedores agem como "beta-testers" de certa forma). Para eficiência, o pós-commit possui alguns benefícios em equipes pequenas e o pré-commit possui alguns benefícios em equipes grandes ou não qualificadas.
• A revisão pré-confirmação pode levar a tempos de ciclo mais longos, quando você tem uma situação em que o início das tarefas dependentes é atrasado pela revisão.

Dessas, derivamos as seguintes regras heurísticas:

• Quando você tem um processo de revisão de código estabelecido, não se preocupe em mudar, provavelmente não vale o esforço
  • a menos que você tenha problemas com o tempo de ciclo => Alternar para postagem
  • Ou problemas reduzidos atrapalham seus desenvolvedores com muita frequência => Alternar para Pré
• Se você ainda não está fazendo críticas
  • Use pré-confirmação se um desses benefícios for aplicável a você
    • A revisão pré-confirmação permite que pessoas de fora sem direitos de contribuição contribuam para projetos de código aberto
    • Se baseada em ferramentas, a revisão pré-confirmação impõe alguma disciplina de revisão em equipes com aderência ao processo
    • A revisão pré-confirmação facilmente impede que alterações não revisadas sejam entregues, o que é ótimo para implantação contínua / ciclos de liberação muito curtos
  • Use pré-confirmação se sua equipe for grande e você puder conviver ou contornar os problemas no tempo de ciclo
  • Caso contrário (por exemplo, pequena equipe industrial qualificada), use pós-confirmação
• Procure combinações que ofereçam os benefícios dos dois mundos (não as estudamos formalmente)

O trabalho completo de pesquisa está disponível aqui: http://dx.doi.org/10.1145/2904354.2904362 ou no meu site: http://tobias-baum.de

Na minha opinião, a revisão por pares de código funciona melhor se for feita após a confirmação.

Eu recomendaria ajustar sua estratégia de ramificação. O uso de uma ramificação de desenvolvedor ou ramificação de recursos tem vários benefícios ... o menos importante é facilitar as análises de código pós-confirmação.

Uma ferramenta como o Crisol facilitará e automatizará o processo de revisão. Você pode selecionar um ou mais conjuntos de alterações confirmados para incluir na revisão. No Crisol, ele mostrará quais arquivos foram tocados nos conjuntos de alterações selecionados, acompanhará quais arquivos cada revisor já leu (mostrando uma porcentagem total concluída) e permitirá que os revisores façam comentários com facilidade.

http://www.atlassian.com/software/crucible/overview

Alguns outros benefícios das ramificações de usuário / recurso:

• Os desenvolvedores obtêm os benefícios do controle de versão (faça backup de alterações, restaure o histórico, faça alterações) com menos preocupação em quebrar o sistema para todos os outros.
• Alterações que causam defeitos ou não são concluídas no prazo podem ser recuperadas, priorizadas ou arquivadas, se necessário.

Para desenvolvedores inexperientes, uma consulta regular com um mentor e / ou programação em pares é uma boa idéia, mas eu não consideraria isso uma "revisão de código".

Ambos. (Mais ou menos.)

Você deve revisar seu próprio código resumidamente antes de enviá-lo. No Git, acho que a área de preparação é ótima. Depois de organizar minhas alterações, corro git diff --cachedpara ver tudo o que é organizado. Uso isso como uma oportunidade para garantir que não esteja verificando arquivos que não pertencem (criar artefatos, logs etc.) e garantir que não haja nenhum código de depuração ou nenhum código importante comentado Fora. (Se estou fazendo algo que sei que não quero fazer check-in, geralmente deixo um comentário em maiúsculas para que eu o reconheça durante a preparação.)

Dito isto, sua revisão de código de pares geralmente deve ser realizada após a confirmação, assumindo que você esteja trabalhando em uma ramificação de tópicos. Essa é a maneira mais fácil de garantir que todos os outros estejam analisando a coisa correta e, se houver grandes problemas, não é importante corrigi-los em sua filial ou excluí-los e começar de novo. Se você realizar revisões de código de forma assíncrona (por exemplo, usando o Google Code ou o Atlassian Crucible), poderá alternar facilmente ramificações e trabalhar em outra coisa sem ter que acompanhar separadamente todos os seus diferentes patches / diferenças que estão atualmente em revisão por alguns dias.

Se você não está trabalhando em uma ramificação de tópico, deve . Reduz o estresse e os aborrecimentos e torna o planejamento de lançamentos muito menos estressante e complicado.

Editar: devo acrescentar também que você deve revisar o código após o teste, que é outro argumento a favor da confirmação do código primeiro. Você não quer que seu grupo de teste se divirta com dezenas de patches / diffs de todos os programadores e, em seguida, arquive erros apenas porque eles aplicaram o patch errado no lugar errado.

Programação 100% emparelhada (não importa o quanto você pense que é mais velha) com muitas pequenas confirmações e um sistema de CI baseado em TODAS as confirmações (com testes automatizados incluindo unidades, integração e funcionalidade, sempre que possível). Revisões pós-confirmação para alterações grandes ou arriscadas. Se você precisar de algum tipo de revisão fechada / pré-confirmação, o Gerrit funcionará.

A vantagem da revisão de código no check-in (verificação de amigo) é o feedback imediato, antes que grandes partes do código sejam concluídas.

A desvantagem da revisão de código no check-in é que ela pode desencorajar as pessoas de fazer check-in até que longas extensões de código sejam concluídas. Se isso acontecer, nega completamente a vantagem.

O que torna isso mais difícil é que nem todo desenvolvedor é o mesmo. Soluções simples não funcionam para todos os programadores . Soluções simples são:

• Programação de pares obrigatórios, que permite fazer check-ins frequentes porque o amigo está ao seu lado. Isso ignora que a programação em pares nem sempre funciona para todos. Feito corretamente, a programação em pares também pode ser muito cansativa, por isso não é necessariamente algo para fazer o dia todo.

• Ramos do desenvolvedor, o código é revisado e verificado somente no ramo principal quando concluído. Alguns desenvolvedores tendem a trabalhar em sigilo e, após uma semana, criam algum código que pode ou não passar na revisão devido a problemas fundamentais que poderiam ter sido detectados anteriormente.

• Faça uma revisão em cada check-in, o que garante avaliações frequentes. Alguns desenvolvedores são esquecidos e dependem de check-ins muito frequentes, o que significa que outros precisam fazer análises de código a cada 15 minutos.

• Revise em algum momento não especificado após o check-in. As críticas serão mais ampliadas quando houver uma restrição no prazo. O código que depende do código já confirmado, mas ainda não revisado, será confirmado. As revisões sinalizarão problemas e os problemas serão colocados no backlog para serem corrigidos "mais tarde". Ok, eu menti: Esta não é uma solução simples, não é uma solução. Revise em um horário especificado após o check-in funcionar, mas é menos simples porque você precisa decidir qual é esse horário especificado

Na prática, você faz esse trabalho, tornando-o ainda mais simples e mais complexo ao mesmo tempo. Você define diretrizes simples e permite que cada equipe de desenvolvimento descubra como uma equipe o que elas precisam fazer para seguir essas diretrizes. Um exemplo dessas diretrizes é:

• O trabalho é dividido em tarefas que devem levar menos de um dia.
• Uma tarefa não será concluída se o código (se houver) não tiver sido verificado.
• Uma tarefa não será concluída se o código (se houver) não tiver sido revisado.

Muitas formas alternativas de tais diretrizes são possíveis. Concentre-se no que você realmente deseja (código revisado por pares, progresso do trabalho observável, responsabilidade) e deixe a equipe descobrir como eles podem lhe dar o que querem.

na verdade, fazemos um híbrido no LedgerSMB. Os confirmadores confirmam as alterações que são revisadas depois. Os não confirmados enviam alterações aos confirmadores para serem revisados ​​antes. Isso tende a significar duas camadas de revisão. Primeiro, você recebe um mentor para revisá-lo e orientá-lo. Então esse mentor recebe o código revisado uma segunda vez depois que ele ou ela assinam e o feedback circula. Os novos committers geralmente gastam muito tempo revisando os commit de outras pessoas no início.

Funciona muito bem. A questão, porém, é que uma revisão depois é geralmente mais superficial do que uma revisão anterior, portanto, você deseja garantir que a revisão posterior seja reservada para aqueles que se comprovaram. Mas se você tiver uma revisão em dois níveis para as novas pessoas, isso significa que é mais provável que os problemas sejam detectados e que houve discussões.

Confirmar onde? Há um ramo que eu criei para fazer algum trabalho. Comprometo-me com esse ramo sempre que me apetecer. Não é da conta de ninguém. Então, em algum momento, esse ramo é integrado a um ramo de desenvolvimento. E em algum lugar no meio é uma revisão de código.

O revisor revisa obviamente depois que eu me comprometo com minha filial. Ele não está sentado na minha mesa, então não pode revisar antes que eu me comprometa com o meu ramo. E ele analisa antes da mesclagem e se compromete com o ramo de desenvolvimento.

Apenas não faça revisões de código. Você acredita que seus desenvolvedores são capazes de escrever um bom código ou deve se livrar deles. Erros na lógica devem ser detectados pelos seus testes automatizados. Erros é estilo devem ser detectados pelas ferramentas de análise estática e de cotão.

Ter seres humanos envolvidos no que deveria ser processos automáticos é apenas um desperdício.