Come eseguire la revisione del codice senza offendere altri sviluppatori [duplicato]

Questo sembra essere un atteggiamento prevalente piuttosto comune tra alcuni sviluppatori. Tutti sembrano ritenere che una revisione del codice sia una sfida al loro lavoro, e questo non ha senso per me. Una revisione del codice è un meccanismo di garanzia della qualità che ha il bonus aggiuntivo di educazione per andare d'accordo con esso. Implementiamo estensivamente le revisioni del codice in cui lavoro e ho promosso all'interno del mio team l'atteggiamento che le revisioni del codice sono un meccanismo di collaborazione più che un processo di qualità.

L'unico modo per iniziare a codificare come team è di vedere il lavoro degli altri e di metterli in discussione. Ecco come si formano le migliori pratiche. La finestra di dialogo è la chiave. Ho inviato il codice agli sviluppatori per motivi sciocchi come la formattazione, le migliori pratiche concordate e l'ortografia. Ho una revisione del codice con un margine molto sottile, e mi aspetto che il mio codice possa resistere allo stesso esame. Ho usato tattiche come il check-in del codice che non avrebbe passato la mia recensione del codice al solo scopo di convincere uno sviluppatore junior a mettermi alla prova . Dimmi che non soddisfa i requisiti. Alzati e fatti un'opinione.

Alcune linee guida che penso che tutti dovrebbero avere quando sono coinvolti in un ambiente di revisione del codice:

1. Riprenditi. Non sei perfetto, e solo perché uno sviluppatore junior è riuscito a vederti fare qualcosa di stupido in un ciclo for non è la fine del mondo. Se sei così bravo, invita le critiche e dimostralo. Aspettatevi le revisioni del codice per rivelare nuove idee su come fare cose, nuove abitudini e, soprattutto, dialoghi costruttivi.
2. Tieni la mente aperta quando rileggi il codice di qualcun altro. Sii ricettivo alle loro idee e assicurati che le modifiche che suggerisci siano suggerite per un motivo. Si presume che il codice del check-in venga compilato, ma ciò non significa che segua le best practice. Assicurati che qualsiasi cosa che fai apparire possa essere salvata con un riferimento citato. Se dici che non segue le migliori pratiche, cita il documento standard e la sezione. Se dici che non è un metodo "performante", quindi avere un link a un documento che mostra perché e, eventualmente, fornisce metriche.
3. Dai suggerimenti utili e spiega perché stai suggerendo qualcosa. Occasionalmente troverai un problema con il codice che si spiega da solo, ma la maggior parte delle volte questa persona ha codificato qualcosa in base alle abitudini. Spiega perché questa abitudine dovrebbe essere modificata e il valore di alterarla (a meno che tu non la spieghi per la quinta volta, in cui hai un problema con il personale).
4. Una volta esaminato il codice, considera tutto ciò che il revisore suggerisce in modo obiettivo. Se stai respingendo, chiediti onestamente se stai solo sulla difensiva o se davvero credi di avere un caso. Se hai un caso, continua la finestra di dialogo. Non essere polemico, portare munizioni come fatti e metriche.
5. Sia che tu sia un revisore o un recensore, utilizza le revisioni del codice come un'opportunità per educare. Sia che tu stia educando te stesso o la persona che stai recensendo, se c'è una discrepanza, allora c'è la possibilità di imparare da qualche parte. Fai buon uso di esso.
6. Fai domande e preparati a rispondere in modo veritiero alle tue domande. Di recente ho fatto una dichiarazione che era "pseudo-vera". Non era sbagliato, ma sicuramente non era giusto. Uno sviluppatore junior mi ha sfidato e non sono d'accordo. La mia risposta è stata "Non ho visto questo comportamento, ma se riesci a trovarmi un documento su questo mi piacerebbe leggerlo". Ho passato circa un'ora quel pomeriggio a leggere il documento che mi ha inviato, e ora ho una risposta molto migliore (educata) quando mi trovo di fronte a una situazione simile.

Dato il taglio dell'istruzione in cui inserisco le recensioni del codice, spesso faccio le mie risposte sulle domande. Se trovo qualcosa di vistoso, istruirò lo sviluppatore a correggerlo. Altrimenti farò le domande. "Perché hai usato il metodo A per raggiungere l'obiettivo B?" "Quale guadagno ha dichiarare una variabile nell'istanza del suo utilizzo nel metodo Z?" "Vedo che hai copiato / incollato del codice, hai considerato il refactoring? Non hai refactoring, qual è stato il ragionamento alla base di questa scelta?" Il codice non progredisce fino a quando il revisore non lo approva e il revisore non lo approverà fino a quando le domande non avranno risposta. Quando inquadrato in modo curioso che indica che non capisci gli sviluppatori, ragionamento diventa meno conflittuale e assume più di una sensazione istruttiva.

Per aiutare le persone a prendere meno personalmente una recensione del codice, cerca di indirizzare critiche e suggerimenti al codice stesso e non al suo autore. Ad esempio,

The function "getFoo" should do xyz because....

è migliore di

You should do xyz in function "getFoo" because...

D'altra parte, se vuoi esprimere un'opinione positiva puoi lodare l'autore. Ad esempio,

I like how you used abc algorithm because ...

In primo luogo, il team dovrebbe concordare l'obiettivo della revisione del codice (non si dovrebbe dare per scontato tale accordo, poiché la revisione del codice può essere utilizzata per una varietà di cose, e diverse organizzazioni lo fanno per diversi motivi. , Supporrò che l'obiettivo fosse migliorare il lavoro futuro di uno sviluppatore.

Per migliorare, lo sviluppatore deve sapere

1. cosa era buono? - > Dovrei continuare a farlo
2. che cosa era male? - > Dovrei cambiarlo
3. e perché? - > Quindi posso valutare la gravità del problema, oltre a riconoscere problemi simili in futuro

Di quale azione si tratta?

Il feedback dovrebbe riguardare un comportamento, non una persona. Pertanto, dovrebbe identificare (preferibilmente con esempi) il comportamento del feedback. Quindi non dire:

You're lazy.

perché non si tratta di comportamento, ma di personalità (percepita). Inoltre, è troppo vago: si tratta della lunga pausa pranzo? Lenta battitura? Non rispondere alle e-mail in modo tempestivo? Troppe funzionalità implementate? Invece, dovresti dire:

Last week, you were assigned bug #1059. You have marked this defect resolved, but the bug still exists, and can be reproduced with the instructions provided in the trouble ticket.

E perché?

Successivamente, dovresti descrivere perché quell'azione o codice era buono o cattivo. La ragione addotta dovrebbe essere obiettiva e di sufficiente importanza per giustificare un cambiamento. Ad esempio:

AClass.amethod() is invoked by concurrent threads, but not thread safe. Data corruption is likely.

o

This method name is misleading. It sounds as if we're merely calulating the pay grade, but actually also pays the employee. Somebody not familiar with the implementation is likely to misuse the method, causing accidental salary payments ...

Naturalmente, a volte non si ha una discussione ironclad perché il codice è cattivo (o buono). Questo significa che non sai che hai ragione. Pertanto è prudente essere curiosi, piuttosto che presuntuosi:

This looks awfully complex. Why don't you just invoke AClass.amethod() instead?

o

This appears to be nearly identical to AClass.amethod(). Why can we not reuse that method?

Puoi anche dare un feedback positivo, naturalmente:

That's a great approach to coordinate the worker threads! I usually do X instead, and I always wondered how I could make it more robust. Now I know :-)

Non fare questo

Per illustrare l'importanza di quanto sopra, consideriamo un esempio di contatore del mondo reale preso alla lettera da una revisione formale del codice:

The code leaves a poorly thought out impression.

E questo è tutto ciò che il recensore ha scritto su questa scoperta.

Questo è un feedback terribile su così tanti livelli:

1. Il feedback implica qualcosa di non lusinghiero nei confronti dell'autore (cioè, che lui pensa male)
2. Il feedback riguarda un'impressione. Questo non è obiettivo.
3. Il feedback non identifica alcuna parte del codice che deve essere modificata,
4. né dare alcuna ragione per cui dovrebbe essere.

Il destinatario di tale "feedback" non può sapere cosa deve cambiare, né perché, e quindi non impara nulla. In realtà, non può nemmeno sapere che il feedback è giusto e, vista la conclusione poco lusinghiera e poco lusinghiera, è molto più probabile vederlo come un attacco personale da parte di qualcuno che vuole elevarsi spingendo giù i suoi pari. Gridare è probabile che ne consegua.

Una revisione del codice non dovrebbe essere solo un fail / pass con il solo esito negativo che restituisce un feedback dettagliato. Questo tende a concentrarsi sul negativo e allontana le persone dall'esporre se stessi alle critiche.

I revisori potrebbero anche dare una risposta più negativa per ottenere più risalto agli occhi del manager in termini di "look sto facendo il mio lavoro e criticando tutto ciò che incontra la mia scrivania"

Se aggiungi un campo di commento che viene restituito al mittente o passa, allora si sentirà più positivo sul suo lavoro e il revisore può ancora dare un feedback su cosa potrebbe ancora essere migliore anche se il codice supera la recensione. Ciò consentirebbe anche al revisore di eseguire un refactoring limitato del codice (formattazione, ridenominazione delle variabili) senza la necessità di disturbare lo sviluppatore per le piccole modifiche oltre a notificarlo.

Dicono che "il codice dovrebbe appartenere al team", quindi una recensione non è tanto un attacco personale a un singolo codificatore, ma un passo di qualità per garantire che le cose siano comprensibili dal resto del team. Direi la maggior parte dei posti in cui ho lavorato, questo è compreso e accettato.

Tuttavia, ho intervistato un ragazzo che era così contrario all'idea di rivedere il codice di altre persone che lo abbiamo rifiutato come candidato! (cioè - non di aver rivisto il suo codice, ma facendo recensioni ... era strano)

Questo mi ha fatto riflettere un po 'sul problema di come alcune persone approcciano le recensioni. Direi che un modo migliore di fare revisioni del codice è ricordare la prima riga della mia risposta e basarci su questo. Per fare questo è abbastanza semplice: una revisione del codice smette di essere percepita come "qualcuno controlla il mio codice e mi dice cosa ho fatto di sbagliato" e diventa un'opportunità per il programmatore di dire a qualcun altro cosa fa il loro codice, come lo fa e quali modifiche hanno apportato.

In questo caso, il codificatore finirà per rivedere il proprio codice, con un paio di occhi in più per aiutare a individuare il bit che potrebbero perdere. È anche una possibilità per il programmatore di spiegare perché ha fatto le cose in un certo modo. Un simile approccio è altrettanto efficace (se non di più in quanto si ha un buon scambio di comunicazioni) e considerevolmente più amichevole. Non penso che ci vorrebbe più tempo considerando che la recensione dovrebbe essere più veloce in quanto il codificatore originale può spiegare direttamente le modifiche. Le persone dovrebbero anche comprendere la necessità di tale revisione poiché stanno "trasferendo" la conoscenza a un altro membro del team che potrebbe essere tenuto a conservare il codice in futuro.

Puoi potenziarlo facendo in modo che il 'revisore' venga preparato con i requisiti originali per la modifica, e contrassegnalo come la loro soluzione è dettagliata, quindi sembrerebbe meno simile a una revisione del codice.