Topic: Code reviews - come, perche', quando ...  (Letto 477 volte)

0 Utenti e 1 Visitatore stanno visualizzando questo topic.

Offline Markon

  • python sapiens sapiens
  • *
  • moderatore
  • Post: 4.104
  • Punti reputazione: 5
    • Mostra profilo
    • Neolithic
Code reviews - come, perche', quando ...
« il: Luglio 30, 2018, 20:47 »
Ciao!

Mi farebbe piacere sapere come applicate code reviews da voi. Ogni azienda/team le fa in modo diverso, per alcuni si tratta di "spiegare" il diff tra il nuovo codice e il vecchio - con live sessions per ogni feature. Alcuni team possono essere sono piu' puntigliosi e vanno a cercare le virgole fuori posto.

Voi cosa cercate in una code review? Cosa fate? E soprattutto, come date feedback ai vostri colleghi (e non)?

Io generalmente guardo al codice, cerco di capire cosa fa (magari testo sulla mia macchina), pongo domande piu' che affermazioni, e a meno che non ci siano vistosi errori di programmazione, solitamente non chiedo granche'. Usiamo tool automatici per prevenire errori di formatting e altro, cosi' che le code reviews si soffermino di piu' sul codice che sulle virgole fuori posto. Inoltre, verifico che i test abbiano senso e non siano ripetitivi, ecc. Ah, spero che non facciate review di design nelle code reviews ... :)

E voi?
« Ultima modifica: Luglio 30, 2018, 20:51 da Markon »

Offline GlennHK

  • python sapiens sapiens
  • ******
  • Post: 1.652
  • Punti reputazione: 1
    • Mostra profilo
    • La Tana di GlennHK
Re:Code reviews - come, perche', quando ...
« Risposta #1 il: Agosto 02, 2018, 00:51 »
Per quanto piccolo sia il team che gestisco io, la code review in genere è una fase in cui io do un'occhiata al codice scritto da altri, e preparo una serie di domande su:

  • razionale dietro le scelte architetturali
  • nomenclatura (metodi, variabili, ecc)
  • efficienza del codice scritto
  • edge-cases non gestiti
Il tutto purtroppo in tempi abbastanza stretti perché non c'è una grande finestra (ahimé) per la code review.

Offline Markon

  • python sapiens sapiens
  • *
  • moderatore
  • Post: 4.104
  • Punti reputazione: 5
    • Mostra profilo
    • Neolithic
Re:Code reviews - come, perche', quando ...
« Risposta #2 il: Agosto 02, 2018, 08:44 »
@GlennHK

Quando dici "razionale dietro le scelte architetturale" , a cosa ti riferisci?

Offline GlennHK

  • python sapiens sapiens
  • ******
  • Post: 1.652
  • Punti reputazione: 1
    • Mostra profilo
    • La Tana di GlennHK
Re:Code reviews - come, perche', quando ...
« Risposta #3 il: Agosto 02, 2018, 14:24 »
@GlennHK

Quando dici "razionale dietro le scelte architetturale" , a cosa ti riferisci?


Al perché è stato utilizzato un tipo di struttura dati, o al perché sono state creati dei metodi piuttosto che altri, eccetera.

Offline Markon

  • python sapiens sapiens
  • *
  • moderatore
  • Post: 4.104
  • Punti reputazione: 5
    • Mostra profilo
    • Neolithic
Re:Code reviews - come, perche', quando ...
« Risposta #4 il: Agosto 07, 2018, 19:13 »
@Glenn
Ok, e quali sono le tue impressioni? Funziona? E chi fa questo genere di review? Tutti i team members o solo una - due persone?

Offline GlennHK

  • python sapiens sapiens
  • ******
  • Post: 1.652
  • Punti reputazione: 1
    • Mostra profilo
    • La Tana di GlennHK
Re:Code reviews - come, perche', quando ...
« Risposta #5 il: Agosto 08, 2018, 12:38 »
@Glenn
Ok, e quali sono le tue impressioni? Funziona? E chi fa questo genere di review? Tutti i team members o solo una - due persone?


In genere la faccio io, e tutto sommato funziona. Però potrebbe essere anche un falso positivo, perché è influenzato dalle skill dei membri del team.

Offline Markon

  • python sapiens sapiens
  • *
  • moderatore
  • Post: 4.104
  • Punti reputazione: 5
    • Mostra profilo
    • Neolithic
Re:Code reviews - come, perche', quando ...
« Risposta #6 il: Agosto 08, 2018, 17:04 »
Ok! Grazie per la risposta.

Qui da noi generalmente attendiamo che 1-2 persone facciano la review del codice prima che venga fatto il merge. Tra l'altro non e' proibito fare il review del codice di altri team.
Troviamo importante questo knowledge transfer cosi' da rendere un pochino tutti owners del codice e per far si' che tutti (o quasi) imparino.

Offline riko

  • python deus
  • *
  • moderatore
  • Post: 7.453
  • Punti reputazione: 12
    • Mostra profilo
    • RiK0 Tech Temple
Re:Code reviews - come, perche', quando ...
« Risposta #7 il: Agosto 11, 2018, 09:19 »
Che dire... mi sa che devo andare contro corrente. ;)

Personalmente trovo che le code review siano davvero efficaci solo nel caso in cui in qualche modo una persona "esterna" vuole contribuire ad un progetto.
E nota che, in questo caso, sono proprio code review, ma non davvero peer review. In un qualche modo la persona che fa la review *non* e' un peer (per lo meno, non nel contesto del progetto).

Quando si lavora tutti sulla stessa code base... come dire. Le code review fanno venire fuori dinamiche interessanti. Parliamo di fare design in fase code review? Cosa e' questo? Questo e' un abuso di fiducia. Le scelte di design che faccio, le prendo io. Punto. Se e' una scelta sufficientemente grossa da coinvolgere il team, deve essere una decisione di team (o di un sotto-gruppo del team). Fare design in code review vuole dire che un ingegnere a caso (il primo che arriva sulla cr) puo' abusare il fatto di essere in posizione di dovere "approvare" per influenzare decisioni che non dovrebbbe influenzare. Nel senso che se potesse influenzare il design con altri canali, non userebbe la code review, no?

Poi bisognerebbe capire cosa si intende design. Sebbene io venda sempre il fatto che "fai design ogni volta che scrivi un metodo", adesso... l'architettura e' una cosa interessante, poi sai se c'e' TMP o Strategy... come dire, i progetti non falliscono per quello, il software non e' che smette di funzionare per quello. Si tratta piu' che altro di essere un po' piu' o meno comodo da mantenere. Ora, se ci si mette *davvero* a parlare di architettura in CR, forse qualcosa a monte e' andato storto.

Il secondo peccato mortale delle CR (che secondo me e' il primo), e' il bike-shedding. Questo fattore toglie da solo ogni potenziale utilita' alla CR quando si manifesta. Ci dovrebbe essere un comitato che in presenza di bike-shedding da ship-it blanket e mette direttamente tutto in produzione e la prossima volta fai le cose seriamente ca**o.

In generale sono uno strumento che non possiamo davvero togliere, perche' non abbiamo alternativa. Ma a seconda dei contesti possono essere completamente inutili. Oggettivamente credo di avere evitato tanti problemi facendo CR agli altri. Indietro di solito prendo "hey, hai invertito due lettere nel commento del test su quella parte di codice che hai detto che deprecherai nella prossima cr". Gia'... cosi' critico...

Ma davvero non puoi toglierle.

Ah... commenti? Se cominci a fare "classi" dove c'e' chi le CR le fa e chi le riceve, crei un botto di friction nel team. Non solo, vai a fare overload della gente piu' senior (che sarebbe il caso che si interessasse di cose piu' rilevanti) e togli responsabilita' a chi sta venendo su. Brutta brutta immagine. Io ti confesso che da un team del genere me ne andrei il giorno stesso. Potrebbe essere diverso nel caso in cui si stanno introducendo, per cui al momento ci sono davvero 2 persone che sanno come funzionano. Ma oggettivamente... se sentissi che si sta "parlando di introdurre le CR nel 2018", forse scapperei a gambe levate dalla compagnia.

> Al perché è stato utilizzato un tipo di struttura dati, o al perché sono state creati dei metodi piuttosto che altri, eccetera.

Su questo sono parzialmente in disaccordo. Usare una struttura dati invece che un altra e' chiaramente una scelta implementativa, non di design.
Ovviamente ci sono edge cases, ma in generale e' una scelta implementativa. Allo stesso modo, suggerire un refactoring in un certo senso o qualcosa di simile non e' una scelta di design.

No, mettersi a scrivere lavagnate di UML per accordarsi e poi fare il codice (che evita il problema di discutere i metodi in CR) non e' time-effective.

> Il tutto purtroppo in tempi abbastanza stretti perché non c'è una grande finestra (ahimé) per la code review.

Non vuoi un processo lungo. Deve essere abbastanza per arrivare ad essere "piu' o meno ok".  I dettagli si possono aggiustare sempre in seguito.

> Io generalmente guardo al codice, cerco di capire cosa fa (magari testo sulla mia macchina), pongo domande piu' che affermazioni, e a meno che non ci siano vistosi errori di programmazione, solitamente non chiedo granche'.

Capisco il concetto di fare domande, spesso mi capita. Ma tento di limitarlo. Generalemnte se sento che debbo fare troppe domande, vuole dire che non sono la persona giusta per fare la CR perche' non ho abbastanza contesto. Una piccola quantita' di domande e' normale.

Io *traccio* le CR perche' l'expectation e' che la maggior parte delle CR passino al primo colpo, con nessun o pochi commenti di poca importanza. Se non si e' qui, bisogna arrivarci. E' importante arrivare al punto in cui "normalmente" quando si manda il codice per review questo passi e basta. Se non succede (frequentemente) bisogna cercare di capire quale e' il problema.

Offline Markon

  • python sapiens sapiens
  • *
  • moderatore
  • Post: 4.104
  • Punti reputazione: 5
    • Mostra profilo
    • Neolithic
Re:Code reviews - come, perche', quando ...
« Risposta #8 il: Agosto 11, 2018, 17:01 »
>Capisco il concetto di fare domande, spesso mi capita. Ma tento di limitarlo. Generalemnte se sento che debbo fare troppe domande, vuole dire che non sono la persona giusta per fare la CR perche' non ho abbastanza contesto. Una piccola quantita' di domande e' normale.

Siamo d'accordo. A volte pero' mi capita di fare domande per evitare un linguaggio piu' assertivo, e ho notato che su certe persone funziona meglio che su altre: alcuni preferiscono un linguaggio piu' diretto e chiaro, altri no, per cui, va bene cosi'.

Ad esempio: "Vedo che hai fatto il refactoring di questo metodo - il che e' positivo. Pero' noto che ora il nuovo codice non gestisce questo use case (per il quale non ci sono unit/integration tests). C'e' un motivo dietro?". Con certe persone trovo molto piu' efficace far riflettere piuttosto che spiegare cosa fare. Magari qualcun altro preferirebbe una cosa del tipo "Ora questo codice non gestisce piu' questo use case, per favore risolvi". Altre volte pongo domande per capire semplicemente cosa si sta cercando di fare. Altre volte semplici affermazioni, anche per dire semplicemente "ben fatto" o "buona idea". Qui si tratta piu' di "come criticare/dare feedback" che altro...

> Io *traccio* le CR perche' l'expectation e' che la maggior parte delle CR passino al primo colpo, con nessun o pochi commenti di poca importanza. Se non si e' qui, bisogna arrivarci. E' importante arrivare al punto in cui "normalmente" quando si manda il codice per review questo passi e basta. Se non succede (frequentemente) bisogna cercare di capire quale e' il problema.

Gia'! Con il tempo mi sono accorto che fare code reviews (o anche peer reviews) non e' cosa banale. Non basta semplicemente assegnare il task al collega e aspettare. E' davvero un fatto culturale, di processo, di feedback, ecc.

Offline riko

  • python deus
  • *
  • moderatore
  • Post: 7.453
  • Punti reputazione: 12
    • Mostra profilo
    • RiK0 Tech Temple
Re:Code reviews - come, perche', quando ...
« Risposta #9 il: Agosto 11, 2018, 21:40 »
> Siamo d'accordo. A volte pero' mi capita di fare domande per evitare un linguaggio piu' assertivo, e ho notato che su certe persone funziona meglio che su altre: alcuni preferiscono un linguaggio piu' diretto e chiaro, altri no, per cui, va bene cosi'.

Si, certo. A me spesso capita di fare domande perche' ci sono cose che puzzano. Il che vuole dire che manca un commento sul *perche'*, oppure c'e' effettivamente un errore.
Quello intendo: se devo farlo "troppo" (nella stessa CR), vuole dire che non ho abbastanza contesto e non dovrei essere io a fare la review (oppure l'altro non ha abbastanza contesto e non dovrebbe fare la modifica). Se invece capita un paio di volte c'e' che sia solo una questione che a quello che fa la modifica sono ovvie cose che non sono ovvie a chi quel codice lo mantiene.

Riguardo allo stile, sono sempre terso. Dai, ci leggiamo da anni... sai come scrivo. :)
Ovviamente e' piu' facile quando si puo' essere precisi. Please use strategy. Terso, rapido, non ambiguo, non te la puoi neanche prendere.
Sappiamo *tutti* che spesso si prendono scorciatoie. E' anche la filosofia di Go. L'idea e' che se il codice e' chiaro e sul pezzo e' piu' facile fare refactoring se e quando si deve piuttosto che (sovra)ingegnerizzare ogni manopola per rendere tutto configurabile. E' il compito del reviewer, a mente fredda, considerare che le scorciatoie non devono essere troppe e non devono essere miopi.

Offline Markon

  • python sapiens sapiens
  • *
  • moderatore
  • Post: 4.104
  • Punti reputazione: 5
    • Mostra profilo
    • Neolithic
Re:Code reviews - come, perche', quando ...
« Risposta #10 il: Agosto 12, 2018, 15:06 »
Siamo d'accordo  :ok:

E ora la domanda ... come sei arrivato a stabilire che quello e' il modo "giusto" di fare code reviews? Perche' davvero, tante aziende le fanno in modo diverso - e non e' detto che lo facciano in modo sbagliato...

Offline riko

  • python deus
  • *
  • moderatore
  • Post: 7.453
  • Punti reputazione: 12
    • Mostra profilo
    • RiK0 Tech Temple
Re:Code reviews - come, perche', quando ...
« Risposta #11 il: Agosto 14, 2018, 21:26 »
Scusa, ma non capisco. E' tutto piuttosto razionale. Vorrei capire quali cose ti sembrano anomale. Tra l'altro io ti dico come *io* faccio code review. Non tutti usano lo stesso stile. Ma non ho capito se stavi parlando del processo di code review nell'insieme o nel day to day della code review.
Anche perche' tipicamente non e' che le CR siano il punto focale della baracca. Sono uno dei tanti strumenti per mantenere la code quality.
In molti casi anche auto-organizzarsi in un modo che funziona, appunto funziona. Se ci sono problemi poi uno degli strumenti puo' essere fare tuning del processo di code review.

Offline Markon

  • python sapiens sapiens
  • *
  • moderatore
  • Post: 4.104
  • Punti reputazione: 5
    • Mostra profilo
    • Neolithic
Re:Code reviews - come, perche', quando ...
« Risposta #12 il: Agosto 15, 2018, 08:05 »
Ciao! Nessuna delle cose citate mi sembra anomala.

Volevo solo capire come sei arrivato a stabilire che quello è il modo giusto per te di fare CR.

Nel mio caso, ad esempio, mi sono accorto (con esperienza, purtroppo) che focalizzarsi su errori di punteggiatura, spazi, import non in ordine, è una perdita di tempo che può essere risolta con tools che fanno formatting automatico. Se non ci fossero, bisognerebbe crearli o purtroppo segnalarli nella review. Ma come arrivare a dire che non fare il design di architettura è sbagliato?
Nel mio caso, mi sono accorto che reca perdite di tempo, al di là dell'abuso di fiducia di cui parlavi. Così preferisco un semplice design document in cui discutiamo cosa va cambiato ecc, così che la CR si focalizzi sul codice piuttosto che su altro. Insomma, l'idea è quella di avere feedback al più presto, dato che come hai detto tu, la CR dovrebbe passare al 90% senza grossi commenti o refactoring.

Spero ora di essere stato più chiaro. È solo per capire come si sia arrivati a "io le faccio così". Magari perché uno ha letto che quello è il modo giusto di fare, non saprei... Anche per capire quali siano i vantaggi e svantaggi che offre rispetto ad altri metodi.
« Ultima modifica: Agosto 15, 2018, 08:07 da Markon »

Offline Markon

  • python sapiens sapiens
  • *
  • moderatore
  • Post: 4.104
  • Punti reputazione: 5
    • Mostra profilo
    • Neolithic
Re:Code reviews - come, perche', quando ...
« Risposta #13 il: Agosto 15, 2018, 23:12 »