Peer / Code Review Frustrazioni

Tipi di recensioni

Non esiste un unico modo per fare recensioni tra pari. Esistono molti modi per giudicare se il codice sia di qualità sufficientemente elevata. Chiaramente c'è la domanda se sia bug o se abbia soluzioni che non sono scalabili o che sono fragili. I problemi di conformità agli standard e alle linee guida locali, anche se forse non così importanti come alcuni degli altri, sono anche parte di ciò che contribuisce al codice di alta qualità.

Tipi di revisori

Proprio come abbiamo diversi criteri per giudicare il software, anche le persone che fanno il giudizio sono diverse. Abbiamo tutti le nostre capacità e preferenze. Alcuni potrebbero pensare che aderire agli standard locali sia molto importante, così come altri potrebbero essere più interessati all'utilizzo della memoria, o alla copertura del codice dei test, e così via. Vuoi tutti questi tipi di recensioni, perché nel complesso ti aiuteranno a scrivere codice migliore.

Una revisione tra pari è la collaborazione, non un gioco di tag

Non sono sicuro che tu abbia il diritto di dire loro come fare il loro lavoro. A meno che tu non sappia diversamente con certezza, supponi che questa persona stia cercando di contribuire nel modo che ritiene opportuno. Tuttavia, se vedi margini di miglioramento o sospetti, forse non capiscono cosa è previsto in una revisione tra pari, parla con loro .

Il punto di una recensione peer è quello di coinvolgere i tuoi pari . Il coinvolgimento non è il lancio di un codice su un muro e l'attesa di una risposta da respingere. Il coinvolgimento sta lavorando insieme per creare un codice migliore. Impegnarsi in una conversazione con loro.

Consigli

Verso la fine della tua domanda hai scritto:

how would I go about encouraging colleagues to actually look for faults in the code in balance with glaring aesthetic errors?

Ancora una volta, la risposta è la comunicazione. Forse puoi chiedere a loro "hey, apprezzo che tu abbia recepito questi errori, mi sarebbe di grande aiuto se potessi concentrarti su alcuni aspetti più profondi, come se strutturassi il mio codice in modo corretto, so che ci vorrà del tempo, ma sarebbe davvero aiuto ".

Su una nota più pragmatica, personalmente divido i commenti di revisione del codice in due campi e li esprimo in modo appropriato: cose che devono essere sistemate e cose che sono più estetiche. Non impedirei mai che il codice solido e funzionante venga archiviato se ci fossero troppe righe vuote alla fine di un file. Lo farò notare, tuttavia, ma lo farò con qualcosa del tipo "le nostre linee guida dicono di avere una sola riga vuota alla fine, e tu ne hai 20. Non è un ostacolo allo spettacolo, ma se ne hai la possibilità potrebbe volerlo aggiustare ".

Ecco qualcos'altro da considerare: potrebbe essere un tuo piccolo problema che facciano una revisione così superficiale del tuo codice. Potrebbe benissimo essere che un peggio di loro è che tu (o qualche altro compagno di squadra che ottiene una recensione simile) sia sciatto rispetto agli standard di codifica della tua organizzazione, ed è così che hanno scelto per comunicarlo con te.

Che cosa fare dopo la revisione

Infine, un po 'di consigli dopo la revisione: quando si esegue il codice dopo una revisione, è possibile prendere in considerazione la cura di tutte le cose estetiche in un commit e le modifiche funzionali in un altro. Mescolare i due può rendere difficile distinguere i cambiamenti significativi da quelli insignificanti. Apporta tutte le modifiche estetiche e poi esegui il commit con un messaggio come "cosmetico, senza modifiche funzionali".

Le persone commentano la formattazione del codice e gli errori di battitura perché sono facili da individuare e non richiedono molto sforzo da parte loro.

Questa parte è facile da correggere - quasi tutte le lingue hanno uno strumento di controllo di linter o stile. Collegalo al tuo processo di creazione, in modo che fallisca la build se c'è uno spazio ridondante. Di conseguenza non ci saranno problemi di stile da commentare.

Farli trovare problemi reali potrebbe essere una vera sfida. Non solo questo richiede una speciale mentalità orientata allo studio e al dettaglio, ma anche una motivazione significativa.

E questa motivazione viene dall'esperienza. Esperienza nel provare a lavorare con codice scadente scritto da sviluppatori precedenti. Gli ingegneri senior ne hanno in abbondanza. Nuotare nell'oceano di merda ti dà un bel desiderio di non arrivare di nuovo.

Se devo scegliere una cosa principale da controllare durante la revisione del codice, sarebbe la manutenibilità del codice. In ogni momento, lo sviluppatore che esamina questo codice dovrebbe capirlo ed essere pronto a migliorarlo e modificarlo. Se non ha la minima idea di cosa sta succedendo qui, deve dirlo subito e il codice deve essere riscritto.

Ecco la risposta pratica:

Scenario 1 : Sei il membro anziano e qualcuno che può decidere la pratica

Convoca una riunione e presenta le regole e le linee guida per la revisione del codice. Fidati di me, le linee guida chiare funzionano meglio di qualsiasi sistema di "onore" o formazione. Metti in chiaro che i problemi di formattazione del codice non devono essere risolti. Alcuni membri obietteranno. Ascoltali e poi chiedi loro di seguire le linee guida per i primi mesi come esperimento. Mostra la volontà di cambiare se le linee guida attuali non funzionano.

Scenario 2 : Non sei una persona che decide la pratica o sei un membro relativamente giovane del team

La soluzione migliore è risolvere i problemi di revisione del codice fino a quando non riesci a raggiungere scenario 1 .

La semplice risposta per evitare commenti "banali" sulla revisione del codice è di insistere (per motivi di efficienza) che il revisore dovrebbe essere quello che li risolve. Quindi se il revisore scopre che hai (horror!) Perso un fullstop o hai scritto un commento in modo errato, dovrebbero semplicemente aggiustarlo e andare avanti.

Nella mia esperienza ciò significa che:

• i tuoi revisori smettono di rendere questi commenti di recensione relativamente inutili e di rimandarli indietro per essere risolti.
• solo la maggior parte dei revisori OCD li risolverà, il che significa che la maggior parte delle tue recensioni passerà. questo ha l'effetto di richiedere agli sviluppatori di eseguire revisioni più sostanziali, quelli che segnalano banalità perché in realtà non stanno rivedendo il codice finiranno per dover giustificare il motivo per cui passano tutte le loro recensioni senza commenti.

In ogni caso, questo è un vantaggio. Il costo di una recensione fallita è alto in termini di far sì che uno sviluppatore interrompa ciò su cui sta lavorando e rivisita il suo codice, e la successiva revisione di follow-up. Se una recensione riscontra la qualità del codice reale o problemi architettonici, allora questo costo vale ogni centesimo, ma non pagare questo costo per i trivia.

Come qualcuno che è stato su entrambi i lati di questo (revisione del codice scritto da altri, oltre al codice che ho scritto revisionato), direi che ho tre categorie in cui ogni feedback cade. Bene, quattro, c'è anche il delizioso caso di "tutto va bene".

"Sarebbe bello, ma non ti bloccherà" (se tutti i feedback rientrano in questa categoria, potrei inviare la richiesta di fusione con un "si unirà a XX: XX, a meno che tu non mi dica che vuoi correggili ", o" sicuro, vai avanti per il check-in, qualunque sia il blocco che il sistema avrebbe generato è stato disabilitato "):

• Dimentica i punti fermi alla fine delle frasi (nelle stringhe doc o nei commenti). Grammatica goffa in tutto ciò che non viene emesso dagli utenti in alcun modo forma o forma (ancora, stringhe di documenti e commenti)
• Codice che ha un modo più elegante, ma quello che c'è è comprensibile e idiomatico (probabilmente inserirò anche il mio suggerimento, quindi è facile lasciare o correggere)

"Cose che devono essere corrette, ma mi fido di te per farlo" (se tutte le cose rientrano in questa o nella categoria precedente, risponderò con "aggiustali, mi fonderò quando dici me hai finito "(ea quel punto probabilmente scriverò velocemente per vedere che non è spuntato nient'altro)):

• Minori cavoli ("stai confrontando un booleano con true , è un po 'paranoico ...", ...)
• Violazioni di stile minori, ("la guida di stile dice X, quello che hai fatto è al di fuori di questo, io farei Y o Z, a seconda del modo in cui vuoi andare")
• Alcuni casi di test mancanti, che non dovrebbero essere difficili da scrivere

E infine, "le cose che sono problematiche, dovrò rivedere il tuo codice una volta risolti questi problemi" (se ce n'è qualcuno in questa categoria, ci sarà bisogno di un altro giro di revisione, quindi inserisci un commenta dicendo "ci sono anche un paio di cose minori, sarebbe bello vedere alcune di quelle fisse mentre ci sei" se c'è qualcosa dalle prime due categorie presenti):

• Sarebbero cose come "no, c'è davvero un modo MOLTO migliore di scriverlo", "non stai calcolando ciò che ti aspetti, perché il tuo test di unità manca questi casi limite" e tutti gli altri gravi problemi.

Sembra che alcune persone abbiano dimenticato la domanda più importante: che cosa vuoi veramente ottenere con le revisioni del codice?

Lo scopo delle revisioni del codice è quello di ottenere codice privo di bug e manutenibile più veloce. Le revisioni del codice lo raggiungono in diversi modi: gli sviluppatori scrivono un codice migliore in primo luogo perché sanno che sarà esaminato, la conoscenza è condivisa come parte del processo di revisione, i bug saranno trovati perché il revisore non è cieco ai propri errori come sviluppatori può essere.

Se vedi il processo di revisione come un'opportunità per mettere giù i tuoi colleghi, o per creare lavoro per loro, allora stai sbagliando.