Review del fix

In parole semplici

La code review di un fix non è “dare l’approvazione”. È una verifica indipendente che il fix faccia ciò che dichiara, niente di più, niente di meno. Una buona review usa una checklist breve e rifiuta la tentazione di “approvare per amicizia” o “bocciare per stile personale”.

Parole nuove

• review: lettura critica delle modifiche da parte di un’altra persona.
• approve / request changes / comment: i tre verdetti possibili.
• nit (nitpick): nota minore, non bloccante.
• side effect: effetto collaterale del fix non dichiarato.

Cosa impari

• Applicare una checklist di review canonica.
• Distinguere blocchi tecnici da preferenze stilistiche.
• Scrivere commenti di review costruttivi.

La checklist canonica del review di fix

[ ] Il fix corregge davvero il bug descritto?
[ ] Il diff è limitato al fix (no refactor opportunistici)?
[ ] C'è un test di regressione che fallisce senza il fix?
[ ] Il test passa con il fix?
[ ] Sono stati considerati casi vicini e di confine?
[ ] Niente regressioni evidenti su funzionalità correlate?
[ ] Il commit message spiega cosa/perché/come verificato?
[ ] Niente codice morto, console.log, debugger lasciati?
[ ] Niente dati sensibili o segreti committati?
[ ] La retro-compatibilità è preservata (o BREAKING CHANGE dichiarato)?

Dieci punti. Ogni “no” è un commento. Ogni “sì” è verificato.

Tre verdetti possibili

Approve

Tutti i punti passano. Eventuali “nit” sono note, non blocchi.

Comment

Domande o suggerimenti che non bloccano: vuoi chiarimenti, proponi alternative, segnali rischi senza fermare.

Request changes

Almeno un punto della checklist non passa: blocchi il merge finché non viene risolto.

Regola di tono: chiedi i cambiamenti su fatti, non su gusti.

Distinguere blocco e preferenza

Esempio bloccante:

Il test non fallisce senza il fix: aggiungi una asserzione sul caso percentuale = null che fallisca pre-fix.

Esempio non bloccante (nit):

Nit: preferirei if (!percentuale) a if (percentuale === null) per coprire anche undefined. Non bloccante.

Mescolare i due rovina la review: ogni nit suona come blocco, ogni blocco rischia di sembrare gusto.

Due fix da revisionare (esercizio del modulo)

Fix A: minimo ma incompleto

 function calcolaSconto(prezzo, percentuale) {
-  return prezzo - (prezzo * percentuale / 100);
+  if (percentuale === null) return prezzo;
+  return prezzo - (prezzo * percentuale / 100);
 }

Commenti possibili:

• ✅ Diff minimo, ottimo.
• ⚠️ Casi vicini: undefined, "", "abc", NaN non coperti. Estendere a !percentuale && percentuale !== 0? (oppure validazione esplicita).
• ⚠️ Manca test che fallisca pre-fix.

Fix B: largo e rischioso

-function calcolaSconto(prezzo, percentuale) {
-  return prezzo - (prezzo * percentuale / 100);
+function calcolaSconto(originalPrice, discountPercent = 0) {
+  if (typeof originalPrice !== "number" || originalPrice <= 0) return 0;
+  if (typeof discountPercent !== "number" || isNaN(discountPercent)) discountPercent = 0;
+  const clampedPercent = Math.max(0, Math.min(100, discountPercent));
+  const discount = (originalPrice * clampedPercent) / 100;
+  return originalPrice - discount;
 }

Commenti possibili:

• ⚠️ Rinomina parametri: rompe ogni chiamante. PR ha aggiornato i call site? Verificare con grep.
• ⚠️ Cambio comportamento: per prezzo = 0 ora ritorna 0 invece del calcolo (era già 0, ma per prezzo < 0 ora ritorna 0 invece di un valore negativo — è un cambio).
• ⚠️ Clamping di percentuale: introdotto senza richiesta. È fix, refactor o nuova feature?
• ⚠️ Diff misto: fix + refactor + nuova validazione in un commit. Splittare.

Come scrivere commenti utili

• Specifici: cita riga e file.
• Costruttivi: proponi alternativa quando bocci.
• Brevi: una frase, due se serve esempio.
• Educati: “questo cambio” non “questa boiata”.
• Distinti: prefisso nit: per non bloccanti, blocking: per blocchi.

Esempio buono:

sconto.js:5 — blocking: il fix non gestisce undefined, che è probabilmente il caso più frequente in produzione. Vedi test calcolaSconto-undefined nel mio commit di esempio.

Esempio cattivo:

“Brutto.”

Self-review prima di chiedere la review

Prima di marcare PR come pronta:

1. Guarda il diff come se non fosse tuo.
2. Esegui la checklist canonica su te stesso.
3. Sistema ciò che troveresti se fossi revisore.
4. Solo dopo, chiedi review.

Questo solo passaggio fa risparmiare un giro di review a tutto il team.

Esercizio guidato

Trova due PR aperte in un progetto open source di tuo interesse.

1. Per ognuna, applica la checklist canonica.
2. Scrivi (sul tuo taccuino, non sul progetto altrui) almeno tre commenti: uno bloccante, uno non bloccante, uno costruttivo.
3. Confronta con i commenti reali della PR. In cosa convergi/divergi?

Errori frequenti del principiante

• Approvare per fiducia: senza leggere il diff.
• Bocciare per stile personale: “io l’avrei fatto così”.
• Lasciare commenti senza file:riga: chi corregge non sa dove guardare.
• Trattare nit e blocchi alla pari: nit accumulati diventano blocco.