Revise antes ou depois do código commit, o que é melhor?

Como Simon Whitehead menciona em seu comentário , 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 de qualquer maneira), eu executaria a revisão de código antes de mesclar com o tronco ou repositório principal. Isso permitirá que os desenvolvedores tenham a liberdade de fazer o check-in com a frequência que quiserem durante seu ciclo de desenvolvimento / teste, mas qualquer código de tempo vai para o ramo que contém o código entregue, e é revisado.

Geralmente, suas experiências ruins com as revisões de código parecem mais um problema com o processo de análise que possui soluções. Ao analisar o código em partes 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 (uma segurança crítica requer mais tempo) essa informação pode ser útil 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: Check-in antecipado e com frequência :

Developers who work for long periods -- and by long I mean more than a day -- without checking anything into source control are setting themselves up for some serious integration headaches down the line. Damon Poole concurs:

Developers often put off checking in. They put it off because they don't want to affect other people too early and they don't want to get blamed for breaking the build. But this leads to other problems such as losing work or not being able to go back to previous versions.

My rule of thumb is "check-in early and often", but with the caveat that you have access to private versioning. If a check-in is immediately visible to other users, then you run the risk of introducing immature changes and/or breaking the build.

     

Eu prefiro ter pequenos fragmentos checados periodicamente do que ir longos períodos sem nenhuma ideia do que meus colegas estão escrevendo. Tanto quanto eu estou preocupado, se o código não é verificado no controle de origem, não existe . Suponho que essa seja mais uma forma de Don't Go Dark ; o código é invisível até que exista no repositório de alguma forma.

     

... Se você aprender a fazer check-in cedo e fazer 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. Uma permitia, desde que não impedisse a compilação. O outro enlouqueceria se você registrasse algum erro. O primeiro é muito preferido. Você deveria estar desenvolvendo em um tipo de ambiente que permitiria que você colaborasse com outras pessoas em coisas que ainda estão em andamento, com o entendimento de que tudo será testado depois.

Há também a declaração de Jeff Atwood: Não seja com medo de quebrar as coisas :

The most direct way to improve as a software developer is to be absolutely fearless when it comes to changing your code. Developers who are afraid of broken code are developers who will never mature into professionals.

Também gostaria de acrescentar que, para revisões por pares, se alguém quiser que você altere algo, ter o histórico da sua versão original na origem é uma ferramenta de aprendizado muito valiosa.

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

A maior desvantagem das revisões pós-commit que vejo é que muitas vezes é uma coisa só para uma 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 dicas geralmente não chegam ao autor.

Isso também altera o resultado percebido das revisões de código: ele é 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 de pré-consolidação parecem tão naturais que nunca me ocorreram comentários que pudessem ser feitos deliberadamente após o envio. A partir de uma perspectiva de integração contínua, você quer comprometer seu código assim que estiver concluído, não em um estado de trabalho, mas possivelmente mal escrito, certo?

Talvez seja porque a maneira como sempre fizemos isso em minhas equipes é o diálogo ao vivo iniciado pelo desenvolvedor original, mas não com base em documentos assíncronos, controlados por controle.

A maioria dos repositórios hoje suporta um commit de duas fases ou um shelveset (private branch, pull request, patch submission ou o que você quiser chamá-lo), que lhe permitirá inspecionar / revisar o trabalho antes de puxá-lo para a linha principal. Eu diria que alavancar essas ferramentas permitiria que você sempre fizesse avaliações prévias.

Além disso, você pode considerar a codificação por pares (pares seniores com junior) como outra maneira de fornecer uma revisão de código incorporada. Considere-o como uma inspeção de qualidade na linha de montagem, em vez de depois que o carro se desfez.

Faça as duas coisas:

• pre commit - faça esse tipo de comentário quando for algo muito importante, como um código muito reutilizável ou uma decisão de design importante
• post commit - faça esse tipo de resenhas quando quiser obter uma opinião sobre um código que pode ser melhorado

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

Isso evita argumentos do tipo "você não tem o arquivo mais recente" e garante que todos estejam revisando a mesma cópia do código-fonte.

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

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

Um sistema como o gerrit ou o trevo (eu acho) pode organizar uma mudança e então fazer com que o revisor a confirme no controle de origem (push in git) se for bom. Esse é o melhor dos dois mundos.

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

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

Com revisão por pares, existe um risco mínimo de perder o controle. O tempo todo duas pessoas têm conhecimento sobre o mesmo código. Eles têm que trocar de vez em quando, então precisam ficar atentos o tempo todo 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 é. Na verdade, há menos bugs e leva menos tempo para consertá-los. Além disso, os novatos aprenderão muito mais rápido.

O que vem impedindo o mau design, isso deve ser feito antes da codificação. Se houver alguma mudança / melhoria / nova peça de design considerável, ela deve ser revisada antes do início da codificação. Quando o design está completamente desenvolvido, não resta muito a fazer. Rever o código será mais fácil e levará menos tempo.

Concordo que não é essencial rever o código antes de cometer apenas se o código for produzido por um desenvolvedor experiente, que já comprovou suas habilidades. Mas se houver um novato, o código deve ser revisado antes de se comprometer: o revisor deve se sentar ao lado do desenvolvedor e explicar cada alteração ou melhoria feita por ele.

As revisões beneficiam de pré e pós-commits.

Confirmar cometer uma pré-revisão

• Dá aos revisores confiança de que estão revisando a última revisão do autor.
• Ajuda a garantir que todos revisem o mesmo código.
• Dá uma referência para comparação, uma vez que as revisões feitas a partir dos itens de revisão estão completas.

Nenhum compromisso é executado durante a revisão

Eu usei ferramentas da Atlassian e vi comissões de execução ocorrerem durante a revisão. Isso é confuso para os revisores, então eu não recomendo.

Revisões posteriores à revisão

Após os revisores preencherem 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 a designar um item de revisão como uma falha, sugestão ou uma investigação. Para resolver divergências e garantir que os itens de 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 não ambíguo, e para a maioria dos tipos de lógica e outras falhas de programação, os resultados da análise são geralmente claros. Ocasionalmente, há um debate sobre nit-picking ou um ponto de estilo pode degenerar para argumentar. No entanto, dar poder de decisão ao moderador evita o impasse.

Commit pós-revisão

• Concede ao moderador e a outros revisores um ponto de dados para comparar com a confirmação anterior à revisão.
• Fornece métricas para avaliar o valor e o sucesso da revisão na remoção de defeitos e na melhoria do código.

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

Nessa linha, ter um bom processo de IC e / ou check-ins fechados diminui a necessidade de revisões antes de se comprometer (e possivelmente postar commit para muitos deles).

Eu e meus colegas fizemos algumas pesquisas científicas sobre esse tópico recentemente, então gostaria de acrescentar algumas de nossas percepções, apesar de essa ser uma pergunta bastante antiga. Construímos um modelo de simulação de um processo / equipe ágil de desenvolvimento Kanban e comparamos a revisão pré-commit e post commit para um grande número de situações diferentes (diferentes números 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 finais da história), qualidade (erros encontrados pelos clientes) e tempo de ciclo (tempo desde o início até a entrega de uma história do usuário) . Nossos resultados são os 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-commit tem alguns benefícios com relação à qualidade (outros desenvolvedores agem como "beta-testers" de certa forma). Por eficiência, o post commit tem alguns benefícios em pequenas equipes e o pré-commit tem alguns benefícios em equipes grandes ou não especializadas.
• A análise pré-commit 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.

Destes, derivamos as seguintes regras heurísticas:

• Quando você tem um processo de revisão de código estabelecido, não se incomode em mudar, provavelmente não vale o esforço
  • , a menos que você tenha problemas com o tempo de ciclo = > Mude para o Post
   
  • Ou problemas escorregados atrapalham seus desenvolvedores com muita frequência = > Mude para Pre
• Se você ainda não está fazendo avaliações
  • Use o pré-commit se um desses benefícios for aplicável a você
    • A análise pré-commit permite que pessoas de fora, sem se comprometerem, contribuam para projetos de código aberto
    • Se a revisão de pré-commit baseada em ferramentas impuser alguma disciplina de revisão em equipes com aderência de processo negligente
    • A análise pré-comprometida facilita que as alterações não revisadas sejam entregues, o que é ótimo para implantação contínua / ciclos de lançamento muito curtos
  • Use o pré-commit se sua equipe for grande e você puder viver com ou contornar os problemas no tempo de ciclo
  • Caso contrário (por exemplo, equipe industrial qualificada e pequena), use post commit
• Procure combinações que lhe proporcionem os benefícios de ambos os mundos (não os estudamos formalmente)

O artigo completo de pesquisa está disponível aqui: link ou no meu site: link

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

Eu recomendaria ajustar sua estratégia de ramificação. O uso de um ramo de desenvolvedor ou ramo de recursos tem vários benefícios ... e nenhum deles facilita a revisão de código pós-commit.

Uma ferramenta como o Crucible irá suavizar e automatizar o processo de revisão. Você pode selecionar um ou mais changesets confirmados para incluir na revisão. O Crucible mostrará quais arquivos foram tocados nos conjuntos de alterações selecionados, rastreará quais arquivos cada revisor já leu (mostrando um% completo no geral) e permitirá que os revisores façam comentários facilmente.

link

Alguns outros benefícios dos ramos de usuário / recurso:

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

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

Ambos. (Kind of.)

Você deve rever seu próprio código antes de enviá-lo. No Git, acho que a área de teste é ótima. Depois de organizar minhas alterações, corro git diff --cached para ver tudo o que é preparado. Eu uso isso como uma oportunidade para ter certeza de que não estou verificando nenhum arquivo que não pertença (constrói artefatos, logs, etc.) e certificando-se de que eu não tenha nenhum código de depuração lá ou qualquer código importante comentado Fora. (Se estou fazendo algo que sei que não quero fazer check-in, geralmente deixo um comentário em letras maiúsculas para que eu o reconheça durante a encenação.)

Se você não estiver trabalhando em um ramo de tópico, você deve . Reduz o estresse e o aborrecimento e torna o planejamento da liberação muito menos estressante e complicado.

Edit: Eu também devo acrescentar que você deveria estar fazendo uma revisão de código após o teste, que é outro argumento a favor de confirmar o código primeiro. Você não quer que seu grupo de teste fique brincando com dezenas de patches / diffs de todos os programadores, e então arquivando bugs só porque eles aplicaram o patch errado no lugar errado.

Programação 100% pareada (independentemente do nível de senioridade que você ache que é) com muitos commits pequenos e um sistema de CI que se baseia em CADA confirmação (com testes automatizados incluindo unidades, integração e funcionalidade sempre que possível). Revisões pós-commit para mudanças grandes ou arriscadas. Se você deve ter algum tipo de revisão de gated / pre-commit, Gerrit funciona.

A vantagem da revisão de código no check-in (verificação de amigo) é um 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 o check-in até que longos trechos de código sejam concluídos. 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 cansativa, por isso não é necessariamente algo para fazer o dia todo.

• Filiais de desenvolvedores, o código só é revisado e verificado na ramificação principal quando concluído. Alguns desenvolvedores estão propensos a trabalhar em sigilo e, depois de uma semana, criam algum código que pode ou não passar na revisão devido a problemas fundamentais que poderiam ter sido vistos antes.

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

• Revise em algum momento não especificado após o check-in. As avaliações serão mais divulgadas quando houver uma crise no prazo final. O código que depende do código já consolidado, mas ainda não revisado, será confirmado. As revisões sinalizarão os 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 algum momento especificado após o check-in, mas é menos simples porque você precisa decidir qual é o tempo 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 determine como uma equipe o que elas precisam fazer para seguir essas diretrizes. Um exemplo de tais 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 quer (código revisado por pares, progresso de trabalho observável, responsabilidade) e deixe que a equipe descubra como eles podem dar o que eles querem.

na verdade, fazemos um híbrido no LedgerSMB. Committers confirmam alterações que são revisadas depois. Os não-committers submetem alterações aos committers para serem revisadas antes. Isso tende a significar dois níveis de revisão. Primeiro você recebe um mentor para revisar e orientar você. Então esse mentor recebe o código revisto uma segunda vez depois que ele ou ela assinou e o feedback circula. Em geral, os novos committers gastam muito tempo revisando os commits de outras pessoas.

Funciona muito bem. A coisa é uma revisão depois é geralmente mais superficial do que uma revisão antes, então você quer ter certeza de que a revisão depois é reservada para aqueles que provaram-se. Mas se você tiver uma revisão de dois níveis para o novo pessoal, isso significa que os problemas provavelmente serão capturados e as discussões ocorreram.

Commit onde? Há um ramo que criei para fazer algum trabalho. Eu me comprometo com esse ramo sempre que tenho vontade. 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.

As revisões do revisor obviamente após eu me comprometi com minha filial. Ele não está sentado na minha mesa, então ele não pode revisá-lo antes de eu me comprometer com a minha minha ramificação. E ele revisa antes da mesclagem e se compromete com o ramo de desenvolvimento.

Apenas não faça revisões de código. Ou você acredita que seus desenvolvedores são capazes de escrever bons códigos, ou você deve se livrar deles. Erros na lógica devem ser detectados pelos seus testes automatizados. Erros é estilo deve ser pego por ferramentas de análise de fiapos e estática.

Ter humanos envolvidos no que deveria ser um processo automático é apenas um desperdício.