Come dovrebbero essere eseguite le revisioni del codice? [duplicare]

• Linee guida per la codifica : in primo luogo vorrei disporre di linee guida e strumenti di codifica formali per un controllo iniziale (estrai le discussioni basate principalmente sulle preferenze personali)

• Revisore qualificato : non penso che il gestore sia qualificato per eseguire la revisione del codice per impostazione predefinita. Dovrebbe essere qualcuno che ha familiarità con la base di codice e ha anche una solida conoscenza della codifica.

• Le discussioni sono di per sé un argomento : se hai discussioni sui nomi di questo significa che il nome è sbagliato, non dovrebbe lasciare spazio per discussioni. Accetta un nome.

• Apri comunicazione : questo è il punto più difficile per la cultura e il modo in cui le persone comunicano. La revisione del codice dovrebbe essere la gente insieme cercando di migliorare la qualità del codice. Dovrebbero essere discussioni aperte con argomenti solidi e solidi argomenti contrari e un accordo finale. Il recensore del codice deve essere in grado di indicare chiaramente i suoi punti, il revisionato deve essere in grado di difendere chiaramente le sue scelte ed entrambi devono essere disposti a essere smentiti. Se non riesci a farlo funziona lascia che qualcun altro faccia la revisione o pensa a ac ode-walkthrough anche se la cultura è sbagliata, una procedura dettagliata potrebbe persino essere peggio.

• Suggerimento o decisione : se la comunicazione è un po 'approssimativa, potrebbe essere un'opzione. Accetto formalmente che il revisore possa dare suggerimenti, ma la decisione è presa in considerazione. Questo imposta la scena e fornisce indicazioni su cosa aspettarsi.

Spero che questo aiuti.

Ecco alcune linee guida che sembrano funzionare bene.

• Verifica del codice ogni check-in immediatamente prima o dopo il commit. Uno dei grandi vantaggi della revisione del codice è che cattura rapidamente alcuni tipi di bug, prima che i bug colpiscano le persone a valle. Inoltre, più fresco è il codice nella tua mente, più velocemente sarai in grado di risolvere i problemi che vengono sollevati.

• Concentrati sul codice, non sulle persone. Questo può essere difficile, ma tutti dovrebbero ricordare che l'obiettivo della revisione del codice è migliorare la qualità del codice, non mostrare quanto sei migliore dei tuoi colleghi. Come autore, riconosci prima che la revisione accada che il tuo codice probabilmente ha bug e / o può essere migliorato. Questo non ti rende un cattivo programmatore.

• Mantiene le recensioni piccole (200-400 LOC). Più di questo, e l'efficacia del revisore è notevolmente diminuita.

• Aspettatevi di rivedere 300-500 LOC / ora. Se segui questa linea guida e quella precedente, le revisioni del codice dovrebbero durare non più di 90 minuti.

• Mantieni i manager fuori dalle revisioni del codice. A meno che tu non stia lavorando in un'azienda in cui il gestore sta scrivendo il codice ogni giorno (startup, in generale), il manager non dovrebbe partecipare. Avere il manager partecipante crea una brutta dinamica sociale, dove i revisori cercano di impressionare il capo e l'autore lavora troppo duramente per difendere il proprio codice.

• Utilizza uno strumento. Le revisioni di codice basate sugli strumenti sono migliori delle riunioni in molti modi, ma la chiave (per me) tra queste è che le revisioni di codice basate sugli strumenti non richiedono di bloccare il tempo su un calendario. La mia esperienza è che non appena il tempo viene bloccato, viene utilizzato. Se la riunione è programmata per un'ora, ci vorrà un'ora (o più) indipendentemente dal fatto che gli obiettivi dell'incontro siano stati raggiunti nei primi 10 minuti.

Alcuni di questi suggerimenti provengono da un white paper pubblicato da SmartBear (il mio datore di lavoro) su 11 Best Practices per la revisione del codice peer .

Mi piace la forma della revisione del codice praticata nella maggior parte dei progetti open source, che è qualcosa come "almeno un'altra persona guarda ogni patch prima di atterrare e l'autore della patch deve affrontare i miglioramenti suggeriti." Potrebbe chiamarlo "revisione delle patch".

Kathryn Rotondo ha un bel post sul blog qui: link

Considero la revisione delle patch principalmente un meccanismo di comunicazione. Se non lo fai, le persone tendono a inventare i propri stili di codifica, non sanno come codice non hanno scritto i lavori, non sanno del codice esistente che potrebbe essere riutilizzato, ecc. Anche l'architettura del progetto generale tende a essere più incoerente senza revisione delle patch perché le persone non devono sincronizzarsi con gli altri.

Potresti pensare alla revisione delle patch come "programmazione asincrona delle coppie", forse. (Una somiglianza con la programmazione delle coppie è che aiuta a scoraggiare la "proprietà del codice" da parte di una singola persona).

In secondo luogo, la revisione delle patch tende a catturare un discreto numero di bug stupidi e puoi applicare regole come "test delle unità di scrittura".

Un elemento importante nella revisione delle patch è che le patch devono essere riviste. Spesso gli sviluppatori trattano il commit del controllo di revisione come una specie di meccanismo di snapshot arbitrario, come premere save in un editor o spingere il work-in-progress al server in modo che possano raggiungerlo da un'altra posizione. Questo tipo di patch non è revisionabile. Per essere rivista, una patch deve essere un singolo cambiamento coerente, ben spiegato, senza "rumore" in esso. Anche il codice stesso deve essere comprensibile, ovviamente!

Una delle grandi cose che la gente ama di git è che ti permette di fare l'approccio snapshot / save mentre lavori, e poi tornare indietro e rebase, unire e dividere i commit in modo che siano una storia leggibile che abbia senso. Quindi puoi inviare le patch per la revisione.

Se esegui la revisione delle patch per un po 'di tempo, costringe le persone a scrivere codice tale che possa essere letto da qualcun altro, il che è enorme. Alcuni sviluppatori non sono in grado di farlo congenitamente e probabilmente non hanno bisogno di far parte del tuo team.

I revisori di patch dovrebbero sempre rifiutarsi di approvare una patch che non ha senso per loro o che non è leggibile o manca un buon messaggio di commit. L'idea è che i log di controllo di revisione e le patch dovrebbero raccontare una storia piacevole e coerente che ha senso .

Le recensioni dei codici possono essere più o meno formali. Una revisione formale del codice richiede una notevole quantità di tempo, poiché significa analizzare a fondo ogni singola riga di codice nel campione specificato. Aspettatevi una velocità di ca. 200 NLOC (linee di codice noncommentate) all'ora . Qualunque cosa sia più veloce significa che non si è veramente capito e analizzato il codice.

In un mio vecchio posto di lavoro, abbiamo fatto revisioni formali del codice in gruppi di 3-4 (più l'autore, se (s) era disponibile). C'era un leader / moderatore. Per risparmiare tempo, il leader ha inviato l'esempio di codice in questione con qualche giorno di anticipo e tutti dovevano passare attraverso il codice e prendere nota di eventuali problemi separatamente. I problemi sono stati classificati come principali / minori / cosmetici . Quindi i fogli Excel con i risultati sono stati inviati al leader, che li ha uniti in un unico elenco. Durante la riunione di revisione, il gruppo ha esaminato l'elenco delle questioni principali e secondarie (i problemi relativi ai cosmetici sono risultati banali e non degni di discussione nella pratica). Ogni oggetto è stato accettato se fosse ovvio, o discusso ulteriormente se vi fosse disaccordo su di esso. Quindi l'autore (se disponibile) ha fatto gli aggiornamenti in base all'elenco finale.

Ogni riunione ha richiesto circa 1,5-2 ore e ha esaminato in media 300 NLOC (si noti che questo era spesso un elemento legacy, codice spaghetti - la revisione del nuovo codice era molto più veloce). Questo ha richiesto parecchio tempo, ma è stato efficace in quanto abbiamo riscontrato numerosi bug importanti anche nel vecchio codice, e le discussioni generate dai dilemmi di progettazione / implementazione sono state una grande esperienza di apprendimento. Questo è in effetti il più grande vantaggio delle recensioni formali di codice di gruppo: aiutano il team a ottenere e mantenere una comprensione pratica comune sullo stile e gli idiomi di codifica accettati e a migliorare le abilità e le pratiche dei giovani / nuovi membri del team rapidamente.

[Aggiornamento] Si trattava di un progetto legacy con gravi problemi di qualità e consegna, quindi c'era un strong impulso da parte della direzione per mettere ordine nel caos. Potremmo esaminare la maggior parte delle nuove modifiche al codice e una piccola parte della base di codice esistente durante l'anno che ho trascorso lì. [/ Update]

In un progetto greenfield in un altro posto di lavoro, abbiamo eseguito revisioni meno formali del codice: per ogni modifica del codice non banale, un altro membro del team è stato nominato revisore. Ha fatto la recensione da solo, ha risolto problemi banali e ha inviato l'elenco dei problemi non banali all'autore da correggere. Questo mirava a filtrare bug o problemi nel modo più efficace possibile. Tuttavia, non abbiamo raccolto statistiche sulla sua efficacia.

Penso che il più efficace possa essere una combinazione dei due. Con le recensioni formali del codice, è molto costoso, se possibile, coprire completamente il codice in qualsiasi progetto reale e non banale. OTOH la maggior parte del nuovo codice dovrebbe essere già scritta abbastanza bene (almeno dopo un periodo iniziale, mentre i membri del team comprendono e concordano le comuni convenzioni sulla codifica e le migliori pratiche). Quindi potrebbe valere la pena di avere un singolo compagno di squadra sfogliando ogni cambio di codice e organizzare una revisione di gruppo solo per modifiche di codice più complesse / critiche / interessanti.

Le revisioni del codice si concentrano spesso su minuzie a basso impatto, cercano di concentrarti sulle cose grandi

Essendo stata una fine sbagliata troppe recensioni di codici estetici, generalmente cerco solo difetti logici o pratiche non sicure / non scalabili quando entro in una recensione. Mi siedo al 90% dell'incontro in cui le persone discutono sui nomi var che nessuno guarderà mai più, di solito nessuno nota le cose strutturali e poi lo porto alla fine se nessuno lo menziona.

Per il mio occhio il 90% di quel tempo è sprecato perché se l'originale dev mantiene il codice conosce la sua denominazione, e se un diverso dev mantiene il codice che può fare un rinominare globale, è davvero infastidito dalla denominazione. Il tuo chilometraggio può variare.

Scala e portata. Nella mia ultima organizzazione abbiamo avuto un processo di revisione formale. Quando avevi un cambiamento relativamente grande per il trimestre (> 500 ore), dovevamo convocare un team di architettura per la revisione. Dovrei passare attraverso tutte le modifiche, riga per riga e domande sul campo. Con cambiamenti inferiori a quello sarei io, uno a uno, con il progettista, assicurandomi che le modifiche fossero specifiche. I progetti più piccoli erano molto meno formali in modo da non sprecare il tempo di tutti.

Un'altra competenza chiave di questo processo è imparare come spiegare argomenti / processi complessi ai non programmatori. Einstein disse: "Non capisci davvero qualcosa se non puoi spiegarlo a tua nonna". Penso che questo si applichi anche al team leader, ai direttori e ai VP;) Impari l'arte dell'analogia.

Per rendere il processo più piacevole e meno stressante? Non essere conflittuale. Stai lontano dalla risposta "Avrei fatto diversamente". A volte il modo in cui qualcun altro ha "spellato il gatto" è abbastanza buono per quello che stai facendo e ti basta andare avanti.

In conclusione, avevamo diversi tipi / livelli di riunioni in base alle dimensioni e alla portata del progetto.

Utilizziamo una serie di hook di commit, singoli repository e filiali per garantire che le revisioni inizino dopo ogni modifica meno che banale, anziché rivedere un'intera revisione del sottosistema durante una finestra di fusione di due settimane.

A nessuno piace un incontro di due ore in cui i loro pari scelgono migliaia di righe di codice. In un mondo ideale, non riesci mai a rivedere pezzi di queste dimensioni. A volte, sì, è inevitabile, ma il più delle volte le recensioni possono essere fatte in piccoli morsi. L'ultima cosa che vuoi viene detta "ri-factor tutto ciò" pochi giorni prima di una scadenza. Questi tipi di problemi possono essere scoperti molto prima.

Abbiamo poche persone che hanno le chiavi dei repository centrali che alla fine si trasformano in build di produzione. Le persone che lavorano su vari progetti inviano richieste di pull (molto simili a come lo fa il kernel di Linux) chiedendo che il nuovo codice venga inserito nel repository benedetto. Inoltre, ogni push su un singolo repository dà il via ad alcuni hook che:

• Eventualmente aggiornare un bug nel database dei bug
• Invia e-mail a coloro che devono essere informati
• Lancia diversi lint

In alcuni casi, potrebbe anche essere attivata una build.

Ciò consente a tutti gli altri membri dello stesso team (inclusi i responsabili) di prendere immediatamente visione dei possibili problemi e rivederli è semplice come rispondere alla posta elettronica che viene generata.

La revisione finale arriva alla richiesta di pull, ma a quel punto la maggior parte del codice è già stata esaminata. I commenti alla revisione finale sono di solito qualcosa del tipo:

• "Ho bisogno che sia documentato prima di poterlo ritirare, o ci dimenticheremo di farlo in seguito"
• "Non è più necessario aggirare [il problema del compilatore], siamo passati a [nuova versione]"
• "Joe ha implementato alcune funzioni statiche che quasi duplicano le tue in [area], forse voi due potete collaborare e creare una singola libreria condivisa"

Ma raramente i problemi a questo punto sono legati al codice che ogni persona ha scritto, sono soprattutto problemi nel fondere tutto insieme.

Molto dipenderà dal codice necessario, ma mentre le recensioni porteranno a un codice più perfetto, esauriscono le risorse e possono ritardare notevolmente la procedura di sviluppo.

Rivedendo quando lo sviluppatore ha speso le sue cellule cerebrali a trovare la migliore soluzione per il problema, ha lavorato su tutti i problemi per farlo funzionare a specifiche pienamente testate, le possibilità di cambiamento sono relativamente piccole e qualsiasi cambiamento su larga scala indicherebbe certamente che lo sviluppatore ha perso troppo tempo sul percorso sbagliato troppo a lungo e che questo avrebbe dovuto essere affrontato prima.

Quello che Joel suggerisce è "test di usabilità del corridoio" in cui mostri il tuo codice a un peer che potrebbe doverlo usare e hanno un aspetto per vedere se riescono a comprenderlo molto rapidamente.

Quindi è necessario ponderare il bilancio, in particolare perché è probabile che l'azienda di successo sia quella che rilascia presto. Esaminare il percorso relativamente presto, i test di usabilità dei corridoi? sì. Modifiche minori in seguito? sì. Una procedura dettagliata che richiede agli sviluppatori "senior" di esaminare ogni linea? No, a meno che il tuo programmatore non sia un giovane appassionato di imparare.