Controlla prima o dopo il commit del codice, che è meglio?

Come Simon Whitehead menziona nel suo commento , dipende dalla tua strategia di ramificazione.

Se gli sviluppatori hanno il loro ramo privato per lo sviluppo (che consiglio comunque nella maggior parte delle situazioni), eseguirò la revisione del codice prima di unirmi al trunk o al repository principale. Ciò consentirà agli sviluppatori di avere la libertà di effettuare il check-in tutte le volte che vogliono durante il loro ciclo di sviluppo / test, ma qualsiasi codice temporale va nel ramo che contiene il codice consegnato, viene revisionato.

In generale, le tue brutte esperienze con le revisioni del codice sembrano più un problema con il processo di revisione che ha delle soluzioni. Esaminando il codice in blocchi più piccoli, puoi assicurarti che non ci voglia troppo tempo. Un buon numero è che 150 linee di codice possono essere riviste in un'ora, ma la velocità sarà più lenta per le persone che non hanno familiarità con il linguaggio di programmazione, il sistema in fase di sviluppo o la criticità del sistema (una sicurezza critica richiede più tempo) - questa informazione potrebbe essere utile per migliorare l'efficienza e decidere chi partecipa alle revisioni del codice.

C'è un mantra che nessuno sembra aver già citato: Accedi presto e spesso :

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.

     

Preferirei avere dei piccoli frammenti controllati periodicamente piuttosto che lunghi periodi senza alcuna idea di quello che i miei colleghi stanno scrivendo. Per quanto mi riguarda, se il codice non è controllato nel controllo del codice sorgente, non esiste . Suppongo che questa sia un'altra forma di Do not Go Dark ; il codice è invisibile finché non esiste nel repository in qualche modo.

     

... Se impari a fare il check-in in anticipo e a fare il check-in spesso, avrai molto tempo per feedback, integrazione e revisione lungo la strada ...

Ho lavorato per un paio di aziende che avevano approcci diversi in questo senso. Uno lo ha permesso, purché non impedisse la compilazione. L'altro sarebbe fuori di testa se avessi controllato qualche bug. Il primo è molto preferito. Dovresti svilupparti in un tipo di ambiente che ti consentirebbe di collaborare con altre persone su cose che sono ancora in corso, con la consapevolezza che tutto verrà testato in seguito.

C'è anche la dichiarazione di Jeff Atwood: Non essere paura di rompere le cose :

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.

Vorrei anche aggiungere che per le revisioni tra pari, se qualcuno vuole che tu cambi qualcosa, avere la cronologia della tua versione originale in origine è uno strumento di apprendimento molto prezioso.

Recentemente ho iniziato a fare revisioni pre-commit in un progetto in cui mi trovo e devo dire che sono piacevolmente sorpreso da quanto sia priva di problemi.

Il più grande svantaggio delle recensioni post-commit che vedo è che spesso è una sola persona afair: Qualcuno esamina il codice per correttezza, ma l' autore di solito non è coinvolto a meno che non ci sia un grave errore. Piccoli problemi, suggerimenti o suggerimenti di solito non raggiungono affatto l'autore.

Questo cambia anche il risultato percepito delle recensioni del codice: è visto come qualcosa che produce solo lavoro aggiuntivo, al contrario di qualcosa in cui tutti (il revisore e l'autore del codice) possono imparare ogni volta nuove cose.

Le recensioni dei codici pre-commit sembrano così naturali, non mi è mai venuto in mente che le recensioni potrebbero essere fatte deliberatamente dopo aver commesso. Da una prospettiva di integrazione continua, vuoi impegnare il tuo codice una volta terminato, non in uno stato funzionante, ma possibilmente mal scritto, vero?

Forse è perché il modo in cui lo abbiamo sempre fatto nei miei team è la finestra di dialogo live avviata dallo sviluppatore originale, non dalle recensioni asincrone, basate sui controlli e basate sui documenti.

La maggior parte dei repository oggi supporta un commit a due fasi o uno shelfet (ramo privato, richiesta pull, invio di patch o qualsiasi altra cosa tu voglia chiamare), che ti consentirà di ispezionare / revisionare il lavoro prima di inserirlo nella linea principale. Direi che sfruttando questi strumenti ti permetteremo di fare sempre recensioni pre-commit.

Inoltre, potresti considerare la codifica della coppia (coppie senior con junior) come un altro modo di fornire una revisione del codice integrata. Consideralo come un controllo di qualità sulla linea di assemblaggio invece che dopo che l'auto è rotolata via.

Fai entrambi:

• pre commit - fai questo tipo di recensioni quando è qualcosa di molto importante, come un pezzo di codice molto riutilizzabile, o una decisione progettuale importante
• post commit - fai questo tipo di recensioni quando vuoi avere un'opinione su un pezzo di codice che potrebbe essere migliorato

Qualsiasi revisione formale deve essere eseguita sui file di origine che si trovano sotto il controllo della configurazione e i record di revisione che indicano chiaramente la revisione del file.

Questo evita qualsiasi argomento di tipo "non hai il file più recente" e garantisce che tutti stiano rivedendo la stessa copia del codice sorgente.

Significa anche che, se sono necessarie correzioni post-revisione, la cronologia può essere annotata con questo fatto.

Per la revisione del codice, il mio voto è per "durante" il commit.

Un sistema come gerrit o clover (credo) può mettere in scena un cambiamento e quindi fare in modo che il revisore lo impegni al controllo del codice sorgente (push in git) se è buono. Questo è il meglio di entrambi i mondi.

Se ciò non è pratico, penso che dopo il commit è il miglior compromesso. Se il design è buono, solo gli sviluppatori più giovani dovrebbero avere cose abbastanza brutte da non volere che vengano mai commesse. (Esegui una revisione pre-commit per loro).

Che porta alla revisione del design - mentre puoi farlo al momento della revisione del codice (o per quel che riguarda il tempo di implementazione del cliente), la ricerca dei problemi di progettazione dovrebbe essere fatta prima - prima che il codice sia effettivamente scritto.

Con la revisione tra pari c'è un rischio minimo di perdere il controllo. Per tutto il tempo due persone hanno conoscenza dello stesso codice. Devono cambiare di tanto in tanto, quindi devono essere sempre attenti a tenere traccia del codice.

Ha senso avere uno sviluppatore abile e un principiante che lavorano insieme. A prima vista questo sembra essere inefficiente, ma non lo è. In effetti, ci sono meno bug e ci vuole meno tempo per sistemarli. Inoltre, i neofiti impareranno molto più velocemente.

Che cosa impedisce la progettazione errata, questo dovrebbe essere fatto prima della codifica. Se c'è qualche cambiamento / miglioramento / nuovo pezzo di design, dovrebbe essere rivisto prima che inizi la codifica. Quando il design è completamente sviluppato, non resta molto da fare. Revisionare il codice sarà più semplice e richiederà meno tempo.

Sono d'accordo che non è essenziale rivedere il codice prima di impegnarsi solo se il codice è prodotto da uno sviluppatore esperto, che ha già dimostrato le proprie capacità. Ma se c'è un principiante, il codice dovrebbe essere esaminato prima di impegnarsi: il revisore dovrebbe sedersi accanto allo sviluppatore e spiegare ogni cambiamento o miglioramento apportato da loro.

Le recensioni beneficiano sia di pre- e post-commit.

commit pre-riesame

• Fornisce ai revisori la sicurezza di rivedere l'ultima revisione dell'autore.
• Aiuta a garantire a tutti di rivedere lo stesso codice.
• Fornisce un riferimento per il confronto una volta completate le revisioni effettuate dagli elementi di revisione.

Nessuna esecuzione si commette durante la revisione

Ho usato strumenti Atlassian e ho visto che durante la revisione si sono verificati dei commit in esecuzione. Questo è fonte di confusione per i revisori, quindi consiglio di non farlo.

Revisioni post revisione

Dopo che i revisori hanno completato il loro feedback, verbalmente o per iscritto, il moderatore dovrebbe assicurarsi che l'autore apporti le modifiche richieste. A volte i revisori o l'autore possono non essere d'accordo sul designare un articolo di revisione come un errore, un suggerimento o un'indagine. Per risolvere i disaccordi e garantire che gli elementi di indagine siano stati chiariti correttamente, il team di revisione dipende dal giudizio del moderatore.

La mia esperienza con circa 100 ispezioni di codice è che quando i revisori possono fare riferimento a uno standard di codifica univoco e per la maggior parte dei tipi di errori di programmazione e logica, i risultati delle revisioni sono generalmente chiari. Occasionalmente c'è un dibattito sulla nit-picking o un punto di stile può degenerare in argomento. Tuttavia, conferire potere decisionale al moderatore evita lo stallo.

Impegno post-revisione

• Fornisce al moderatore e ad altri revisori un punto dati da confrontare con il commit pre-revisione.
• Fornisce le metriche per giudicare sia il valore che il successo della revisione alla rimozione dei difetti e al miglioramento del codice.

Dipende dal tuo team di truccarsi. Per un team relativamente esperto che è bravo con commit piccoli e frequenti, la revisione post-commit solo per ottenere un secondo paio di occhi sul codice va bene. Per committenti più grandi e complessi e / o per sviluppatori meno esperti, quindi, pre-commit delle recensioni per risolvere i problemi prima che entrino in azione sembra più prudente.

Lungo queste linee, avere un buon processo CI e / o gating check-in riduce la necessità di revisioni prima del commit (e probabilmente postare il commit per molte di esse).

Recentemente, io e i miei colleghi abbiamo fatto qualche ricerca scientifica su questo argomento, quindi vorrei aggiungere alcune delle nostre intuizioni, nonostante questa sia una domanda piuttosto vecchia. Abbiamo creato un modello di simulazione di un processo / team di sviluppo Kanban agile e abbiamo confrontato la verifica pre-commit e post-commit per un gran numero di situazioni diverse (numero diverso di membri del team, diversi livelli di abilità, ...). Abbiamo esaminato i risultati dopo 3 anni di tempo di sviluppo (simulato) e abbiamo cercato differenze in termini di efficienza (punti della storia finiti), qualità (bug rilevati dai clienti) e tempi di ciclo (tempo dall'inizio alla consegna di una storia utente) . I nostri risultati sono i seguenti:

• Le differenze in termini di efficienza e qualità sono trascurabili in molti casi. Quando non lo sono, la revisione del commit post ha alcuni vantaggi in termini di qualità (altri sviluppatori fungono da "beta-tester" in un certo senso). Per efficienza, il post-commit ha alcuni vantaggi in piccoli team e il pre-commit ha alcuni vantaggi in team di grandi dimensioni o non qualificati.
• La revisione preliminare del commit può portare a tempi di ciclo più lunghi, quando si ha una situazione in cui l'inizio delle attività dipendenti è ritardato dalla revisione.

Da questi abbiamo ricavato le seguenti regole euristiche:

• Quando hai un processo di revisione del codice stabilito, non preoccuparti di cambiare, probabilmente non ne vale la pena
  • a meno che tu non abbia problemi con il tempo di ciclo = > Passa al post
   
  • Oppure i problemi con scivoloni disturbano troppo spesso i tuoi sviluppatori = > Passa al Pre
• Se non stai ancora recensendo
  • Utilizza il pre-commit se uno di questi vantaggi è applicabile per te
    • La revisione preliminare del commit consente agli estranei di non impegnare i diritti a contribuire a progetti open source
    • Se basato sugli strumenti, la revisione pre commit applica una certa disciplina di revisione in team con aderenza al processo altrimenti lassista
    • La revisione preliminare del commit consente di mantenere facilmente le modifiche non verificate dalla distribuzione, il che è accurato per la distribuzione continua / cicli di rilascio molto brevi
  • Usa il pre-commit se la tua squadra è grande e puoi vivere con o eludere i problemi nel tempo di ciclo
  • Altrimenti (ad esempio, un team industriale di piccole dimensioni) utilizza il post commit
• Cerca le combinazioni che ti offrono i vantaggi di entrambi i mondi (non li abbiamo studiati formalmente)

Il documento completo di ricerca è disponibile qui: link o sul mio sito web: link

Secondo me, la peer review del codice funziona meglio se fatta post-commit.

Consiglierei di adattare la strategia di ramificazione. L'utilizzo di un ramo di sviluppo o di un ramo di funzionalità ha un certo numero di vantaggi ... non ultimo il quale facilita le recensioni di post-commit del codice.

Uno strumento come Crucible faciliterà e automatizzerà il processo di revisione. È possibile selezionare uno o più changeset impegnati da includere nella revisione. Crucible mostrerà quali file sono stati toccati nei changeset selezionati, tieni traccia di quali file sono già stati letti da ogni revisore (mostrando una percentuale completa in generale) e lascia che i revisori facciano facilmente commenti.

link

Alcuni altri vantaggi dei rami utente / funzione:

• Gli sviluppatori ottengono i vantaggi del controllo della versione (modifiche di backup, ripristino dalla cronologia, cambiamenti di differenze) con meno preoccupazioni sulla rottura del sistema per tutti gli altri.
• Le modifiche che causano difetti o non vengono completate in tempo possono essere annullate, rideterminate le priorità o accantonate se necessario.

Per gli sviluppatori inesperti, una normale consultazione con un mentore e / o una programmazione di coppie è una buona idea, ma non la considererei una "revisione del codice".

Entrambi. (Tipo di.)

Dovresti riesaminare sommariamente il tuo codice prima di eseguirlo. In Git, penso che l'area di sosta sia grandiosa. Dopo aver messo in scena le mie modifiche, eseguo git diff --cached per vedere tutto ciò che è in scena. Lo uso come un'opportunità per assicurarmi che non stia verificando alcun file a cui non appartenga (costruisci artefatti, log, ecc.) E assicurandomi che non disponga di alcun codice di debug o di codice importante commentato su. (Se sto facendo qualcosa che so che non voglio controllare, di solito lascia un commento in maiuscolo in modo che lo riconosca durante la messa in scena.)

Detto questo, la revisione del tuo peer code dovrebbe essere generalmente condotta dopo il commit, supponendo che tu stia lavorando su un ramo dell'argomento. Questo è il modo più semplice per assicurarsi che tutti gli altri stiano rivedendo la cosa giusta, e se ci sono grossi problemi, non è un grosso problema risolverli sul tuo ramo o eliminarli e ricominciare da capo. Se conduci le revisioni del codice in modo asincrono (ovvero utilizzando Google Code o Atlassian Crucible), puoi facilmente cambiare ramo e lavorare su qualcos'altro senza dover tenere traccia di tutte le diverse patch / diff che sono attualmente in revisione per alcuni giorni.

Se non lavori su un ramo dell'argomento, dovresti . Riduce lo stress e la seccatura e rende la pianificazione del rilascio molto meno stressante e complicata.

Modifica: dovrei anche aggiungere che dovresti eseguire la revisione del codice dopo il test, che è un altro argomento a favore del commit del codice per primo. Non vuoi che il tuo gruppo di test armeggi con dozzine di patch / diff da tutti i programmatori, quindi archivi i bug solo perché hanno applicato la patch sbagliata nel posto sbagliato.

Programmazione accoppiata al 100% (indipendentemente da quanto tu pensi che tu sia senior) con un sacco di piccoli commit e un sistema di CI che si basa su OGNI commit (con test automatizzati che includono unità, integrazione e funzionalità ove possibile). Revisioni post-commit per modifiche grandi o rischiose. Se devi avere qualche tipo di revisione gated / pre-commit, Gerrit funziona.

Il vantaggio della revisione del codice al momento del check-in (buddy check) è un riscontro immediato, prima che siano state completate grandi parti di codice.

Lo svantaggio della revisione del codice al momento del check in è che può scoraggiare le persone dal fare il check-in fino a quando sono stati completati lunghi periodi di codice. Se ciò accade, nega completamente il vantaggio.

Ciò che rende questo più difficile è che non tutti gli sviluppatori sono uguali. Le soluzioni semplici non funzionano per tutti i programmatori . Le soluzioni semplici sono:

• Programmazione di coppie obbligate, che consente di effettuare frequenti controlli in quanto l'amico è proprio accanto a te. Questo ignora che la programmazione della coppia non funziona sempre per tutti. Fatto correttamente, la programmazione della coppia può anche essere davvero stancante, quindi non è necessariamente qualcosa da fare tutto il giorno.

• Filiali sviluppatore, il codice viene solo controllato e verificato nel ramo principale al termine. Alcuni sviluppatori sono inclini a lavorare in segreto e dopo una settimana escogitano un codice che può o meno passare la revisione a causa di problemi fondamentali che potrebbero essere stati individuati in precedenza.

• Verifica ogni registrazione, che garantisce recensioni frequenti. Alcuni sviluppatori sono smemorati e si affidano a check-in molto frequenti, il che significa che gli altri devono fare revisioni del codice ogni 15 minuti.

• Esamina in un momento non specificato dopo il check-in. Le recensioni verranno espulse ulteriormente quando si verificherà un crollo delle scadenze. Il codice che dipende dal codice già impegnato ma non ancora recensito sarà impegnato. Recensioni segnalerà i problemi e i problemi verranno inseriti nel backlog per essere corretti "successivamente". Ok, ho mentito: questa non è una soluzione semplice, non è affatto una soluzione. Revisiona in un momento specifico dopo il check-in, ma è meno semplice perché devi decidere qual è il tempo specificato

In pratica, rendi questo lavoro rendendolo ancora più semplice e più complesso allo stesso tempo. Stabilisci semplici linee guida e consenti a ogni team di sviluppo di capire come un team ciò che devono fare per seguire queste linee guida. Un esempio di tali linee guida è:

• Il lavoro è suddiviso in attività che dovrebbero richiedere meno di un giorno.
• Un'attività non è terminata se il codice (se presente) non è stato archiviato.
• Un'attività non è terminata se il codice (se presente) non è stato revisionato.

Sono possibili molte forme alternative di tali linee guida. Concentrati su ciò che desideri realmente (codice sottoposto a revisione paritaria, progressi del lavoro osservabili, responsabilità) e lascia che sia il team a capire come possono darti quello che vogliono.

in realtà facciamo un ibrido su LedgerSMB. I committer commettono le modifiche che vengono riesaminate dopo. I non committer presentano modifiche ai committer che devono essere esaminati in precedenza. Questo tende a significare due livelli di revisione. Per prima cosa ottieni un mentore da rivedere e da mentore. Quindi quel mentore ottiene il codice rivisto una seconda volta dopo che lui o lei è stato firmato e il feedback circola. I nuovi committer di solito passano molto tempo a rivedere i commit di altre persone all'inizio.

Funziona piuttosto bene. La cosa però è una recensione dopo di solito è più superficiale di una recensione prima, quindi vuoi assicurarti che la recensione dopo è riservata a coloro che si sono dimostrati. Ma se hai una revisione a due livelli per le persone nuove vuol dire che i problemi hanno più probabilità di essere scoperti e di avere discussioni.

Impegnarsi dove? C'è una filiale che ho creato per fare un po 'di lavoro. Mi impegno in quel ramo ogni volta che ne ho voglia. Non sono affari di nessuno. Quindi a un certo punto quel ramo è integrato in un ramo di sviluppo. E una via di mezzo è una revisione del codice.

Il revisore recensisce ovviamente dopo mi sono impegnato nel mio ramo. Non è seduto alla mia scrivania, quindi non può riesaminarlo prima di impegnarmi nel ramo mio . E rivede prima l'unione e si impegna nel ramo di sviluppo.

Semplicemente non fare recensioni sul codice. O credi che i tuoi sviluppatori siano in grado di scrivere un buon codice, o dovresti sbarazzartene. Errori nella logica dovrebbero essere catturati dai test automatici. Gli errori devono essere catturati dallo stile e da strumenti di analisi statica.

L'essere umani coinvolti in processi automatici dovrebbe essere solo uno spreco.