Quale feedback è o non è appropriato per una revisione del codice? [duplicare]

Una revisione del codice è limitata alla revisione del codice che viene modificato e a qualsiasi cosa direttamente correlata a questo. Ai fini della revisione del codice, la progettazione o i requisiti non sono appropriati per la discussione. Idealmente dovresti aver scelto il design prima che il codice sia stato scritto, quindi se trovi questi problemi durante una revisione del codice, c'è stato un errore nella comunicazione (qualcuno ha frainteso i requisiti).

Non fraintendermi; è molto importante avere il design e i requisiti giusti, non importa quando si scoprono dei problemi con loro, ma se li si scopre durante una revisione del codice, i suggerimenti per farlo in un modo diverso sono non parte del recensione vera e propria!

Should code review comments be limited to actual problems with the code?

Si

Are criticisms about other LOC in the file appropriate even if those lines are not being modified? i.e., criticism of pre-existing code?

A meno che non siano correlati al codice che si sta modificando, no. Ad esempio, "Vedo che stai modificando queste righe in questo modo, sarebbe bello cambiare anche queste altre linee" - è appropriato.

What about suggestions of different ways to solve the same problem? Should I suggest a new way only when a case can be made for it being better? How do I draw the line between contributing ideas but not sounding like a know-it-all?

Non appropriato

In matters which are purely a difference of opinion, who should get the 'final say'? The author, the person who knows the code base the best, the person with the highest title or someone else?

Chiunque ha l'ultima parola su ciò che entra nel codice base. Questo non è correlato al processo di revisione del codice.

Any other advice for ensuring the code reviews serve their purpose without unnecessarily blocking the developer's progress?

Le revisioni del codice devono essere ottimizzate in modo da non bloccare mai il processo di uno sviluppatore. Ecco alcuni problemi a cui prestare attenzione:

• Assicurati che il codice venga esaminato tempestivamente; questo significa che la persona che sta esaminando il codice non dovrebbe essere troppo impegnata
• Il revisore deve essere abbastanza informato sul codice in fase di revisione
• Assicurati che le persone non debbano mai ripetere il lavoro dopo che i problemi sono stati scoperti durante una revisione del codice. Cioè, avere specifiche chiare o comunicare tra il programmatore e il revisore su quale sarà la soluzione proposta, prima che il codice sia stato scritto.
• Promuovi una cultura di recensioni aperte: permetti a chiunque di rivedere facilmente il codice di altre persone, perché le recensioni di codice sono un ottimo modo per condividere le conoscenze tra gli sviluppatori.
• Puntare agli standard di codifica dell'organizzazione ogni volta che sorgono differenze di opinione. Idealmente lo standard è l'ultima parola, e se sorgono conflitti perché non sono coperti dallo standard, aggiorna lo standard.

Should code review comments be limited to actual problems with the code?

Sì. Lo scopo di una revisione è trovare dove il codice non soddisferà le sue specifiche (la progettazione e, per estensione, i requisiti). Se trovi qualcosa di strano / mancante nel design o nei requisiti, l'autore del codice di solito non è in grado di fare qualcosa a riguardo (almeno non senza incorrere in un ritardo significativo). Se riscontri tali problemi durante una revisione del codice, dovresti inserire una segnalazione di bug e continuare la revisione assumendo che i requisiti / il design siano effettivamente corretti.

Are criticisms about other LOC in the file appropriate even if those lines are not being modified? i.e., criticism of pre-existing code?

In una revisione del codice, dovresti solo criticare ciò che ti viene chiesto di guardare. L'unica cosa che dovresti far apparire al di fuori di quel changeset sono luoghi in cui l'autore ha mancato una modifica richiesta o luoghi in cui dovrebbe essere applicato anche un cambiamento dall'autore per mantenere la coerenza complessiva del codice a un livello elevato. Qualsiasi altro problema riscontrato durante la revisione, dovresti entrare nel bug tracker se è abbastanza importante da giustificare il fixing. Non mi dilungherò troppo con gli errori di battitura nei commenti.

What about suggestions of different ways to solve the same problem? Should I suggest a new way only when a case can be made for it being better? How do I draw the line between contributing ideas but not sounding like a know-it-all?

Idealmente, queste cose dovrebbero essere emerse durante la progettazione (revisione). Dopodiché è un atto di bilanciamento e vorrei portarli avanti solo se la vostra soluzione fornisce un miglioramento chiaro e misurabile in modi che sono rilevanti per il successo del progetto. Il saldo qui è tra i vantaggi della soluzione rispetto ai costi di implementazione.

In matters which are purely a difference of opinion, who should get the 'final say'? The author, the person who knows the code base the best, the person with the highest title or someone else?

L'ultima parola su tali questioni dovrebbe riguardare lo standard di codifica. Se lo standard di codifica non regola il problema, è meglio accettare la vista della persona che conosce meglio la base di codice, perché è probabile che mantenga la maggiore coerenza nella base di codice.

Any other advice for ensuring the code reviews serve their purpose without unnecessarily blocking the developer's progress?

Come revisore:

• Cerca di dare priorità alla recensione rispetto ad altre attività

Come autore:

• Seleziona attentamente i revisori. Non dovrebbero essere già sovraccarichi di lavoro e dovrebbero conoscere già l'argomento e la base di codice.
• Durante l'attesa della revisione, verifica se è possibile avviare l'attività successiva senza compromettere il codice attualmente in corso di revisione.

Come sviluppatore junior, non sei in una buona posizione per fare la differenza sull'efficacia delle revisioni del codice del tuo team. Questo è davvero per il leader (s) nel team.

I miei due centesimi sono che è il risultato che conta sulla revisione del codice, non sul processo. Le recensioni che non risultano in una progettazione e un codice sostanzialmente migliori sono una perdita di tempo. La struttura può aiutare. Ma può anche ostacolare se impedisce al team di affrontare i problemi reali.

Quindi ...

1) Should code review comments be limited to actual problems with the code?

È difficile tracciare una linea. Dipende anche dal fatto che si tratti solo di una revisione del codice o anche di un tutoraggio.

2) Are criticisms about other LOC in the file appropriate even if those lines are not being modified? i.e., criticism of pre-existing code?

Dipende. Se è necessario migliorare il codice preesistente e la possibilità di farlo nel contesto del lavoro svolto, potrebbe essere utile parlarne.

3) What about suggestions of different ways to solve the same problem? Should I suggest a new way only when a case can be made for it being better?

Se 1) il modo alternativo darebbe risultati significativamente migliori, 2) sarebbe pratico implementarlo ora, e 3) la persona che farebbe quel lavoro sarà probabilmente ricettiva, quindi potrebbe essere appropriata.

How do I draw the line between contributing ideas but not sounding like a know-it-all?

Usa il buon senso e sii sensibile alle reazioni degli altri.

4) In matters which are purely a difference of opinion, who should get the 'final say'? The author, the person who knows the code base the best, the person with the highest title or someone else?

Questo è compito del team / della direzione decidere.

5) Any other advice for ensuring the code reviews serve their purpose without unnecessarily blocking the developer's progress?

Se le revisioni del codice ostacolano i progressi, devi discuterne con il tuo capo progetto. Il capo squadra porta il barattolo per ritardi di progetto e problemi di qualità.