Cultura della revisione del codice: perché è necessario averne una

Quante volte incroci le dita quando apri una richiesta di pull nella speranza che non venga assegnata a quello sviluppatore del tuo team che lascia sempre una manciata di commenti su tutto e niente? Non sto parlando di quelli molto ragionevoli che rivelano errori commessi o, per favore no, code smell.

Si tratta di commenti sul tuo stile di codice o di piccole cose soggettive che non mirano a migliorare la base di codice alla fine, ma piuttosto a dire "L'avrei fatto in un altro modo perché mi piace di più". Come membro del team responsabile, è probabile che tu li affronti.

Non importa quale opzione decidi di seguire (applica semplicemente tutti i cambiamenti o opponiti con lunghe spiegazioni sul perché non sei d'accordo su questo e quello), probabilmente finirai per sentirti agitato, esausto e frustrato per il tempo speso in cose non essenziali. E ne valeva davvero la pena? Questa domanda ti verrà in mente ogni volta che ti troverai ad affrontare situazioni del genere.

Oppure, a volte, hai un'altra estremità del bastone: insta approva senza commenti e senza segni che un revisore abbia effettivamente controllato attentamente le tue modifiche. Che le abbiano riviste, e non ci siano input oggettivi, o meno, ti ritrovi a mettere in discussione sia te stesso che questo compagno di squadra.

Certo, anche se tutto va bene, non sarebbe bello includere un breve saluto all'autore e assicurarsi di essere sulla stessa lunghezza d'onda e di non avere più dubbi?

Se entrambe le storie sopra ti suonano troppo familiari, questo articolo fa per te. Ciò che manca al tuo team è una cultura di pull request o, in altre parole, un set di linee guida su come comunicare nelle pull request per supportare un processo di collaborazione amichevole e altamente efficiente. Parlerò degli elementi essenziali che i revisori vorrebbero cercare e anche di esempi di commenti discutibili, più semplicemente, pignolerie, che creano attriti o talvolta persino problemi che vanno oltre i tuoi sentimenti personali.

Essendo uno sviluppatore iOS, uso Swift nei miei esempi di codice, ma in generale questo argomento è importante per qualsiasi sviluppatore, indipendentemente dalla piattaforma o dal linguaggio utilizzato quotidianamente.

Tuttavia, credo che la cultura della revisione del codice sia ancora più rilevante per le piattaforme Apple perché, secondo la mia esperienza, tendiamo a essere più pignoli in molti modi. Probabilmente deriva da una mentalità perfezionista ereditata dalla visione di Steve ai vecchi tempi.

Lezione appresa

Pull request o merge request sono un modo per verificare le modifiche al codice con i colleghi e controllare se ci sono errori, valutarne la prontezza prima di passare alle fasi successive e, infine, agli utenti finali. È anche uno dei principali canali di comunicazione tra sviluppatori. A volte, potremmo non conoscere nemmeno una persona al di fuori dei thread nelle PR.

Molto spesso, soprattutto nelle fasi iniziali della loro carriera, gli sviluppatori non prestano attenzione alla loro comunicazione di PR. In particolare, a come i loro commenti possono essere percepiti da altre persone, se i punti sollevati sono chiari e correttamente scritti in inglese e così via.

Credetemi, ci sono passato anch'io e ho fatto degli errori in passato. Un'esperienza in particolare mi è rimasta impressa fino a oggi. La tiro fuori quasi ogni volta durante le discussioni sulla cultura delle PR perché sottolinea perfettamente l'importanza di questo argomento.

Anni fa, quando cercavo un nuovo lavoro, mi è stata posta una richiesta di pull simulata che dovevo esaminare. Non ho pensato a tutto attentamente quella volta perché ero emozionato di aver ricevuto un compito facile invece di un'altra tortura di Leetcode. Ovviamente, non è stato così facile. È stato un test sulla cultura della richiesta di pull che ho fallito drasticamente.

Ho letto velocemente le PR e ho lasciato un mucchio di commenti per lo più inutili, ad esempio:

 import UIKit import AVFoundation // It would be nice to list your imports alphabetically, so that it's easier to read.

Oppure un altro:

 func someMethod() { // about 30 lines of code } // What do you think about splitting this method in half?

In altri rari commenti in cui ho individuato un problema, non sono stato abbastanza chiaro al riguardo e non ho fornito alcuna soluzione ragionevole. Ma il fallimento principale di questo esercizio è stato che ho tralasciato un paio di problemi di performance perché la mia attenzione non era nel posto giusto. Alla fine, ho ricevuto il seguente feedback:

Ho la sensazione che ti siano sfuggiti molti dettagli importanti, come la correttezza del codice, le prestazioni e i miglioramenti generali (con alcune piccole eccezioni).

Hai concentrato gran parte della revisione sulla formattazione, che dovrebbe essere una discussione di gruppo e/o utilizzare strumenti di formattazione. Ciò consente anche agli sviluppatori di concentrarsi sulle cose giuste in una revisione.

Da allora, mi sono reso conto che il mio approccio alle revisioni del codice era tutt'altro che perfetto e mancava di molti aspetti importanti.

Questo esempio è in un certo senso piuttosto primitivo, ma mostra chiaramente come la mancanza di una cultura delle pubbliche relazioni possa facilmente trasformarsi in una strada pericolosa che porta direttamente a bug e problemi in produzione.

Entriamo nei dettagli e iniziamo con qualcosa che un revisore del codice dovrebbe evitare: scrivere commenti pignoli.

Cos'è un commento pignolo?

Ci sono molti modi per esprimere la tua logica e i tuoi pensieri nel codice. I linguaggi di programmazione moderni ti danno abbastanza versatilità per farlo e spesso non c'è giusto o sbagliato tra più approcci. Tuttavia, non vogliamo che i nostri progetti sembrino mostri di Frankenstein: ogni team dovrebbe avere uno stile di codice decente con linee guida.

Allo stesso tempo, non possiamo e non dovremmo controllare ogni riga di codice dei nostri compagni di squadra. Credo che trovare un buon equilibrio tra i due sia una virtù a cui idealmente vogliamo tendere.

Un commento nitpick è una richiesta di apportare una modifica, solitamente piccola, che non ha ragioni oggettive per farlo. Spesso, è una proiezione di una preferenza personale di un revisore del codice nel codice revisionato. Lasciate che ne mostri alcuni esempi:

 // Original guard let newValue, newValue != oldValue else { return } // Change request guard let newValue, newValue != oldValue else { return }

Se me lo chiedete, sceglierei sempre la seconda opzione perché secondo me è più facile eseguire il debug di tale codice e mettere un breakpoint proprio sulla riga di ritorno. Ma potete sostenere che la prima variante fa risparmiare 2 righe, e potete vedere questo stile anche dagli ingegneri Apple.

 // Original func handleErrorIfNeeded(_ error: SomeError) { if error == .failedRequest { return } if error == .validationFailed { state.errorMessage = “Data input is incorrect” return } // … other cases } // Change request func handleErrorIfNeeded(_ error: SomeError) { guard error != .failedRequest else { return } guard error != .validationFailed { state.errorMessage = “Data input is incorrect” return } // … other cases }

In molte situazioni logiche, guard e if in Swift possono essere usati in modo intercambiabile. Tuttavia, direi che non hanno la stessa semantica del linguaggio. guard , come suggerisce il nome, vuole "proteggere" il flusso del codice da un risultato indesiderato, mentre if è neutrale nella sua natura e consente una probabilità del 50-50% che si verifichino percorsi di codice diversi. Entrambe le varianti tecnicamente non sono sbagliate e sono perfettamente leggibili, comunque.

E l'ultimo:

 // Original, the constant value is not being repeated anywhere else func calculateDuration(endDuration: Duration) -> Duration { let startDuration = Duration.milliseconds(130) // … calculation return finalDuration } // Change request func calculateDuration(endDuration: Duration) -> Duration { let startDuration = Duration.milliseconds(Constant.startDuration) // … calculation return finalDuration } enum Constant { static let startDuration = Double(130) }

Si può discutere sulla necessità di estrarre qualsiasi costante in uno spazio dei nomi separato se viene usata una sola volta e quando non è una costante magica. Nella prima variante, si sa chiaramente qual è questo numero di 130. Tuttavia, qualcuno ti dirà di estrarre qualsiasi costante in questo modo, non importa cosa.

Tutti gli esempi sopra sono modifiche pignole. Entrambe le varianti sono perfettamente a posto, leggibili da chiunque e non interrompono il codice. Sono semplicemente alternative con lo stesso peso nell'utilizzo.

Quindi, cosa vuoi fare con commenti come questi? Dal punto di vista di un revisore, alcuni diranno qualcosa del genere:

Beh, è solo un suggerimento. Non è obbligatorio applicarlo.

Vero, ma allo stesso tempo è importante farlo sapere all'autore, ad esempio:

È solo un suggerimento, sentiti libero di scartarlo, ma cosa ne pensi di questo approccio: … ?

Tuttavia, la mia opinione sui cavilli è di non pubblicarli affatto . Perché?

Diciamo che hai inviato un commento pignolo, evidenziando persino che si tratta di un suggerimento di bassa priorità. Cosa succede dopo?

• 1a opzione: l'autore applica la modifica. Cosa significa realmente dietro le quinte?
  • Devono salvare le modifiche correnti che stanno apportando.
  • Ora devono passare alla filiale PR.
  • Devono applicare la modifica.
  • Potrebbero risolvere i conflitti.
  • Devono promuovere il cambiamento.
  • Verrà quindi attivato un altro ciclo di test CI a causa del nuovo codice inserito.
  • Devono rispondere al commento con un testo o un'emoji.

Da un lato, è un insieme di attività molto semplici che svolgiamo ogni giorno, come sviluppatori. Ma immagina, non è un commento pignolo, ma 10. Oppure hai 5 richieste di pull aperte contemporaneamente con pignolo di quantità diverse. Ne valeva davvero la pena, il tuo tempo e la seccatura? Quando sappiamo cos'è un pignolo, la risposta è no. Preferirei consegnare le mie modifiche più velocemente e risparmiare tempo.

• 2a opzione: l'autore non applica la modifica.

  • In molte situazioni, è semplicemente maleducato ignorare il commento del tuo compagno di squadra senza rispondere affatto. E potrebbe sembrare maleducato anche rispondere semplicemente con:

    
    

    Grazie per il suggerimento, ma non mi piace. Non lo applicherò, perché non lo ritengo necessario.

  
  

  • Invece, come autore, probabilmente approfondirai la spiegazione dei dettagli del perché non applicherai le modifiche. Ad esempio, la tua visione su guard vs let come sopra.

    
    

  • Quindi, sarebbe fantastico se il recensore accettasse semplicemente la tua opinione e andasse avanti. Ma potrebbe anche percepire la resistenza e rispondere perché ritiene sinceramente che il suo modo di fare sia migliore.

    
    

  • Tali discussioni possono facilmente sproporzionarsi con lunghi commenti avanti e indietro, il che non è affatto produttivo. E alla fine non porta da nessuna parte. Potete entrambi concordare di non essere d'accordo, non c'è un "vincitore", ma la domanda finale è la stessa. Ne è valsa davvero la pena, il tuo tempo e la seccatura?

  
  

La mia prassi è quella di filtrare queste pignolerie nelle revisioni del codice come revisore, prima ancora di pubblicare qualsiasi cosa, ponendomi queste domande:

È davvero importante cambiare qualcosa?

Ci sono delle ragioni oggettive per questo? Se sì, elencale.

Voglio postare questa osservazione solo perché l'avrei fatto diversamente? O c'è una preoccupazione reale?

In questo modo, puoi rimuovere il "rumore" dei commenti pignoli dalla revisione del codice e lasciare solo le parti importanti, quelle su cui devi concentrarti per scalabilità, robustezza, sicurezza dei thread, ecc.

Ma perché compaiono tali commenti? Di solito, è un segno che i revisori non hanno ben chiaro su cosa concentrarsi o cercare nelle pull request. Nella prossima sezione, discuteremo esattamente di questo.

Come creare una cultura di revisione del codice

Vediamo come possiamo creare un modello di "gold standard" per le PR. Elencherò una serie di valori e linee guida che, a mio parere, sono essenziali per migliorare le revisioni del codice:

1. Per tutti: definisci un set di bucket o categorie per i tuoi commenti di revisione del codice e assegna loro diversi livelli di severità. Mostra sempre esempi ai tuoi colleghi, in modo che siate tutti sulla stessa pagina. Esempio:

…E così via. Può essere diverso nel tuo team, ma l'idea qui è di avere una struttura e una classificazione. Portare valori di severità ci consente di stabilire meglio le priorità e concentrarci sull'essenziale.

2. Revisore : prova ad assegnare uno slot di tempo specifico per le tue revisioni del codice. Non affrettare le cose e permettiti di immergerti più a fondo nel significato delle modifiche al codice, non nella sintassi. Ciò dovrebbe migliorare la qualità e la chiarezza dei tuoi commenti.

   
   

3. Autore: Aiuta i tuoi pari. Se sai che ci sono alcune parti che potrebbero non essere chiare a causa della mancanza di contesto o perché la tua PR è solo una parte di una serie di cose a venire, lascia semplicemente un commento con una spiegazione sulla tua richiesta di pull in anticipo. Farà risparmiare tempo a entrambe le parti!

   
   

4. Revisore: rileggi i tuoi commenti prima di postarli. Mettiti nei panni dell'altra parte. È tutto chiaro? Cerca di attenerti alla seguente struttura:

   1. Esponi la tua preoccupazione.
   2. Perché è importante.
   3. Cosa suggerisci di fare invece?

   
   

5. Per tutti: Controlla la grammatica e l'ortografia in inglese. Sì, per molti di noi l'inglese non è la nostra lingua madre, e a volte è una cosa difficile da fare. Ma se non è scritto bene, può portare a tutti i tipi di confusione e disallineamenti che non vuoi avere.

   
   

6. Per tutti: come viene percepito il tuo commento? Sembra amichevole? Questo punto è abbastanza simile al precedente, ma l'idea è di mantenere un approccio sano e amichevole nella forma scritta. Ad esempio:

   
   

   È necessario estrarre questa logica in un metodo separato.

   
   

   Anche se non c'è niente di sbagliato grammaticalmente in questa frase, la parola "must" è raramente usata in questo contesto e può essere trattata come un comando per un lettore. I funzionari governativi ti diranno cosa devi fare perché ci sono delle leggi. Ma non è il tipo di formulazione che vuoi usare con i tuoi colleghi quando siete nella stessa squadra. Prova invece questo:

   
   

   Cosa ne pensi di estrarre questa logica in un metodo separato?

   
   

7. Revisore: bilancia il negativo con il positivo. Se hai lasciato una manciata di commenti critici, trovane alcuni buoni e mettili in evidenza. È una piccola cosa, ma migliora la percezione generale agli occhi dell'autore.

   
   

8. Revisore: se tutto va bene nelle modifiche al codice, non esitare a postare elogi. L'approvazione di Insta senza segnali non è un comportamento così buono e lascia molto a cui fare domande. Pubblica un messaggio con le cose esatte che hai trovato fantastiche nel lavoro del tuo pari. Il semplice "LGTM" è troppo generico e può essere percepito come sprezzante.

Penso che questi siano buoni punti da cui partire nel tuo team per lavorare verso una comunicazione migliore e più sana nelle tue pull request. La parte più difficile dopo è continuare a seguire queste linee guida. A volte, possono essere due passi avanti e uno indietro.

Conclusione

Alla fine, la codifica non è sempre la parte più difficile. La comunicazione e la collaborazione possono essere più impegnative, specialmente quando si proviene da paesi e background diversi, con esperienze e pensieri propri.

Spero che questa panoramica sulla cultura della revisione del codice ti sia stata utile e che forse potrai trarne qualcosa di utile.

Buona fortuna e amichevoli recensioni del codice a tutti voi!