Quali sono i processi comuni di revisione del codice e cosa è considerato negativo?

16

La mia azienda ha recentemente iniziato a fare revisioni di codice formalizzate. Il processo si svolge in questo modo: ti iscrivi in un github, richiedi una richiesta di pull, il codice viene esaminato da circa tre persone, quindi se tutto passa, il tuo codice entra.

Il processo sembra equo, tuttavia le tre persone che eseguono le revisioni del codice non sembrano corrette. Ho notato che quando inserisco il mio codice per la revisione, ottengo ovunque tra 100-200 commenti. Il numero più alto per me era 300 commenti una volta. Ovviamente penseresti che siano grandi cambiamenti, ma questo può essere anche un piccolo cambiamento con meno di 50 righe di codice (che include i test unitari). Tutti i commenti sono considerati "devono fare" e senza argomenti.

Con questo in mente, il mio problema principale qui è che sembra un po 'eccessivo. Ho parlato con il gruppo e mi hanno detto fondamentalmente che solo perché ho avuto così tanti anni di sviluppo in php non significa che io sia uno "sviluppatore". Ovviamente questo sembra più doloroso del contrario. Inoltre, noto che all'interno del gruppo, non sembrano produrre tanti commenti e il più delle volte ignorano o altrimenti ignorano altri commenti o suggerimenti che raramente accettano come punto valido anche se qualcosa è rotto.

Quindi la mia domanda è se questo è giusto? O comune?

    
posta user1207047 19.12.2013 - 19:49
fonte

8 risposte

15

All comments are considered "must do" and without argument.

IMHO è il vero problema, dal momento che non esiste una priorità in questo. Quando ricevi 100-300 commenti, là devono essere alcuni di loro aventi una priorità A (veri bug), alcuni dei quali prio B (probabilmente portano a bug in seguito) e alcuni di loro prio C ( tutto il resto). Dì ai tuoi colleghi che sei disposto a rispettare tutti i loro desideri, ma per rendere effettive le modifiche, e il tuo tempo è limitato, insistere su una priorizzazione. Quindi, inizia con la correzione di prio Un commento per primo, e se hai davvero tempo per ulteriori dopo, puoi iniziare con B (se sei fortunato, il tuo capo capirà che fissare prio B e C non è così importante e darti alcuni compiti più importanti invece di perdere tempo)

    
risposta data 19.12.2013 - 21:03
fonte
10

Le revisioni del codice possono essere un processo di divisione.

Sei a un incrocio importante, però. Fai un'analisi ponderata sulla loro recensione. Stanno identificando problemi di selezione nit, o evidenziando gravi difetti nel tuo stile e nella logica?

Se è il primo, consiglierei di lavorare per una risoluzione (nuovo lavoro o nuovi processi di revisione del codice).

Se è quest'ultimo, ti consiglio di fare molto codice di lettura e di studio per cercare di portare il tuo codice alla qualità professionale.

    
risposta data 19.12.2013 - 20:18
fonte
5

Sembra dai tuoi commenti che i tuoi colleghi stanno usando il processo di revisione del codice per concordare una metodologia o lucidare il codice. Ho appena iniziato a fare revisioni del codice come te e noto che a volte discutiamo molto di cose che sono solo approcci o miglioramenti all'implementazione. Questo non è affatto male per quanto ragionevole (300 commenti sembrano troppi per me, che devono apparire come un thread di reddit)

Forse è necessario concordare alcune decisioni architetturali sul codice prima di iniziare a implementarlo o forse è solo d'accordo sulla denominazione di convenzioni, schemi e buone pratiche in modo che tutti sappiano cosa è considerato "buon codice".

Se rispetti gli standard del codice come dici tu e il codice funziona come previsto, non ci dovrebbero essere così tanti commenti, quindi o stanno usando il tuo codice come forum o ti stanno trollando come sembra che tu stanno indicando.

Cercherò di essere critico con me stesso, proverò a prendere parte alle conversazioni e vedrò il motivo di tutti questi commenti e magari parlerò con loro di questo in modo costruttivo per capire perché sono così scontenti del tuo codice e se puoi codificare in un modo che rende tutti felici e il lavoro non si blocca nella revisione del codice.

Ho appena letto i tuoi ultimi commenti, a volte quando non sei d'accordo con il codice puoi superarlo cento volte e proporre cambiamenti ovunque che non ti rendono felice perché il vero motivo è che avresti fatto un diversa decisione architettonica e non ti piace quel codice, non importa quante volte lo rifatti. Come ho detto sopra, forse è necessario concordare l'approccio al codice in anticipo, quindi quando lo scrivi sai cosa si aspettano da esso e quindi il tuo codice sarebbe più ragionevole per loro.

    
risposta data 19.12.2013 - 21:35
fonte
4

Da quello che stai dicendo, mi sembra che potrebbero avere un pregiudizio contro gli sviluppatori di php, e quindi stanno cercando di trovare ogni singola cosa che non va nel tuo codice al fine di dimostrare il loro punto.¹

Riguardo alla revisione del codice stesso, credo che, come hai già detto, una quantità così grande di commenti minori sia meno utile di alcune critiche valide e valide. E anche se ho un'esperienza limitata rispetto alle revisioni del codice, la seguente tecnica ha funzionato bene per le squadre in cui ho lavorato in passato.

  • Prima di tutto, è necessario utilizzare un analizzatore di codice statico per identificare la maggior parte dei problemi prima che avvenga la revisione del codice. Ad esempio: eseguire il codice tramite Sonar, Lint o qualsiasi altro buon analizzatore di codice dovrebbe aiutarti a sbarazzarti dei problemi minori. Soprattutto perché i revisori possono definire profili personalizzati per garantire tutto, dalla collocazione delle staffe, agli spazi bianchi, ai commenti, alla denominazione delle variabili appropriate e molto altro ancora ...
  • In secondo luogo, mi sembra che funzioni bene se dividi i commenti in diverse categorie. Ad esempio due categorie in cui un gruppo include piccole cose che dovresti prendere in considerazione e applicare in futuro. E un secondo gruppo per quei commenti che richiedono una modifica immediata del codice che richiederebbe un altro commit e una successiva revisione. Ovviamente, il numero di commenti in quest'ultimo gruppo dovrebbe essere minore.

Inoltre, devo dire che le mie prime recensioni di codice reali contenevano anche più commenti di quanto mi aspettassi inizialmente. Tuttavia non ho mai considerato questa cosa negativa. Se continui a imparare dai loro commenti² e sei disposto ad applicare le nuove tecniche / best practice apprese nelle tue future presentazioni di codice, i commenti dovrebbero diventare meno. È stato sicuramente il caso per me; -)

¹ Nella mia esperienza, questo accade un numero molto elevato di programmatori che affermano che php è il linguaggio di programmazione più malvagio, con i programmatori più inesperti che lo utilizzano. Mi distolgo da questa affermazione poiché credo che un ottimo software possa essere scritto in qualsiasi lingua!

² Supponendo che, sebbene i commenti siano eccessivi, alcuni valori sono in essi

    
risposta data 19.12.2013 - 20:55
fonte
3

È comune per chiunque ricevere più di 100 commenti nelle loro revisioni del codice su una base di routine? Direi di no È comune per le persone la cui qualità del codice "lascia molto a desiderare" per ottenere molti commenti, assolutamente.

Tuttavia, dipende anche dalle "regole" del processo di revisione del codice. TUTTI hanno le loro idee su come qualcosa dovrebbe essere fatto. Se il tuo processo di revisione del codice consente ai commenti di avere il formato "Dovresti farlo in questo modo invece che in quel modo", allora probabilmente otterrai MOLTISSIMI commenti anche per un codice adeguato. Se il tuo processo è destinato a trovare "difetti", il numero di commenti dovrebbe essere molto più piccolo.

Nella mia esperienza, le recensioni che consentono "suggerimenti" per metodi alternativi sono perdite di tempo. Questi "suggerimenti" dovrebbero essere gestiti uno a uno al di fuori del processo di revisione. Le revisioni dei difetti sono più utili in quanto portano le persone a concentrarsi sui bug invece di "perché non l'hai fatto come avrei fatto io?". È anche più utile perché non si può negare un bug se qualcuno lo trova. Quindi, non ci sono sentimenti feriti, ma probabilmente gratitudine.

AGGIORNAMENTO: Con tutto ciò che detto, un certo codice è semplicemente cattivo, anche se privo di difetti. In tal caso, il commento della recensione dovrebbe essere un singolo commento che dice qualcosa di simile. "Questo codice deve essere ripulito. Si prega di posticipare la revisione fino a quando il codice non viene discusso con [il tuo nome qui]". In tal caso, un'ulteriore revisione del codice dovrebbe interrompersi finché il commento non viene corretto.

UPDATE2: @User: discuti il tuo codice / design con uno di essi mentre lo stai sviluppando in modo da poter implementare ciò che stanno cercando prima di arrivare a molto a fare a modo tuo? Stai cambiando qualcosa su come stai sviluppando il codice in base ai loro suggerimenti o continui a pensare che la tua strada stia bene? Stai imparando qualcosa dai loro commenti?

Quando sono il responsabile di un progetto, è mio compito essere responsabile di TUTTI i prodotti di lavoro. Se approvo un prodotto di lavoro, dichiaro che il prodotto è accettabile. Voglio avere una reputazione per la costruzione di prodotti di qualità. Quindi, ho aspettative e non accetto meno che soddisfacente. Allo stesso tempo, cerco di insegnare e spiegare le ragioni delle mie preferenze. Queste preferenze potrebbero non essere sempre ideali (in particolare agli occhi degli altri), ma la maggior parte di queste preferenze deriva dall'esperienza. Di solito una reazione per evitare di ripetere quelle cattive. Pertanto, ci sono alcuni miei "stickler" personali che sono necessari per ottenere la mia approvazione, indipendentemente dal pushback.

Dall'altra parte, devi imparare le aspettative che sono necessarie per ottenere l'approvazione dei tuoi prodotti di lavoro. Non si può essere d'accordo, ma dal momento che non sembra avere l'autorità per sovrastimare, si impara cosa è previsto. Dubito che la squadra stia cercando di farti fallire. Anche questo li fa sembrare cattivi. A tal proposito, dimostra semplicemente che sei desideroso di imparare (anche se non lo sei), prendi ciò che dicono e fai del tuo meglio per adattarsi alle loro preferenze e probabilmente li vedrai arretrare un po '. Forse trova quello che puoi almeno tollerare e vedere se faranno un po 'di tenere in mano per insegnarti i loro modi. Chissà, nel processo potresti imparare qualcosa che potrebbe davvero portare le tue abilità al livello successivo.

    
risposta data 19.12.2013 - 20:51
fonte
2

Alcune importanti differenze con il nostro processo di ispezione del team:

  • la base di un'ispezione è una checklist, compilata da tutto il team.
  • Focus è difetti (presenti e futuri), non stile per amore dello stile.
  • i 3 ispettori (incluso l'autore) si siedono insieme per esaminare i commenti. Restano solo i commenti con voto a maggioranza.
risposta data 20.12.2013 - 07:39
fonte
2

Per 50 commenti LOC 300 sembra essere un po 'eccessivo e - wow - 3 revisori per ogni richiesta pull? La tua azienda deve disporre di molte risorse.

Dalla mia esperienza per un utile processo di revisione del codice ci devono essere alcune regole e / o linee guida:

  • Priorità dei commenti
  • Classificazione dei commenti (Bug, Stile codice, ecc.)
  • Architettura concordata / design da seguire
  • Stile del codice concordato

Se non ricevi una priorità dai revisori chiedi al tuo responsabile di progetto / capogruppo responsabile; la persona responsabile per i costi dovrebbe avere un'opinione sulle priorità.

Se hai un'architettura definita, una comprensione comune di quali modelli di progettazione utilizzi nel tuo progetto e uno stile di codice concordato, i commenti di revisione dovrebbero riguardare solo "problemi reali" come problemi di sicurezza, bug non intenzionali, casi angolari non coperti dall'architettura specificata, ecc.

Se il tuo team di sviluppo non ha concordato "problemi di gusto" (es. se una variabile membro inizia con "m_"), allora ogni recensore ti costringerà a seguire il suo stile, che è solo uno spreco di tempo / denaro.

    
risposta data 20.12.2013 - 08:40
fonte
1

Questo mi sembra davvero un problema di comunicazione. Ti aspetti che il tuo codice non sia abbastanza cattivo da meritare 300 commenti. I revisori sembrano ritenere che tu abbia bisogno di molti feedback. Discutere avanti e indietro in modo asincrono sta per perdere un sacco di tempo. Diamine, scrivere 300 commenti è una perdita di tempo TREMENDA. Se questi non sono tutti difetti allora è possibile come nuovo membro del team che non conosci ancora lo stile del team. È normale e qualcosa che dovrebbe essere imparato per accelerare l'intera squadra.

Il mio suggerimento è di risparmiare tempo. Accelerare il feedback. Vorrei:

  • Esegui più recensioni intermedie per evitare di commettere lo stesso errore e generare lo stesso commento 50 volte
  • Siediti con un revisore mentre rivedono il tuo codice in modo che tu possa parlare dei problemi che emergono, evitando così di documentare 300 "problemi" che verranno cancellati nel prossimo commit
  • Accoppia per un po 'con uno di questi "veri" sviluppatori mentre scrivi il codice per vedere cosa farebbero diversamente

Le persone possono discutere contro l'accoppiamento perché "ci vorrà più tempo", ma ovviamente non è un problema qui.

    
risposta data 20.12.2013 - 18:39
fonte

Leggi altre domande sui tag