Che cosa fare quando il codice inviato per la revisione del codice sembra essere troppo complicato?

Se non può essere esaminato, non può superare la revisione.

Devi capire che la revisione del codice non è per trovare bug. Ecco a cosa serve il QA. La revisione del codice garantisce che sia possibile la manutenzione futura del codice. Se non riesci nemmeno a seguire il codice ora, come puoi farlo in sei mesi quando ti vengono assegnati miglioramenti alle funzionalità e / o correzioni di errori? Trovare bug in questo momento è solo un vantaggio collaterale.

Se è troppo complesso, sta violando un sacco di principi SOLID . Refactor, refactor, refactor. Scomposizione in funzioni correttamente denominate che fanno molto meno, più semplice. Puoi pulirlo e le tue test case faranno in modo che continui a funzionare correttamente. Hai casi di test, giusto? Altrimenti, dovresti iniziare ad aggiungerli.

Tutto ciò che menzioni è perfettamente valido per farti notare in una revisione del codice.

Quando ricevo una recensione del codice, rivedo i test. Se i test non forniscono una copertura sufficiente, è qualcosa da sottolineare. I test devono essere utili per garantire che il codice funzioni come previsto e continui a funzionare come previsto nelle modifiche. In effetti, questa è una delle prime cose che cerco in una revisione del codice. Se non hai dimostrato che il tuo codice soddisfa i requisiti, non voglio investire il mio tempo a guardarlo.

Una volta che ci sono test sufficienti per il codice, se il codice è complesso o difficile da seguire, è anche qualcosa che gli umani dovrebbero considerare. Gli strumenti di analisi statica possono indicare alcune misure di complessità e segnalare i metodi eccessivamente complessi, oltre a individuare potenziali difetti nel codice (e devono essere eseguiti prima di una revisione del codice umano). Ma il codice viene letto e gestito dagli esseri umani e deve essere scritto prima per la manutenibilità. Solo se c'è un motivo per utilizzare un codice meno gestibile dovrebbe essere scritto in quel modo. Se è necessario disporre di codice complesso o non intuitivo, dovrebbe essere documentato (preferibilmente nel codice) perché il codice è così e avere commenti utili per gli sviluppatori futuri per capire perché e cosa sta facendo il codice.

Idealmente, respingere le revisioni del codice che non hanno test appropriati o hanno un codice troppo complesso senza una buona ragione. Potrebbero esserci motivi di lavoro per andare avanti e per questo è necessario valutare i rischi. Se vai avanti con il debito tecnico in codice, inserisci subito i ticket nel tuo sistema di tracciamento dei bug con alcuni dettagli su ciò che deve cambiare e alcuni suggerimenti per cambiarlo.

Verifying overall correctness manually via code review however is very difficult and time-consuming, if it is even possible at all.

Questo non è da meno il punto di una revisione del codice. Il modo di pensare a una revisione del codice è immaginare che ci sia un bug nel codice e devi risolverlo. Con questa mentalità, sfoglia il codice (in particolare i commenti) e chiediti "È facile capire il quadro generale di ciò che sta accadendo in modo da poter restringere il problema?" Se è così, è un passaggio. Altrimenti, è un fallimento. È necessaria una maggiore documentazione, o forse è necessario un refactoring per rendere il codice ragionevolmente comprensibile.

È importante non essere perfezionisti a questo riguardo, a meno che tu non sia sicuro che questo è ciò che il tuo datore di lavoro cerca. La maggior parte del codice fa schifo a tal punto che potrebbe essere facilmente rifatto per 10 volte di seguito, diventando ogni volta più leggibile. Ma il tuo datore di lavoro probabilmente non vuole pagare per avere il codice più leggibile al mondo.

Verifying overall correctness manually via code review however is very difficult and time-consuming, if it is even possible at all.

Molti anni fa, in realtà, il mio lavoro era esattamente quello di valutare i compiti degli studenti. E mentre molti hanno consegnato una qualità ragionevole con un bug qua e là, ce ne sono stati due che si sono distinti. Entrambi hanno sempre inviato il codice senza errori. Un codice inviato che potrei leggere dall'alto e dal basso ad alta velocità e contrassegnato come corretto al 100% con zero sforzi. L'altro codice inviato era un WTF dopo l'altro, ma in qualche modo è riuscito a evitare eventuali bug. Un dolore assoluto da segnare.

Oggi il secondo avrebbe rifiutato il suo codice in una revisione del codice. Se verificare la correttezza è molto difficile e richiede molto tempo, allora questo è un problema con il codice. Un programmatore decente potrebbe capire come risolvere un problema (richiede tempo X) e prima di assegnarlo a un refact del codice in modo che non svolga semplicemente il lavoro, ma ovviamente fa il lavoro. Ciò richiede molto meno di X in tempo e consente di risparmiare molto tempo in futuro. Spesso scoprendo bug prima ancora di arrivare allo stadio di una revisione del codice. Successivamente, rendendo la revisione del codice molto più veloce. E tutto il tempo in futuro, rendendo il codice più facile da adattare.

Un'altra risposta ha detto che il codice di alcune persone potrebbe essere rifattorizzato 10 volte, diventando ogni volta più leggibile. È solo triste. Quello è uno sviluppatore che dovrebbe cercare un lavoro diverso.

Questo vecchio codice è stato leggermente modificato? (100 linee di codice modificate in un codebase di 10000 linee è ancora un piccolo cambiamento) A volte ci sono vincoli di tempo e gli sviluppatori sono costretti a rimanere all'interno di un quadro vecchio e scomodo, semplicemente perché una riscrittura completa richiederebbe ancora più tempo e è fuori bilancio . + Di solito c'è il rischio, che può costare milioni di dollari se valutato erroneamente. Se è un vecchio codice, nella maggior parte dei casi dovrai conviverci. Se non lo capisci da solo, parla con loro e ascolta ciò che dicono, cerca di capire. Ricorda, potrebbe essere difficile da seguire per te, ma perfettamente bene per le altre persone. Prendi la loro parte, vedila dalla loro estremità.

Questo nuovo codice ? A seconda dei limiti di tempo, dovresti sostenere il refactoring il più possibile. Va bene dedicare più tempo alle revisioni del codice, se necessario. Non dovresti timebox a 15 minuti, ottenere l'idea e andare avanti. Se l'autore ha trascorso una settimana a scrivere qualcosa, va bene dedicare 4-8 ore per esaminarlo. Il tuo obiettivo qui è di aiutarli a refactoring. Non si restituisce semplicemente il codice che dice "refactor. Now". Scopri quali metodi possono essere suddivisi, prova a proporre idee per introdurre nuove classi ecc.

Spesso, le patch / changelists "complicate" sono quelle che fanno molte cose diverse contemporaneamente. C'è un nuovo codice, codice cancellato, codice refactored, codice spostato, test espansi; rende difficile vedere il quadro generale.

Un indizio comune è che la patch è enorme ma la sua descrizione è minuscola: "Implementa $ FOO."

Un modo ragionevole per gestire una tale patch è chiedergli di essere suddiviso in una serie di pezzi più piccoli e autonomi. Proprio come il principio di responsabilità singola dice che una funzione dovrebbe fare solo una cosa, una patch dovrebbe concentrarsi anche su una sola cosa.

Ad esempio, le prime patch potrebbero contenere refactoring puramente meccanici che non apportano modifiche funzionali, e quindi le patch finali possono focalizzarsi sull'effettiva implementazione e test di $ FOO con meno distrazioni e false falsificazioni.

Per funzionalità che richiedono un sacco di nuovo codice, il nuovo codice può spesso essere introdotto in blocchi testabili che non modificano il comportamento del prodotto fino a quando l'ultima patch della serie non chiama effettivamente il nuovo codice (un flag flip).

Per quanto mi riguarda con tatto, di solito lo esprimo come mio problema e poi chiedo l'aiuto dell'autore: "Sto avendo problemi a seguire tutto quello che succede qui. Potresti rompere questa patch in piccoli passi per aiutarmi a capire come tutto questo combacia? " Talvolta è necessario fornire suggerimenti specifici per i passaggi più piccoli.

Una patch così grande come "Implement $ FOO" si trasforma in una serie di patch come:

1. Introduci una nuova versione di Frobnicate che accetta un paio di iteratori perché dovrò chiamarlo con sequenze diverse dal vettore per implementare $ FOO.
2. Cambia tutti i chiamanti esistenti di Frobnicate per utilizzare la nuova versione.
3. Elimina il vecchio Frobnicate.
4. Frobnicate stava facendo troppo. Calcola il passaggio rifleto nel proprio metodo e aggiungi test per questo.
5. Introduci Zerzify, con i test. Non ancora usato, ma ne avrò bisogno per $ FOO.
6. Implementa $ FOO in termini di Zerzify e del nuovo Frobnicate.

Si noti che i passaggi 1-5 non apportano modifiche funzionali al prodotto. Sono banali da recensire, compreso il fatto di avere tutti i test giusti. Anche se il passaggio 6 è ancora "complicato", almeno è focalizzato su $ FOO. E il log ti dà un'idea migliore di come è stato implementato $ FOO (e perché Frobnicate è stato modificato).

Come altri hanno sottolineato, la revisione del codice non è realmente progettata per trovare bug. Se stai riscontrando bug durante la revisione del codice che probabilmente significa che non hai abbastanza copertura di test automatizzata (ad esempio test di unità / integrazione). Se la copertura non è sufficiente per convincermi che il codice fa ciò che dovrebbe , di solito chiedo più test e indico il tipo di casi di test che sto cercando e di solito non autorizzo il codice codebase che non ha una copertura adeguata.

Se l'architettura di alto livello è troppo complessa o non ha senso, di solito chiedo un incontro con un paio di membri del team per parlarne. A volte è difficile ripetere su una cattiva architettura. Se lo sviluppatore era un principiante, di solito mi assicuro che passiamo attraverso ciò che il loro pensiero è avanti di tempo invece di reagire a una cattiva richiesta di pull. Questo di solito è vero anche con sviluppatori più esperti se il problema non ha una soluzione ovvia che sarà più che probabile.

Se la complessità è isolata al livello del metodo che di solito può essere corretta iterativamente e con buoni test automatici.

Un ultimo punto. Come revisore devi decidere se la complessità del codice è dovuta a essenziale o accidentale complessità . La complessità essenziale riguarda le parti del software che sono legittimamente difficili da risolvere. La complessità accidentale si riferisce a tutte le altre parti del codice che scriviamo che è troppo complessa senza motivo e potrebbe essere facilmente semplificata.

Di solito mi assicuro che quel codice con una complessità essenziale sia proprio questo e non possa essere ulteriormente semplificato. Mirando anche a più copertura di test e buona documentazione per queste parti. La complessità accidentale dovrebbe quasi sempre essere ripulita durante il processo di richiesta di pull perché quelli sono la maggior parte del codice che trattiamo e possono facilmente causare incubo di manutenzione anche a breve termine.

Come sono le prove? Dovrebbero essere chiari, semplici e facili da leggere con idealmente una sola affermazione. I test devono documentare chiaramente il comportamento previsto e casi d'uso del codice.

Se non è ben testato, è un buon punto di partenza per la revisione.