Nelle revisioni del codice, il revisore dovrebbe sempre presentare una soluzione per i problemi? [chiuso]

Come revisore, il tuo compito è verificare se un pezzo di codice (o un documento) soddisfa determinati obiettivi che sono stati concordati prima della revisione.

Alcuni di questi obiettivi implicano in genere una chiamata di giudizio se l'obiettivo è stato raggiunto o meno. Ad esempio, l'obiettivo che il codice deve essere gestibile in genere richiede una chiamata di giudizio.

Come revisore dei conti, è compito tuo indicare dove gli obiettivi non sono stati raggiunti ed è compito dell'autore assicurarsi che il suo lavoro soddisfi effettivamente gli obiettivi. In questo modo, non è compito tuo dire come devono essere fatte le correzioni.

D'altro canto, basta dire all'autore "questo è difettoso, correggerlo" di solito non porta ad un'atmosfera positiva nella squadra. Per un'atmosfera positiva, è bene almeno indicare perché qualcosa è imperfetto nei tuoi occhi e fornire un'alternativa migliore se ne hai uno.
Inoltre, se stai rivedendo qualcosa che sembra "sbagliato" ma non hai davvero un'alternativa migliore, puoi anche lasciare un commento sulla falsariga di "Questo codice / design non si adatta bene a me, ma io non ho un'alternativa chiara. Possiamo discutere di questo? " e poi cerca di ottenere qualcosa di meglio insieme.

Alcune buone risposte qui, ma penso che manchi un punto importante. Fa una grande differenza di quale codice stai recensendo, quanto è esperta quella persona e come lui o lei gestisce questi suggerimenti. Se conosci bene il tuo compagno di squadra e ti aspetti una nota come "questo codice è difettoso perché ..., fallo in modo diverso" per essere sufficiente a trovare una soluzione migliore, quindi tale un commento può andare bene. Ma ci sono sicuramente persone in cui un tale commento non è sufficiente, e chi ha bisogno di sentirsi dire esattamente come migliorare il proprio codice. Quindi IMHO questa è una chiamata di giudizio che puoi fare solo per il singolo caso.

So generally, as a reviewer, is it fine to say "this code is flawed, do it differently" or do you have to come up with a specific solution?

Nessuno dei due è l'IMO ideale. La cosa migliore da fare è parlare all'autore e risolvere il problema in modo collaborativo.

Le revisioni del codice non devono essere asincrone. Un sacco di problemi si sbloccherà se smetterai di vederli come un processo burocratico e prenderai un po 'di tempo per la comunicazione dal vivo.

In code reviews, should the reviewer always present a solution for issues?

No. Se lo fai non sei un recensore, sei il prossimo programmatore.

In code reviews, should the reviewer never present a solution for issues?

No. Il tuo compito è quello di comunicare il problema a portata di mano. Se presentare una soluzione rende il problema chiaro, fallo. Non aspettarti che segua la tua soluzione. L'unica cosa che puoi fare qui è di fare un punto. Non puoi dettare l'implementazione.

When should a reviewer present a solution for issues?

Quando questo è il modo più efficace per comunicare. Siamo code di scimmie non inglesi. A volte il modo migliore per mostrare che il codice fa schifo ... è meno che ottimale ... è mostrare loro il codice che fa schifo meno ... è più opt ... oh, cavolo, sai cosa intendo.

Il problema principale è che se le persone sapessero come scrivere meglio il codice, in genere lo avrebbero fatto in primo luogo. Se una critica è specifica dipende molto dall'esperienza dell'autore. I programmatori di grande esperienza potrebbero essere in grado di prendere una critica come "questa classe è troppo complicata" e tornare al tavolo da disegno e trovare qualcosa di meglio, perché hanno appena avuto un giorno di riposo a causa di un mal di testa o sono stati sciatti perché erano in fretta.

Di solito, però, devi almeno identificare la fonte della complicazione. "Questa lezione è troppo complicata perché infrange la Legge di Demetra dappertutto". "Questa classe mescola le responsabilità del livello di presentazione e dello strato di persistenza." Imparare a identificare queste ragioni e spiegarle brevemente fa parte del diventare un recensore migliore. Raramente devi entrare in molti dettagli sulle soluzioni.

Esistono due tipi di programmatori non validi: quelli che non seguono le pratiche standard e quelli che "solo" seguono le pratiche standard.

Quando ho avuto contatti di lavoro limitati / fornire feedback a qualcuno, non direi: "Questo è un cattivo progetto". ma qualcosa come "Puoi spiegarmi questa lezione?" Potresti scoprire che è una buona soluzione, che lo sviluppo ha fatto sinceramente il meglio che poteva, o anche un riconoscimento che è una cattiva soluzione, ma è abbastanza buono.

A seconda della risposta, avrai un'idea migliore di come affrontare ogni situazione e persona. Possono riconoscere rapidamente il problema e scoprire la correzione da soli. Potrebbero chiedere aiuto o cercheranno semplicemente di risolverlo da soli.

Ci sono pratiche suggerite nella nostra attività, ma quasi tutte hanno delle eccezioni. Se capisci il progetto e come si sta avvicinando il team, questo può essere il contesto per determinare lo scopo della revisione del codice e come affrontare i problemi.

Mi rendo conto che questo è più un approccio al problema che una soluzione esplicita. Ci sarà troppa variabilità per coprire tutte le situazioni.

Quando ripasso il codice, fornisco solo una soluzione per i problemi che identifico se riesco a farlo con poco sforzo. Cerco di essere specifico su ciò che penso sia il problema, riferendomi alla documentazione esistente laddove possibile. Aspettarsi che un revisore fornisca una soluzione per ogni problema identificato crea un incentivo perverso - scoraggerà il revisore dall'indicare problemi.

La mia opinione si sta rafforzando nel non fornire codice nella maggior parte dei casi, per una serie di motivi:

• Se la spiegazione a parte non è sufficiente, possono sempre chiedere un campione di ciò a cui stai pensando.
• Non stai perdendo tempo cercando di familiarizzare con un codice che non hai toccato da molto tempo, solo per modificarlo leggermente, mentre qualcun altro ha appena trascorso il proprio tempo a fare proprio questo.
• Se hanno già familiarità con il pezzo di codice e non lo sei, dare solo il feedback può portare a un codice migliore di quello che scriveresti. Dare a qualcuno una soluzione pronta spesso li fa semplicemente usare, senza considerare di migliorarlo ulteriormente.
• Fornire sempre una soluzione è al limite della condiscendenza. Stai lavorando con qualcuno, si spera perché sono stati abbastanza bravi da essere assunti. Se sei riuscito a capire perché qualcosa è una cattiva idea, perché non lo imparerebbero ascoltando feedback e facendolo da soli?
• Fornire sempre una soluzione è solo strano. Immagina di essere un paio di programmatori alla loro scrivania: hanno appena finito un paio di righe che ritieni non siano grandiose. Dici solo quello che hai individuato e perché, o prendi la tastiera e mostra subito la tua versione? Questo è ciò che fornisce sempre la tua soluzione alle altre persone.
• Puoi sempre dire cosa faresti invece, senza perdere tempo a scriverlo. Hai fatto proprio questo quando descrivi il primo problema nella domanda.
• Non distribuire cibo, insegnare a pescare;)

Certo, ci sono alcuni casi in cui stai pensando ad un'alternativa specifica, e vale la pena allegarla. Ma questo è davvero raro nella mia esperienza. (molte recensioni, grandi progetti) L'autore originale può sempre chiederti un campione, se necessario.

Anche allora, a causa della terza ragione, quando si fornisce un esempio, può valere la pena di dire, ad esempio, "l'utilizzo di x.foo() renderebbe questo più semplice" piuttosto che una soluzione completa e consentirà all'autore di scriverlo. Ciò consente anche di risparmiare tempo.

Penso che la chiave per la revisione del codice sia quella di concordare le regole prima della revisione.

Se hai una serie di regole chiare, non dovrebbe esserci bisogno di offrire una soluzione. Stai solo controllando che le regole siano state seguite.

L'unica volta che si presenterebbe la domanda di un sostituto sarebbe se l'ideatore originale non fosse in grado di pensare a un modo per implementare la funzione che si adatta alle regole. Supponiamo che tu abbia un requisito di rendimento, ma la funzionalità impiega più tempo della soglia dopo diversi tentativi di ottimizzazione.

Tuttavia! se le tue regole sono soggettive "I nomi devono essere approvati da me!" allora, sì, ti sei appena nominato namer in chief e dovresti aspettarti richieste per liste di nomi accettabili.

L'esempio di ereditarietà (parametri facoltativi) è forse migliore, int che ho visto regole di revisione del codice che vietano i metodi lunghi e i parametri di funzione "troppi". Ma normalmente questi possono essere risolti banalmente dividendo. È la parte "questa soluzione sembra complicare le cose" in cui la tua obiettività si sta intromettendo e forse richiede una giustificazione o una soluzione alternativa.

Se una soluzione potenziale è veloce e facile da scrivere cerco di includerla nei miei commenti di revisione tra pari. In caso contrario, elencherò almeno la mia preoccupazione e perché trovo problematico quel particolare elemento. Nel caso di nomi di variabili / funzioni in cui non riesci a pensare a qualcosa di meglio, di solito, riconosco che non ho un'idea migliore e termino il commento sotto forma di una domanda a risposta aperta nel caso in cui qualcuno possa . In questo modo, se nessuno si presenta con un'opzione migliore, la recensione non viene realmente tenuta in considerazione.

Per l'esempio che hai dato nella tua domanda, con la classe mal progettata. Vorrei lasciare alcuni commenti che, mentre sembra che potrebbe essere eccessivo, l'ereditarietà sarebbe probabilmente il modo migliore per affrontare il problema che il codice sta cercando di risolvere, e lasciamo perdere. Proverò a esprimere una frase in modo da indicare che non è un ostacolo allo show e che dipende dalla discrezione dello sviluppatore se aggiustarlo o no. Vorrei anche aprire con la consapevolezza che non hai particolare familiarità con quella parte del codice e invitare altri sviluppatori e / o revisori esperti a chiarire se c'è una ragione per cui è fatta così com'è.

Vai e parla con la persona di cui stai recensendo il codice. Di 'loro, in maniera amichevole, che hai trovato un po' difficile da capire, e poi discuti con loro come potrebbe essere reso più chiaro.

La comunicazione scritta porta a grandi quantità di tempo sprecato, oltre a risentimenti e incomprensioni.

Faccia a faccia, la larghezza di banda è molto più alta e uno ha il canale laterale emozionale per prevenire l'ostilità.

Se in realtà parli con il ragazzo, è molto più veloce e potresti creare un nuovo amico e scoprire che entrambi ti piacciono di più.