Come fare una 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:

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:

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 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:

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é:

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:

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:

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:

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:

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:

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:

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:

Imparare cose nuove

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

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:

In altre parole…

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!