Il mio approccio personale alle code review

Spunti di riflessione su “cosa fare realmente” mentre si fa una revisione del codice.

Molto tempo fa, in una galassia molto, molto lontana… il codice non conteneva bug e tutti avevano competenze e facoltà di aggiungere nuove funzionalità senza introdurre bug nel software.

Ma siamo ancora sulla Terra, nel XXI secolo… e questo lontano passato deve ancora arrivare! Nel frattempo (armati di pazienza e popcorn, aspettando con ansia che questo splendente scenario si dispieghi davanti a noi) possiamo migliorare la nostra situazione aggiungendo nella nostra pipeline di sviluppo del software un nuovo step: la revisione del codice!

Lo so, lo so, un’altra cosa da aggiungere a una lista di cose da fare sempre più grande e senza fine. Ne abbiamo davvero bisogno? Questi sono alcuni dei possibili benefici che mi vengono in mente:

• i membri del team sono più allineati con ciò che sta accadendo all’interno della base di codice -> meno overhead per lavorare su una parte di codice che “non ho mai toccato”
• più occhi e teste per individuare bug o comportamenti indesiderati il prima possibile -> meno correzioni dopo il deployment (che sono più costose!)
• un momento dedicato alla condivisione e diffusione della conoscenza tra gli sviluppatori -> coltivare la crescita di ogni membro del team porta ad un team più efficiente

Ne vale la pena? Decidete voi! Io sono qui solo per offrire alcuni spunti e idee su come eseguire una revisione del codice efficace… spero li troverete utili, in caso contrario indirizzate pure le vostre lamentele a /dev/null 🙂

A prima vista, una code review potrebbe apparire come un passo semplice e lineare: basta guardare il codice e poi premere quel pulsante con il pollice in su o in giù. Tuttavia, se siamo disposti a guardare più da vicino, allora le cose diventano più articolate:

• in pratica, cosa significa “guardare solo il codice”? Ci sono cose specifiche a cui fare attenzione? È sufficiente “solo guardare”?
• come dovrebbero interagire tra loro il revisore e chi presenta il codice? Questo porta sul tavolo le abilità di comunicazione dei membri del team. Prima tra tutte la capacità di essere chiari e concisi (ad esempio, come scrivere la descrizione della pull request per il revisore, come spiegare al presentatore un problema che abbiamo trovato nel codice). Ma entrano in gioco anche altre soft-skills, come prendere in considerazione l’aspetto psicologico di “dare un qualche tipo di feedback ad un altro essere umano” (ad esempio, ricordare di essere sempre gentili, non incolpare una persona specifica ma assumersi la responsabilità come squadra, non indirizzare mai la conversazione a livello personale, riconoscere che il codice è solo uno strumento per risolvere effettivamente un qualche tipo di problema della vita reale e che un “attacco” ad un pezzo di codice non è un attacco diretto a chi lo ha scritto)
• quanto tempo investire in una revisione? O ancora più fondamentale: quando è veramente utile eseguire una revisione? Una patch che corregge alcuni refusi dovrebbe richiedere lo stesso sforzo di una patch che cambia le API di un programma?
• come gestire effettivamente il processo di revisione? Il pulsante di merge viene premuto da chi esegue la revisione, da chi ha inviato la patch o solo da un manutentore del repository? Come fa qualcuno a scoprire se c’è una revisione in sospeso? Per quanto tempo una richiesta di merge dovrebbe essere tenuta aperta per la revisione prima di “cambiare revisore” oppure “la si mergia ugualmente altrimenti blocchiamo altri sviluppatori che hanno bisogno di quella patch”?

Come potete immaginare, ognuno dei punti precedenti è un argomento che potrebbe  richiedere un blog-post a sé stante per essere approfondito a dovere. Ma allora, cos’è questa sequenza di parole che state leggendo in questo momento? Beh, è uno di questi ipotetici blog-post! In particolare, è quello che vuole far luce su come poter depennare dalla lista “il codice è stato guardato”.

Ma prima di scendere nei dettagli, vorrei iniziare offrendovi un piccolo mantra che dovrebbe tenervi compagnia ogni volta che si entra in modalità “revisione di codice” (o per lo meno, ha aiutato me a non allontanarmi dalla retta via a causa del mio ego e dei miei gusti personali):

Una code review è un processo che favorisce l'evoluzione di una codebase, e non un ostacolo ai cambiamenti:
- non puntare ad ottenere una "patch perfetta" durante la revisione... valutare sempre il fatto che una patch sta intrinsecamente migliorando una codebase in qualche modo (ad esempio, corregge un bug, aggiunge una feature): la codebase senza quella patch è molto probabilmente peggiore. Bloccare una patch perché il codice non è perfetto non è utile
- va bene se la patch non è quello che avreste scritto voi... aspettarsi che la soluzione implementata in una patch corrisponda esattamente alle preferenze del revisore è sia impossibile (ogni sviluppatore è diverso con idee e gusti diversi) che dannoso (avere punti di vista diversi sullo stesso codice aiuta a individuare bug e comportamenti indesiderati)

E ora: entriamo nel dettaglio! Personalmente tendo a considerare la “revisione del codice” non tanto come un’attività legata al 100% al codice sorgente stesso, quanto una attività composta principalmente da 3 macro fasi di cui solo una è incentrata sul codice:

• controllare che la patch funzioni correttamente
• controllare il codice
• imparare cose nuove

Controllare che la patch funzioni correttamente

Controllare che la patch funzioni correttamente? Facile! `git pull`, `make`, avviare il programma e premere quei due nuovi bottoni e abbiamo finito!

Certo, è un sogno. Controllare che la patch funzioni correttamente è “leggermente” più complicato:

• validare che la patch faccia ciò che ci si aspetta
• controllare che la patch non introduca nuovi bug
• controllare che la patch non comprometta le funzionalità preesistenti

Ma: ehi! Perché dovremmo preoccuparci? Non è più efficiente, in termini di costi, confidare nel fatto che chi ha presentato la patch ci abbia “giocato abbastanza” o semplicemente affidarsi ai tester o alla fase di controllo della qualità? Sì e no. Certo, è difficile e richiede tempo impostare un “ambiente di sviluppo pulito” ogni volta che dobbiamo testare una nuova patch. Ed è anche una seccatura studiare e capire le specifiche o lo use case della issue originale quando chi ha presentato la patch si è già impegnato a farlo. Ma se questo costo è ripagato dal fatto che i potenziali problemi vengono scoperti prima e sono molto più facili da risolvere, dipende davvero molto sia dal tipo di progetto a cui state lavorando, sia da come avete impostato il vostro processo di sviluppo: se non ci sono altri momenti migliori per eseguire questo controllo di qualità, allora la revisione è probabilmente il posto migliore dove inserire questo step. Ho imparato a mie spese che un po’ di QA può individuare molti problemi perché:

• siamo tutti molto occupati… non è così raro che siamo così sicuri del codice che abbiamo scritto (ad esempio, perché pensiamo che la patch sia banale) che ci sembra che nulla possa essere andato storto e quindi decidiamo di guadagnare un po’ di tempo evitando di fare “la prova dei 5 minuti”. E magari ci è sfuggito completamente il fatto che ci siamo dimenticati di collegare effettivamente tutti i pezzi del “backend” al “frontend”!
• non tutte le specifiche/issues sono scritte allo stesso modo, e il cervello (almeno il mio) tende a leggere troppo tra le righe e finisce per inventarsi dei vincoli nuovi (e indesiderati) o semplicemente capisce una cosa diversa. Un secondo cervello per controllare che la richiesta iniziale sia stata compresa e implementata correttamente è sempre apprezzato!
• i tester sono una risorsa limitata (talmente limitata che il più delle volte il loro numero oscilla intorno allo 0) e la fase di controllo qualità è così lontana nel futuro che “questa feature attuale” sarà probabilmente dimenticata o sarà sommersa da centinaia di altre patch da rendere sempre più difficile testarla e validarla in isolamento.
• I test di unità e integrazione a volte sono un lusso. Lottare contro un progetto che non ha una suite di test è una cosa buona e giusta: riempire la voragine lasciata da tutti i “test-mai-scrittii” è un impegno a lungo termine che dovrebbe essere programmato ed attuato, ma nel frattempo lo spettacolo deve andare avanti, ed eseguire un po’ di QA manuale per assicurarsi che la patch sia a posto potrebbe aiutare ad aumentare la qualità complessiva del progetto.

Chiaramente, sto scrivendo ben consapevole che la maggior parte della mia esperienza riguarda lo sviluppo di applicazioni desktop, e quindi raramente ho dovuto combattere contro HW molto specifici, sistemi embedded, complessi sistemi di distribuzione cloud, ecc.. quindi sentitevi liberi di saltare questo capitolo se pensate che non sia applicabile al vostro caso specifico. Non mi offenderò, promessa di lupetto!

Validare il nuovo comportamento

Prima ancora di iniziare a leggere il codice sorgente modificato attraverso la patch, è utile vedere se la patch fa quello che dovrebbe fare. Ovvero: leggere la issue che ha originato la patch e controllare se la patch corregge veramente il bug o implementa correttamente la feature richiesta.

Spesso trascurato, questo semplice passo può individuare un problema nella patch molto presto (e quindi fa risparmiare il tempo di guardare effettivamente il codice). Questa verifica dovrebbe essere facile da eseguire, ma a volte non lo è: fare il pull delle modifiche e ricompilare il progetto può essere una seccatura, tuttavia l’ostacolo più grande è avere a disposizione immediata i dati di input necessari per testare la patch o conoscere i passi esatti necessari per innescare il comportamento desiderato:

• controllare se ci sono esempi o file/script allegati al problema
• chiedere gentilmente allo sviluppatore che l’ha prodotta di fornire qualche input o qualche suggerimento su “come testare la patch”

Giocare con la nuova feature

Tutti sanno che ogni patch non triviale introduce nuovi bug. O, almeno, ha una probabilità molto alta di farlo. Ma stiamo facendo la revisione del codice: uno dei suoi obiettivi è cercare di diminuire il numero di nuovi bug introdotti nella codebase!

Ma come possiamo individuare nuovi bug? Se la patch introduce nuovi test, allora possiamo avere una certa speranza che alcune cose andranno bene. Ma cosa succede se non ci sono test o se i test non sono esaustivi? Perché ammettiamolo: i test sono come cittadini di terza classe in una codebase. Inoltre, a volte non è nemmeno possibile (o non è conveniente) testare tutto ciò che una patch introduce. Sì, sì, TDD…ma come la mettiamo con la complessità di creare un test di integrazione robusto e completo? Avete mai lottato per far avviare e far dialogare automaticamente 5 diversi eseguibili solo per riuscire a testare quella piccola feature che non è quasi mai utilizzata, e l’incubo di riuscire a terminarli in modo coerente all’interno della CI? sia OSX, Linux e Windows? Testare l’UI… devo dire altro? siamo sicuri che quel pulsante “OK” sia disabilitato se l’utente tiene premuto il tasto “Ctrl”? O devo citare quel bug infernale che salta fuori solo quando vuole lui a causa di un qualche problema di sincronizzazione di threads?… E quasi dimenticavo: avete visto quella nuova card aperta dal PM? Deve essere implementata al più presto, e preferibilmente dovrebbe essere completata per ieri!

Ma tralasciamo i piccoli incubi di vita vissuta e torniamo a noi: individuare nuovi bug. “Giocare” con il programma patchato è la strada da seguire. C’è però un problema: oltre ad essere un approccio che richiede potenzialmente tanto tempo, “giocare efficacemente” con un programma non è facile, tanto più se il revisore è anche uno sviluppatore dello stesso progetto. Infatti, avendo una conoscenza intima della struttura interna del programma, è improbabile che uno sviluppatore lo stressi come solo dei veri utenti finali riuscirebbero (e a proposito, come ha fatto quell’utente ad aprire 2 dialog modali contemporaneamente? È veramente possibile?). Ma questo può anche essere un vantaggio: se il revisore è uno sviluppatore, allora è probabile che conosca le aree del programma che sono più fragili e quindi hanno più probabilità di fallire o che (sotto il cofano) sono inaspettatamente collegate ai file interessati dalla patch. E quelle sono le aree che dovrebbero ricevere una certa attenzione quando si gioca. Inoltre, come suggerimento gratuito, incoraggio tutti coloro che hanno a che fare con programmi il cui input contiene stringhe o percorsi a file, a cambiare il locale e la codifica della propria macchina o semplicemente usare strani caratteri unicode… molte volte questa è la causa di problemi fastidiosi!

Il resto del programma è rimasto inalterato

E questa è la parte difficile: “controllare che la patch non rompa altre cose”.

Ci si augura che ci siano un po’ di test nel progetto e una CI funzionante che li compili e li esegua. Questo dà una certa dose di tranquillità sul fatto che la patch non rompa il resto del progetto. E se il progetto non ha una suite di test esaustiva? Allora siamo nei guai. Posso solo suggerire di avere all’interno del progetto una “checklist” condivisa dove ogni voce è una  operazione end-to-end sufficientemente importante e abbastanza comune non coperta dai test, con una breve ma dettagliata descrizione dei passi da eseguire e con i dati di input necessari per riprodurre effettivamente quella operazione. Aka: ogni revisore dovrebbe sforzarsi di compilare una suite di test in linguaggio naturale e facilmente riproducibile manualmente. 

Va da sé che quest’ultimo approccio richiede molto tempo, e dovrebbe probabilmente essere utilizzato solo per patch molto grandi o delicate (o appena prima di un rilascio :P).

Controllare il codice

Sono fortemente convinto che sia possibile controllare il codice sorgente a diversi livelli di qualità: Il problema è trovare il giusto equilibrio tra la precisione e il tempo speso. Inoltre, alcuni controlli sono più facili da fare a seconda della familiarità e dell’anzianità che lo sviluppatore ha con il codice.

Questi livelli, in ordine di complessità, hanno obiettivi diversi:

• estetica e stile
• leggibilità della documentazione
• complessità e copertura dei test
• gestione della complessità degli algoritmi utilizzati
• contenimento della complessità della soluzione adottata
• uniformità con l’architettura del sistema

Cosmetica e stile

Questo è l’obiettivo più superficiale, più veloce e più facile per il revisore. La sua utilità è quella di individuare delle sviste o delle piccole inconsistenze banali che, se accumulate, andrebbero ad aumentare il debito tecnico sotto forma di codice non pulito.

Cose da cercare:

• refusi. Sì, un refuso in un commento o nel nome di una variabile non è un grosso problema, ma correggendoli al più presto si manterrà la “sensazione” di un codice curato e mantenuto, una sensazione che sperabilmente spingerà i futuri sviluppatori a continuare a mantenere le cose in ordine.
• nomi migliori per variabili o funzioni. Sì, ci sono commenti e docstring, ma tendono ad andare fuori sincrono. Suggerire un nome più carino o più adatto per una variabile non costa nulla e crea un codice più manutenibile
• stile. Si spera che ci sia una direttiva per lo stile del codice da mantenere nel progetto. Se non c’è alcuna automazione per far rispettare questo stile, allora il dovere ingrato ricade sul revisore. Ricordate: evidenziare il problema dello stile non è “essere schizzinosi”, è solo che tutto si accumula, e anche le cose più piccole possono fare la differenza quando vengono sommate insieme

Una nota importante: ci sono linters, formattatori, correttori di sintassi per quasi tutti i linguaggi di programmazione… usateli! Ritagliatevi un po’ di tempo per impararli e usateli nel vostro progetto: non lasciate che un umano faccia ciò che un bot potrebbe fare con più facilità e con più precisione!

Documentazione leggibile

La documentazione… una controversia senza fine: ogni programmatore avrà la propria idea su cosa sia una “buona documentazione”. Tuttavia, qui non si tratta di “scrivere” o “decidere cosa documentare e come farlo”, ma di “revisionare”, ovvero “leggere e capire”.

Se il progetto ha alcune regole sulla documentazione (ad esempio, usa qualche script per generare documentazione navigabile), la revisione dovrebbe controllare che queste regole siano applicate correttamente.

Più in generale, poiché la documentazione è parte del progetto ed è utile solo se è aggiornata, facile da trovare e facile da capire, i controlli da eseguire durante una revisione sono:

• i documenti del progetto sono stati aggiornati. E’ facile dimenticarsene ma un progetto non è composto solo da codice sorgente, contiene anche documenti txt/md (ad esempio, un documento sull’architettura del sistema, un documento che elenca tutte le feature) e pagine wiki. E nel caso stiate pensando “è già obsoleto” o “sono inutili perché nessuno li legge”, ripensate a quando avete iniziato a lavorare al progetto: sarebbe stato utile avere una documentazione aggiornata che vi prendesse per mano per farvi una panoramica del progetto?
• la documentazione “pubblica” è chiara e comprensibile. Come regola generale, dovrebbe andare dritta al punto, essere scritta correttamente, senza errori di battitura e dovrebbe trasmettere tutte le informazioni necessarie. Se la lettura di una docstring ci lascia con più domande che risposte, probabilmente significa che la documentazione dovrebbe essere riformulata. Se una docstring ci dice come la funzione dovrebbe essere usata e in quale contesto, allora forse la funzione non dovrebbe essere pubblica o la documentazione dovrebbe essere più “permissiva” (è l’utente della funzione a decidere quando usarla, e non il contrario)
• la documentazione “privata” è utile. Tutti i commenti all’interno delle funzioni dovrebbero aiutare il revisore nella lettura del codice. Se affermano cose ovvie o se non sono chiari (cioè non hanno senso per il lettore) o non descrivono ciò che accade nel codice che segue, allora dovrebbero essere evidenziati durante la revisione. Suggerirei anche di segnalare commenti innocui ma “inutili”: capita, quando si scrive il codice, di pensare che alcuni commenti siano necessari per chiarire eventuali malintesi. Tuttavia, quella “necessità” potrebbe essere davvero circostanziale, e due occhi incontaminati (quelli del revisore) possono aiutare a decidere se il commento è effettivamente utile. Inoltre, se aggiungiamo un commento che spiega il “perché” e il “come” e la “storia” che ci ha portato a scrivere ogni singola linea di codice, ci ritroveremo con un poema che sarebbe quasi impossibile da tenere aggiornato, e perderebbe il suo reale valore poiché nessuno lo leggerà veramente (e ricordate, alcune cose è meglio scriverle nel commento del commit, dove possono essere recuperate quando si fa un po’ di archeologia!)
• i commenti limitrofi alla parte di codice modificata sono ancora validi. I commenti sono ignorati dal compilatore o dall’interprete, quindi tendono ad andare fuori sincrono. Tenete gli occhi aperti per i commenti che sono vicini alle righe cambiate dalla patch, ora potrebbero dire qualcosa di sbagliato perché la patch ha cambiato il codice a cui si riferivano.
• controllare che la documentazione “pubblica” sia allineata con il codice. Se la precondizione o la postcondizione di una funzione sono cambiate a causa della patch, date un’occhiata alla sua docstring e valutate se la documentazione è ancora valida o se deve essere aggiornata per riflettere il cambiamento nel codice.

Complessità e copertura dei test

Leggete i test! I test sono come una “documentazione dal vivo” per le feature del vostro progetto. Se sono troppo complessi da capire durante la revisione (quando il revisore dovrebbe avere il contesto per sapere cosa stanno cercando di testare) allora potrebbero diventare difficili da mantenere: se un test si rompe in futuro e nessuno capisce cosa sta testando, è probabile che il test venga rimosso invece di essere corretto. E un test rimosso perché non è comprensibile significa che tempi e sforzi impiegati dallo sviluppatore per scriverlo sono andati sprecati, e che ora c’è una funzionalità non testata.

Inoltre i test sono “il primo utente” del vostro sistema: se è troppo complesso provare la patch manualmente (ad esempio, perché per funzionare il sistema deve connettersi a n server remoti e a M diversi database), allora provare la patch leggendo, eseguendo e modificando leggermente i test è un approccio valido!

E come corollario al paragrafo precedente: se la patch manca di alcuni test, non abbiate timore e ditelo. Questo potrebbe stimolare un’interessante discussione su “è davvero questa la caratteristica che vogliamo testare” o “non sono riuscito a pensare a nessun test ragionevole da eseguire, ma il vostro suggerimento sembra facile da implementare e potrebbe aumentare la copertura del sistema”. E test più utili portano ad un progetto più robusto e sano!

Altre cose da considerare quando si esaminano i test:

• molti test non sono necessariamente una buona cosa: preferire la qualità alla quantità. Se ci sono 3 test che stressano la stessa cosa in un modo leggermente diverso senza aggiungere valore effettivo alla copertura, allora forse potrebbero essere rimossi (aka meno codice da mantenere) o essere leggermente modificati per stressare un diverso percorso del codice
• i test sono belli, ma se stanno testando un’API pubblica basandosi sui dettagli dell’implementazione interna, allora è molto probabile che diventino obsoleti molto rapidamente e che debbano essere modificati ogni volta che cambia il dettaglio specifico dell’implementazione. Individuare questi test durante la revisione è molto utile in quanto dovrebbe innescare una discussione su “qual è il modo corretto di testare qualcosa” e potrebbe portare a progettare una migliore API pubblica che sia più facile da testare o portare a dei test che siano più longevi (aka meno sforzo per mantenerli)

Gestione della complessità degli algoritmi utilizzati

La complessità all’interno di un progetto a volte è inevitabile. Il più delle volte però è evitabile. Ogni sviluppatore ha sentito quella scarica di dopamina nel momento in cui ha scritto quel pezzo di codice molto molto furbo. Ed è successo ad ogni sviluppatore di inciampare in quello stesso pezzo di codice alcuni mesi dopo e di pentirsi di averlo scritto:  quel “codice furbo” è diventato un “hack inutilmente complesso”. Ricordate: il codice viene letto molto più di quanto venga scritto. Il revisore è una delle prime persone a sperimentare effettivamente la leggibilità del codice: se il codice potrebbe essere scritto in modo più semplice o guadagnarci in comprensibilità con alcune piccole correzioni, allora è utile segnalarlo. I suggerimenti fatti da un revisore non sono un affronto allo sviluppatore che ha scritto la patch, sono solo tentativi di creare una codebase più agevole dove ogni sviluppatore (dal junior al senior) possa lavorare con il minor attrito possibile.

Alcuni suggerimenti qui sono:

• usare la libreria standard quando possibile. Meno codice da scrivere e da manutenere!
• utilizzare le funzioni già implementate nel progetto. Evitare di riscrivere la stessa funzione a meno che non ci sia una buona ragione per farlo. Succede per lo più perché lo sviluppatore ha dimenticato o non sapeva che la stessa funzione esisteva da qualche altra parte.
• le funzioni molto lunghe possono normalmente essere divise in funzioni più piccole su cui è più facile lavorare
• ottimizzare il codice per le prestazioni porta ad avere codice più complesso (ad esempio, necessità di gestire le cache, utilizzo di strane strutture dati che si adattano meglio ad una pagina di RAM). Se le prestazioni non sono un problema potrebbe essere utile valutare un cambiamento nell’algoritmo

Uniformità con l’architettura del sistema

Il punto più critico di una revisione, e probabilmente il più difficile da eseguire.

L’architettura del progetto dovrebbe essere preservata il più possibile. Ogni cambiamento nell’architettura dovrebbe essere discusso ampiamente da tutto il team. Se una patch è semanticamente corretta, ma rompe l’architettura del progetto, allora è dovere del revisore rilevarlo. Come risolvere il problema, tuttavia, è legato al caso specifico. Nella peggiore delle eventualità potrebbe portare a dover riscrivere la patch.

È molto difficile dare consigli su questo punto, poiché il concetto di “architettura del progetto” dipende molto dal progetto in esame. Suggerimenti generali potrebbero essere:

• la patch non dovrebbe creare nuove correlazioni indesiderate tra parti diverse e altrimenti indipendenti del sistema. Per esempio, abbiamo un oggetto ‘Car‘ e dobbiamo aggiungere il metodo ‘estimateTravelTime‘, e decidiamo di crearlo come ‘estimateTravelTime(Building *destination)‘ e di cambiare il costruttore di ‘Car()‘ in ‘Car(Building *starting_point)‘: se il nostro sistema è stato costruito con l’intenzione di tenere ‘Car‘ e ‘Building‘ separati, ora stiamo aggiungendo una dipendenza tra i due e stiamo rompendo un principio architetturale del sistema. Questo può portare ad un aumento della complessità nel sistema e nei test (perché ora per lavorare con ‘Car‘ dobbiamo anche usare e istanziare ‘Building‘)
• la patch non dovrebbe cambiare il principio alla base delle interfacce pubbliche. Ad esempio, abbiamo un oggetto controller la cui docstring dice che il suo compito principale è quello di eseguire controlli di consistenza su alcuni modelli, e abbiamo bisogno di aggiungere una funzionalità al sistema per esportare quei modelli come JSON. La nuova funzionalità viene implementata all’interno del controller perché gestisce già i modelli. Questa è una soluzione potenzialmente sbagliata: offre un precedente al fatto che il ruolo del controller all’interno del sistema sia cambiato in modo non documentato
• la patch non dovrebbe creare un “nuovo modo” di eseguire un task se quel task dovrebbe essere risolto da un sottosistema già esistente all’interno del progetto. Ad esempio, abbiamo un logger nel sistema, ma richiede un po’ di configurazione e setup prima di essere usato, e abbiamo solo bisogno di registrare su file ciò che sta accadendo all’interno di una specifica parte del sistema: decidiamo di aprire manualmente un file e scrivere manualmente al suo interno perché è più veloce e più semplice da implementare (o solo perché non sapevamo del logger). Questo è male: ora abbiamo all’interno del nostro sistema un componente con il compito specifico di loggare e un’altra parte del sistema che imita quel comportamento in modo completamente personalizzato. Questo comporterà dei problemi quando un altro sviluppatore si aspetterà di trovare dei messaggi di log all’interno del logger di default e non ce ne saranno perché quei messaggi stanno usando un altro logger personalizzato.

Imparare cose nuove

A questo punto, se avete davvero seguito tutti i punti precedenti, avrete imparato un sacco di cose:

• come la codebase sta evolvendo
• scoperto nuove parti del progetto su cui non avete mai avuto la possibilità di lavorare
• scoperto nuovi modi di risolvere un problema (aka quello implementato dallo sviluppatore che ha presentato la patch)
• come creare un’API più solida e testabile: tutti impariamo dai nostri errori, ma impariamo anche guardando gli errori degli altri (o almeno, riconoscere ciò che non abbiamo trovato comprensibile nella patch dovrebbe spingere noi, “i revisori”, a diventare degli sviluppatori migliori, perché la prossima volta non saremo noi i revisori, ma saremo quelli che hanno inviato la patch!)

E se questo non fosse abbastanza, la review è il luogo perfetto non solo per far sapere a chi ha inviato la patch cosa abbiamo trovato anomalo, ma anche per chiedere chiarimenti su un algoritmo utilizzato, l’architettura del sistema, l’uso della libreria standard e molto altro.

E non dimenticate: è molto apprezzabile il fatto di far notare le cose buone e dirle allo sviluppatore, o semplicemente ringraziarlo perché finalmente ha trovato il tempo e il modo di estirpare quel bug  che vi ha dato fastidio negli ultimi 3 mesi!

Chi è il primo revisore?

Il primo revisore dovrebbe essere colui che ha creato la patch! Prendetevi un momento per pensarci: come revisori preferireste controllare una patch pulita o una patch con residui di stampe di debug, codice commentato o parametri con nomi obsoleti? Scommetto che preferireste la prima. Quindi, come programmatori e giocatori di squadra, il vostro ultimo compito prima di presentare una patch è quello di pulire la patch stessa. Scrivere una patch è un processo iterativo che può durare diversi giorni (con molte interruzioni) e si articola in diverse fasi (come il debug, trovare casi limite, correggere e iterare). Tuttavia la “patch finale” riassume tutto ciò che è stato fatto da uno sviluppatore e “dimentica” i singoli passi intrapresi per raggiungere “la fine”. È probabile che “alla fine” lo sviluppatore abbia imparato molte cose sul codice, una conoscenza che non c’era quando è stata scritta la prima riga di codice: rileggere la patch come un’unica entità permette allo sviluppatore di individuare piccole cose che potrebbero produrre una patch addirittura migliore. Come ad esempio:

• eseguire qualche piccolo refactoring dell’architettura: individuare piccoli miglioramenti mentre si ha la diff completa sotto gli occhi è più facile che eseguire il refactoring in-itinere un commit alla volta (dove in ogni commit non era nemmeno chiaro lo “scopo/punto d’arrivo”)
• rimuovere il codice indesiderato: perché diciamolo, è capitato a tutti di dimenticare qualche stampa di debug
• scrivere una documentazione migliore: la prima docstring scritta nella patch può essere abbastanza criptica se letta dopo 3/4 giorni (l’idea che si voleva trasmettere presupponeva probabilmente qualche conoscenza tecnica che si pensava fosse un presupposto chiaro solo perché si stava lottando su un determinato pezzo di codice da diverse ore)
• migliorare i test: forse è utile aggiungere un test di integrazione che sottolinei quell’ultimo caso limite che è diventato evidente solo avendo sotto gli occhi tutti i commit

In altre parole…

• La revisione del codice è un compromesso tra precisione e tempo. Ci sono potenzialmente molte cose da fare, scegliete solo quelle più convenienti
  • ricordate che la revisione del codice dovrebbe mirare ad accogliere i cambiamenti e cercare di bloccare solo “qualcosa che peggiora la salute generale del progetto”
  • cercate di individuare e segnalare quanti più problemi possibile: prima vengono trovati, meno costosi sono da risolvere

• la revisione del codice è un buon luogo dove eseguire un po’ di QA
  • non date per scontato che il codice funzioni correttamente solo perché è molto semplice
  • controllate due volte che la richiesta originale (problema/bug/feature) sia stata correttamente compresa e implementata
  • è il primo ciclo di feedback che può evidenziare problemi di alto livello (ad esempio, UX scomoda perché i requisiti iniziali erano sbagliati, API per il login molto scomoda e contorta)

• automatizzate tutte le cose che possono essere automatizzate!
  • linters, formattatori, correttori ortografici… usateli in modo da poter dimenticare gli errori di battitura e lo stile. Il loro costo è abbastanza basso, ma forniscono un minimo di qualità del codice che non dovrebbe essere trascurato
  • investite tempo nella scrittura di test… i test aumentano la salute generale di un progetto e offrono una certa garanzia che la patch funzioni correttamente e che non si romperà facilmente (o in modo silenzioso) in futuro. Come revisori, se individuate un test mancante: lasciate una nota e un breve suggerimento!

• non guardate solo la diff: alcuni problemi riguardano “cosa manca nella diff“!
  • ci sono altre parti del sistema che dovrebbero essere aggiornate per tenere conto del nuovo cambiamento?
  • c’è della documentazione che è andata fuori sincrono?

• chiedete chiarimenti
  • il codice è una responsabilità comune e non ha un vero proprietario. È nell’interesse della squadra avere un codice facile da capire
  • non abbiate paura di suggerire algoritmi o soluzioni più semplici… questo tipo di commenti tende a creare conversazioni molto interessanti e tende a diffondere molta conoscenza

• non dimenticate l’interazione umana
  • date la colpa al codice, non al programmatore
  • siate gentili e cordiali
  • ricordatevi di evidenziare le cose belle

• siate pronti a diventare sviluppatori migliori
  • chiedete le cose che non sapete o non capite
  • suggerite miglioramenti: altre persone impareranno da voi mentre voi imparerete ad insegnare agli altri
  • finirete per familiarizzare con quelle parti del progetto su cui non avete mai lavorato, e saprete come il progetto si sta evolvendo

Complimenti se siete arrivati alla fine, è stata forse una lettura lunga e tortuosa, ma ora potete riposare gli occhi: giuro che questo era l’ultimo elenco puntato!