Share
Nella nostra lezione precedente abbiamo imparato un nuovo modo per capire e rendere il codice migliore estraendo fino alla caduta. Mentre quel tutorial era un buon modo per imparare le tecniche, non era certo l'esempio ideale per comprenderne i benefici. In questa lezione estrarremo fino a farci cadere su tutto il nostro codice trivia relativo al gioco e analizzeremo il risultato finale.
Questa lezione concluderà anche la nostra serie sul refactoring. Se pensi di aver perso qualcosa, sentiti libero di commentare con un argomento proposto. Se ci sono buone idee, continuerò con tutorial extra basati sulle tue richieste.
Quale modo migliore per iniziare il nostro articolo piuttosto che prendere il nostro metodo più lungo ed estrarlo in piccoli pezzi. Testare prima, come al solito, renderà questa procedura non solo efficiente ma anche divertente.
Come al solito, hai il codice com'era quando ho iniziato questo tutorial nel php_start cartella, mentre il risultato finale è nel php cartella.
function wasCorrectlyAnswered () if ($ this-> inPenaltyBox [$ this-> currentPlayer]) if ($ this-> isGettingOutOfPenaltyBox) $ this-> display-> correctAnswer (); $ This-> borse [$ this-> currentPlayer] ++; $ this-> display-> playerCoins ($ this-> players [$ this-> currentPlayer], $ this-> purses [$ this-> currentPlayer]); $ winner = $ this-> didPlayerNotWin (); $ This-> currentPlayer ++; if ($ this-> shouldResetCurrentPlayer ()) $ this-> currentPlayer = 0; return $ winner; else $ this-> currentPlayer ++; if ($ this-> shouldResetCurrentPlayer ()) $ this-> currentPlayer = 0; return true; else $ this-> display-> correctAnswerWithTypo (); $ This-> borse [$ this-> currentPlayer] ++; $ this-> display-> playerCoins ($ this-> players [$ this-> currentPlayer], $ this-> purses [$ this-> currentPlayer]); $ winner = $ this-> didPlayerNotWin (); $ This-> currentPlayer ++; if ($ this-> shouldResetCurrentPlayer ()) $ this-> currentPlayer = 0; return $ winner;
Questo metodo, wasCorrectlyAnswered (), è la nostra prima vittima.
Come abbiamo appreso dalle lezioni precedenti, il primo passo quando si modifica il codice legacy è quello di testarlo. Questo può essere un processo difficile. Fortunatamente per noi, il wasCorrectlyAnswered () il metodo è piuttosto semplice. È composto da diversi se altro dichiarazioni. Ogni ramo del codice restituisce un valore. Quando abbiamo un valore di ritorno, possiamo sempre ipotizzare che il test sia fattibile. Non è necessario facile, ma almeno possibile.
function testWasCorrectlyAnsweredAndGettingOutOfPenaltyBoxWhileBeingAWinner () $ this-> setAPlayerThatIsInThePenaltyBox (); $ this-> game-> isGettingOutOfPenaltyBox = true; $ this-> game-> purses [$ this-> game-> currentPlayer] = Game :: $ numberOfCoinsToWin; $ This-> assertTrue ($ this-> gioco-> wasCorrectlyAnswered ());
Non esiste una regola definita su quale test scrivere per primo. Abbiamo appena scelto il primo percorso di esecuzione qui. In realtà abbiamo avuto una bella sorpresa e abbiamo riutilizzato uno dei metodi privati che abbiamo estratto un sacco di tutorial prima. Ma non abbiamo ancora finito. Tutto verde, quindi è tempo di refactoring.
function testWasCorrectlyAnsweredAndGettingOutOfPenaltyBoxWhileBeingAWinner () $ this-> setAPlayerThatIsInThePenaltyBox (); $ This-> currentPlayerWillLeavePenaltyBox (); $ This-> setCurrentPlayerAWinner (); $ This-> assertTrue ($ this-> gioco-> wasCorrectlyAnswered ());
Questo è più facile da leggere e significativamente più descrittivo. Puoi trovare i metodi estratti nel codice allegato.
function testWasCorrectlyAnsweredAndGettingOutOfPenaltyBoxWhileNOTBeingAWinner () $ this-> setAPlayerThatIsInThePenaltyBox (); $ This-> currentPlayerWillLeavePenaltyBox (); $ This-> setCurrentPlayerNotAWinner (); $ This-> assertFalse ($ this-> gioco-> wasCorrectlyAnswered ()); funzione privata setCurrentPlayerNotAWinner () $ this-> game-> purses [$ this-> game-> currentPlayer] = 0;
Ci aspettavamo che questo passasse, ma fallisce. Le ragioni non sono chiare. Uno sguardo più da vicino didPlayerNotWin () potrebbe essere utile.
function didPlayerNotWin () return! ($ this-> purses [$ this-> currentPlayer] == self :: $ numberOfCoinsToWin);
Il metodo restituisce effettivamente true quando un giocatore non ha vinto. Forse potremmo rinominare la nostra variabile ma prima, i test devono passare.
funzione privata setCurrentPlayerAWinner () $ this-> game-> purses [$ this-> game-> currentPlayer] = Game :: $ numberOfCoinsToWin; funzione privata setCurrentPlayerNotAWinner () $ this-> game-> purses [$ this-> game-> currentPlayer] = 0;
Ad un esame più attento possiamo vedere che abbiamo mescolato i valori qui. La nostra confusione tra il nome del metodo e il nome della variabile ci ha fatto capovolgere le condizioni.
funzione privata setCurrentPlayerAWinner () $ this-> game-> purses [$ this-> game-> currentPlayer] = 0; funzione privata setCurrentPlayerNotAWinner () $ this-> game-> purses [$ this-> game-> currentPlayer] = Game :: $ numberOfCoinsToWin - 1;
Questo sta funzionando. Durante l'analisi didPlayerNotWin () abbiamo anche osservato che usa l'uguaglianza per determinare il vincitore. Dobbiamo impostare il nostro valore su uno in meno perché il valore viene incrementato nel codice di produzione che testiamo.
I restanti tre test sono semplici da scrivere. Sono solo variazioni dei primi due. Li puoi trovare nel codice allegato.
Il problema più confuso è l'ingannevole $ vincitore nome variabile. Dovrebbe essere $ notAWinner.
$ notAWinner = $ this-> didPlayerNotWin (); $ This-> currentPlayer ++; if ($ this-> shouldResetCurrentPlayer ()) $ this-> currentPlayer = 0; return $ notAWinner;
Possiamo osservare che il $ notAWinner la variabile è usata solo per restituire un valore. Potremmo chiamare il didPlayerNotWin () metodo direttamente nella dichiarazione di ritorno?
$ This-> currentPlayer ++; if ($ this-> shouldResetCurrentPlayer ()) $ this-> currentPlayer = 0; return $ this-> didPlayerNotWin ();
Questo supera ancora il nostro test unitario, ma se eseguiamo i nostri Golden Master test, falliranno con un errore di "memoria insufficiente". In realtà, il cambiamento rende il gioco non finisce mai.
Quello che sta succedendo è che il giocatore attuale è aggiornato al giocatore successivo. Dato che avevamo un solo giocatore, abbiamo sempre riutilizzato lo stesso giocatore. Ecco come stanno le prove. Non si sa mai quando si scopre una logica nascosta in un codice difficile.
Accentsconagua
function testWasCorrectlyAnsweredAndGettingOutOfPenaltyBoxWhileBeingAWinner () $ this-> setAPlayerThatIsInThePenaltyBox (); $ this-> game-> add ('Another Player'); $ This-> currentPlayerWillLeavePenaltyBox (); $ This-> setCurrentPlayerAWinner (); $ This-> assertTrue ($ this-> gioco-> wasCorrectlyAnswered ()); Aggiungendo semplicemente un altro giocatore in ciascuno dei nostri test relativi a questo metodo, possiamo assicurarci che la logica sia coperta. Questo test farà fallire la dichiarazione di reso modificata sopra.
funzione privata selectNextPlayer () $ this-> currentPlayer ++; if ($ this-> shouldResetCurrentPlayer ()) $ this-> currentPlayer = 0;
Possiamo immediatamente notare che la selezione del giocatore successivo è identica su entrambi i percorsi della condizione. Possiamo spostarlo in un metodo a sé stante. Il nome che abbiamo scelto per questo metodo è selectNextPlayer (). Questo nome aiuta a mettere in evidenza il fatto che il valore del giocatore corrente sarà cambiato. Lo suggerisce anche didPlayerNotWin () potrebbe essere rinominato in qualcosa che riflette la relazione con il giocatore attuale.
function wasCorrectlyAnswered () if ($ this-> inPenaltyBox [$ this-> currentPlayer]) if ($ this-> isGettingOutOfPenaltyBox) $ this-> display-> correctAnswer (); $ This-> borse [$ this-> currentPlayer] ++; $ this-> display-> playerCoins ($ this-> players [$ this-> currentPlayer], $ this-> purses [$ this-> currentPlayer]); $ notAWinner = $ this-> didCurrentPlayerNotWin (); $ This-> selectNextPlayer (); return $ notAWinner; else $ this-> selectNextPlayer (); ritorna vero; else $ this-> display-> correctAnswerWithTypo (); $ This-> borse [$ this-> currentPlayer] ++; $ this-> display-> playerCoins ($ this-> players [$ this-> currentPlayer], $ this-> purses [$ this-> currentPlayer]); $ notAWinner = $ this-> didCurrentPlayerNotWin (); $ This-> selectNextPlayer (); return $ notAWinner;
Il nostro codice si sta facendo più breve e più espressivo. Cosa potremmo fare dopo? Potremmo cambiare il nome strano della logica "non vincente" e cambiare il metodo in una logica positiva anziché negativa. Oppure potremmo continuare a estrarre e gestire la confusione logica negativa in seguito. Non penso ci sia un modo preciso per andare. Quindi, lascerò il problema della logica negativa come esercizio per te e continueremo con l'estrazione.
function wasCorrectlyAnswered () if ($ this-> inPenaltyBox [$ this-> currentPlayer]) return $ this-> getCorrectlyAnsweredForPlayersInPenaltyBox (); else return $ this-> getCorrectlyAnsweredForPlayersNotInPenaltyBox ();
Come regola generale, prova ad avere una singola riga di codice su ogni percorso di una logica decisionale.
Abbiamo estratto l'intero blocco di codice in ogni parte del nostro Se dichiarazione. Questo è un passo importante e qualcosa a cui dovresti sempre pensare. Quando hai un percorso decisionale o un ciclo nel tuo codice, all'interno di esso dovrebbe essere una singola istruzione. La persona che legge questo metodo probabilmente non si preoccuperà dei dettagli di implementazione. A lui o lei interesserà la logica della decisione, il Se dichiarazione.
function wasCorrectlyAnswered () if ($ this-> inPenaltyBox [$ this-> currentPlayer]) return $ this-> getCorrectlyAnsweredForPlayersInPenaltyBox (); return $ this-> getCorrectlyAnsweredForPlayersNotInPenaltyBox ();
E se possiamo sbarazzarci di qualsiasi codice aggiuntivo, dovremmo. Rimozione del altro e mantenendo la stessa logica abbiamo fatto una piccola economia. Mi piace questa soluzione meglio perché evidenzia quale è il comportamento "predefinito" della funzione. Il codice che si trova direttamente all'interno della funzione (l'ultima riga di codice qui). Il Se la dichiarazione è l'eccezionale funzionalità aggiunta a quella di default.
Ho sentito ragionamenti sul fatto che scrivere le condizioni in questo modo potrebbe nascondere il fatto che il default non viene eseguito se il Se la dichiarazione si attiva. Posso solo essere d'accordo con questo, e se preferisci mantenere il altro parte lì per chiarezza, per favore fallo.
funzione privata getCorrectlyAnsweredForPlayersInPenaltyBox () if ($ this-> isGettingOutOfPenaltyBox) return $ this-> getCorrectlyAnsweredForPlayerGettingOutOfPenaltyBox (); else return $ this-> getCorrectlyAnsweredForPlayerStayingInPenaltyBox ();
Possiamo continuare a estrarre all'interno dei nostri metodi privati appena creati. Applicare lo stesso principio alla nostra successiva dichiarazione condizionale porta al codice sopra.
funzione privata giveCurrentUserACoin () $ this-> purses [$ this-> currentPlayer] ++;
Osservando i nostri metodi privati getCorrectlyAnsweredForPlayersNotInPenaltyBox () e getCorrectlyAnsweredForPlayerGettingOutOfPenaltyBox () possiamo immediatamente osservare che un semplice compito è duplicato. Questo incarico può essere ovvio per qualcuno come noi che sa già cosa è con le borse e le monete, ma non per un nuovo arrivato. Estrarre quella singola linea in un metodo giveCurrentUserACoin () è una buona idea.
Aiuta anche con la duplicazione. Se in futuro cambiamo il modo in cui diamo monete ai giocatori, dovremo cambiare il codice solo all'interno di questo metodo privato.
funzione privata getCorrectlyAnsweredForPlayersNotInPenaltyBox () $ this-> display-> correctAnswerWithTypo (); restituire $ this-> getCorrectlyAnsweredForAPlayer (); funzione privata getCorrectlyAnsweredForPlayerGettingOutOfPenaltyBox () $ this-> display-> correctAnswer (); restituire $ this-> getCorrectlyAnsweredForAPlayer (); funzione privata getCorrectlyAnsweredForAPlayer () $ this-> giveCurrentUserACoin (); $ this-> display-> playerCoins ($ this-> players [$ this-> currentPlayer], $ this-> purses [$ this-> currentPlayer]); $ notAWinner = $ this-> didCurrentPlayerNotWin (); $ This-> selectNextPlayer (); return $ notAWinner;
Quindi i due metodi con risposta corretta sono identici, tranne che uno di loro emette qualcosa con un errore di battitura. Abbiamo estratto il codice duplicato e mantenuto le differenze in ogni metodo. Potresti pensare che avremmo potuto usare il metodo estratto con un parametro nel codice del chiamante, e produrre una volta normalmente e una volta con un errore di battitura. Tuttavia, la soluzione sopra proposta ha un vantaggio: mantiene separati i due concetti di non nella casella di rigore e di uscire dalla casella di rigore.
Questo conclude il lavoro wasCorrectlyAnswered ().
function wrongAnswer () $ this-> display-> incorrectAnswer (); $ currentPlayer = $ this-> players [$ this-> currentPlayer]; $ This-> Display-> playerSentToPenaltyBox ($ currentPlayer); $ this-> inPenaltyBox [$ this-> currentPlayer] = true; $ This-> currentPlayer ++; if ($ this-> shouldResetCurrentPlayer ()) $ this-> currentPlayer = 0; return true;
A 11 linee questo metodo non è enorme, ma certamente grande. Ti ricordi la magia numero sette più meno due ricerche? Afferma che il nostro cervello può contemporaneamente pensare a 7 + -2 cose. Cioè, abbiamo una capacità limitata. Quindi, per comprendere facilmente e pienamente un metodo, vogliamo che la logica al suo interno si adatti a tale intervallo. Con un totale di 11 linee e un contenuto di 9 linee, questo metodo è al limite. Puoi dire che in realtà c'è una linea vuota e un'altra con solo una parentesi graffa. Ciò renderebbe solo 7 linee di logica.
Mentre bretelle e spazi sono brevi nello spazio, hanno un significato per noi. Separano parti della logica, hanno un significato, quindi il nostro cervello deve elaborarle. Sì, è più facile rispetto a una logica di confronto a riga intera, ma ancora.
Questo è il motivo per cui il nostro obiettivo per le linee di logica all'interno di un metodo è di 4 righe. Questo è al di sotto del minimo della teoria di cui sopra, quindi sia un geniale che un mediocre programmatore dovrebbero essere in grado di comprendere il metodo.
$ This-> currentPlayer ++; if ($ this-> shouldResetCurrentPlayer ()) $ this-> currentPlayer = 0;
Abbiamo già un metodo per questo pezzo di codice, quindi dovremmo usarlo.
function wrongAnswer () $ this-> display-> incorrectAnswer (); $ currentPlayer = $ this-> players [$ this-> currentPlayer]; $ This-> Display-> playerSentToPenaltyBox ($ currentPlayer); $ this-> inPenaltyBox [$ this-> currentPlayer] = true; $ This-> selectNextPlayer (); ritorna vero;
Meglio, ma dovremmo lasciar perdere o continuare?
$ currentPlayer = $ this-> players [$ this-> currentPlayer]; $ This-> Display-> playerSentToPenaltyBox ($ currentPlayer);
Potremmo integrare la variabile da queste due righe. $ This-> currentPlayer sta ovviamente restituendo il giocatore attuale, quindi non c'è bisogno di ripetere la logica. Non impariamo nulla di nuovo o astratto nulla di nuovo usando la variabile locale.
function wrongAnswer () $ this-> display-> incorrectAnswer (); $ This-> Display-> playerSentToPenaltyBox ($ this-> giocatori [$ this-> currentPlayer]); $ this-> inPenaltyBox [$ this-> currentPlayer] = true; $ This-> selectNextPlayer (); ritorna vero;
Siamo giù a 5 linee. Qualcos'altro lì dentro?
$ this-> inPenaltyBox [$ this-> currentPlayer] = true;
Possiamo estrarre la riga sopra nel suo metodo. Aiuterà a spiegare cosa sta succedendo e isolerà la logica relativa all'invio del giocatore corrente nella casella di rigore al suo posto.
function wrongAnswer () $ this-> display-> incorrectAnswer (); $ This-> Display-> playerSentToPenaltyBox ($ this-> giocatori [$ this-> currentPlayer]); $ This-> sendCurrentPlayerToPenaltyBox (); $ This-> selectNextPlayer (); ritorna vero;
Ancora 5 righe, ma tutte le chiamate di metodo. I primi due mostrano cose. I prossimi due sono legati alla nostra logica. L'ultima riga restituisce solo true. Non vedo alcun modo per rendere questo metodo più facile da capire senza introdurre complessità dalle estrazioni che potremmo fare, ad esempio estraendo i due metodi di visualizzazione in uno privato. Se dovessimo farlo, dove dovrebbe andare questo metodo? In questo Gioco classe, o in Display? Penso che questa sia già una domanda troppo complessa per meritare di essere considerata rispetto alla pura semplicità del nostro metodo.
Facciamo un po 'di statistiche controllando questo fantastico strumento dallo scrittore di PHPUnit https://github.com/sebastianbergmann/phploc.git
./ phploc ... / Refactoring \ Legacy \ Code \ - \ Part \ 1 \: \ The \ Golden \ Master / Source / trivia / php / phploc 2.1-gca70e70 di Sebastian Bergmann. Dimensioni righe di codice (LOC) 232 righe di commento del commento (CLOC) 0 (0,00%) righe di codice non commentate (NCLOC) 232 (100,00%) righe logiche di codice (LLOC) 99 (42,67%) classi 88 (88,89 %) Lunghezza media della classe 88 Lunghezza minima della classe 88 Lunghezza massima della classe 88 Metodo medio Lunghezza 7 Metodo minimo Lunghezza 1 Lunghezza massima del metodo 17 Funzioni 1 (1.01%) Durata media della funzione 1 Non in classi o funzioni 10 (10.10%) Complessità ciclomatica Complessità media per LLOC 0,26 Complessità media per classe 25.00 Complessità di classe minima 25.00 Complessità di classe massima 25.00 Complessità media per metodo 3.18 Complessità del metodo minimo 1.00 Metodo massimo Complessità 10.00 Dipendenze Accesso globale 0 Costanti globali 0 (0,00%) Variabili globali 0 (0,00%) Super-Global Variabili 0 (0,00%) Accesso agli attributi 115 Non statico 115 (100,00%) Statico 0 (0,00%) Metodo Chiamate 21 Non statico 21 (100,00%) Statico 0 (0,00%) Struttura Spazi dei nomi 0 Interfacce 0 Tratti 0 Classi 1 Riassunto Classi 0 (0,00%) Concrete Classes 1 ( 100,00%) Metodi 11 Campo di applicazione Metodi non statici 11 (100,00%) Metodi statici 0 (0,00%) Metodi pubblici di visibilità 11 (100,00%) Metodi non pubblici 0 (0,00%) Funzioni 1 Funzioni denominate 1 (100,00%) Funzioni anonime 0 (0,00%) Costanti 0 Costanti globali 0 (0,00%) Costanti di classe 0 (0,00%)
./ phploc ... / Refactoring \ Legacy \ Code \ - \ Part \ 11 \: \ The \ End \? / Source / trivia / php phploc 2.1-gca70e70 di Sebastian Bergmann. Dimensioni righe di codice (LOC) 371 righe di commento del commento (CLOC) 0 (0,00%) righe di codice non di commento (NCLOC) 371 (100,00%) righe di codice logiche (LLOC) 151 (40,70%) classi 145 (96,03 %) Lunghezza media della classe 36 Lunghezza minima della classe 8 Lunghezza massima della classe 89 Metodo medio Lunghezza 2 Metodo minimo Lunghezza 1 Lunghezza massima del metodo 14 Funzioni 0 (0,00%) Lunghezza funzione media 0 Non in classi o funzioni 6 (3,97%) Complessità ciclomatica Complessità media per LLOC 0,15 Complessità media per classe 6,50 Complessità di classe minima 1.00 Complessità di classe massima 17.00 Complessità media per metodo 1.46 Complessità del metodo minimo 1.00 Metodo massimo Complessità 10.00 Dipendenze Accesso globale 0 Costanti globali 0 (0,00%) Variabili globali 0 (0,00%) Super-Global Variabili 0 (0,00%) Accesso attributi 96 Non statico 94 (97,92%) Static 2 (2,08%) Metodo Chiamate 74 Non statico 74 (100,00%) Statico 0 (0,00%) Struttura Spazi dei nomi 0 Interfacce 1 Tratti 0 Classi 3 Riassunto Classi 0 (0,00%) Concrete Classes 3 (100,00 %) Metodi 59 Metodi non statici di Scope 59 (100,00%) Metodi statici 0 (0,00%) Metodi pubblici di visibilità 35 (59,32%) Metodi non pubblici 24 (40,68%) Funzioni 0 Funzioni nominate 0 (0,00%) Funzioni anonime 0 (0,00%) Costanti 3 Costanti globali 0 (0,00%) Costanti di classe 3 (100,00%)
I dati brutali sono altrettanto validi che possiamo capire e analizzare.
Il numero di righe logiche di codice è aumentato in modo significativo da 99 a 151. Ma questo numero non dovrebbe farti pensare che il nostro codice sia diventato più complesso. Questa è una tendenza naturale del codice ben refactored, a causa dell'aumento del numero di metodi e chiamate a loro.
Non appena esaminiamo la lunghezza media della classe, possiamo vedere un drastico calo delle righe di codice, da 88 a 36.
Ed è semplicemente sorprendente come la lunghezza del metodo sia scesa da una media di sette righe a solo due righe di codice.
Mentre il numero di linee è un buon indicatore del volume di codice per unità di misura, il vero guadagno è nelle analisi della complessità ciclomatica. Ogni volta che prendiamo una decisione nel nostro codice, aumentiamo la complessità ciclomatica. Quando facciamo catena Se affermazioni una dentro l'altra, la complessità ciclomatica di quel metodo aumenta esponenzialmente. Le nostre continue estrazioni hanno portato a metodi con una sola decisione al loro interno, riducendo così la loro complessità media per metodo da 3.18 a 1.00. Puoi leggere questo come "i nostri metodi refactored sono 3.18 volte più semplici del codice originale". A livello di classe, il calo della complessità è ancora più sorprendente. Si abbassa da 25,00 a 6,50.
Bene. Questo è tutto. Fine della serie. Sentitevi liberi di esprimere le vostre opinioni e se pensate di aver perso qualche argomento di refactoring chiedetelo nei commenti qui sotto. Se sono interessanti li trasformo in parti extra di questa serie.
Grazie per la tua totale attenzione.
