舊程式碼,醜陋的程式碼,複雜的程式碼,義大利麵條似的程式碼,鬼話廢話……就是四個字:遺留程式碼。這是一個系列文章,將有助於你處理並解決它。
- 重構遺留程式碼(1):金牌大師
- 重構遺留程式碼(2):魔術字串和常量
- 重構遺留程式碼(3):複雜的條件語句
- 重構遺留程式碼(4):第一個單元測試
- 重構遺留程式碼(5):遊戲的可測試方法
- 重構遺留程式碼(6):進攻複雜的方法
- 重構遺留程式碼(7):識別表示層
- 重構遺留程式碼(8):一個整潔架構的依賴反轉
- 重構遺留程式碼(9):分析 Concerns
- 重構遺留程式碼(10):剖析長方法
在我們前面的課程中,我們已經學習了一種通過提取直到放棄來理解和做出更好程式碼的方法。雖然那篇教程是學習這項技術的不錯的方式,但它並不是理解這項技術好處的理想例子。在這篇教程中,我們將對所有的問答遊戲相關程式碼使用提取直到放棄的方法,並且我們將分析最後的結果。
這篇教程也將總結我們關於重構這一系列的文章。如果你認為我們遺漏了什麼,以建議的主題來隨意評論吧。如果好主意收集了,我將基於你的要求繼續寫額外的教程。
進攻我們最長的方法
拿我們最長的方法並將它提取到小段程式碼中,有比這更好地開始我們文章的方式嗎。像往常一樣,測試優先將使得這一過程不僅有效而且也有趣。
通常,當我開始這篇教程的時候,你已經有了程式碼,它在php start資料夾裡,而最終結果在php資料夾。
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 |
function wasCorrectlyAnswered() { if ($this->inPenaltyBox[$this->currentPlayer]) { if ($this->isGettingOutOfPenaltyBox) { $this->display->correctAnswer(); $this->purses[$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->purses[$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; } } |
wasCorrectlyAnswered()這個方法是我們第一個犧牲者。
1 |
測試wasCorrectlyAnswered()
正如我們在前一課程學到的,修改遺留程式碼的第一步是測試它。這是個艱難的過程。對我們來說幸運的是,wasCorrectlyAnswered()方法很簡單。它由幾個if-else表示式組成。程式碼的每個分支返回一個值。當我們有返回值時,我們總可以推測測試是可行的。不一定容易,但至少可能。
1 2 3 4 5 6 7 |
function testWasCorrectlyAnsweredAndGettingOutOfPenaltyBoxWhileBeingAWinner() { $this->setAPlayerThatIsInThePenaltyBox(); $this->game->isGettingOutOfPenaltyBox = true; $this->game->purses[$this->game->currentPlayer] = Game::$numberOfCoinsToWin; $this->assertTrue($this->game->wasCorrectlyAnswered()); } |
先寫什麼測試沒有明確的規則。我們只選擇這裡執行部分的第一分支。實際上我們有個不錯的驚喜,我們重用了在之前的教程中提取很多次的私有方法中的一個。但我們還沒完成。全部通過,那麼是時候重構了。
1 2 3 4 5 6 7 |
function testWasCorrectlyAnsweredAndGettingOutOfPenaltyBoxWhileBeingAWinner() { $this->setAPlayerThatIsInThePenaltyBox(); $this->currentPlayerWillLeavePenaltyBox(); $this->setCurrentPlayerAWinner(); $this->assertTrue($this->game->wasCorrectlyAnswered()); } |
這更容易閱讀並且明顯更具描述性。你可以在附帶的程式碼中找到提取的方法。
1 2 3 4 5 6 7 8 9 10 11 |
function testWasCorrectlyAnsweredAndGettingOutOfPenaltyBoxWhileNOTBeingAWinner() { $this->setAPlayerThatIsInThePenaltyBox(); $this->currentPlayerWillLeavePenaltyBox(); $this->setCurrentPlayerNotAWinner(); $this->assertFalse($this->game->wasCorrectlyAnswered()); } private function setCurrentPlayerNotAWinner() { $this->game->purses[$this->game->currentPlayer] = 0; } |
我們期望這能通過,但它失敗了。原因還不明確。對didPlayerNotWin()解析也許會有幫助。
1 2 3 |
function didPlayerNotWin() { return !($this->purses[$this->currentPlayer] == self::$numberOfCoinsToWin); } |
事實上當玩家沒有贏的時候方法返回了true。也許我們可以重新命名變數,但首先測試必須通過。
1 2 3 4 5 6 7 |
private function setCurrentPlayerAWinner() { $this->game->purses[$this->game->currentPlayer] = Game::$numberOfCoinsToWin; } private function setCurrentPlayerNotAWinner() { $this->game->purses[$this->game->currentPlayer] = 0; } |
通過仔細觀察,我們知道我們在這兒混淆了值。我們的困惑存在於在方法名和變數名之間,使得我們反轉了條件。
1 2 3 4 5 6 7 |
private function setCurrentPlayerAWinner() { $this->game->purses[$this->game->currentPlayer] = 0; } private function setCurrentPlayerNotAWinner() { $this->game->purses[$this->game->currentPlayer] = Game::$numberOfCoinsToWin - 1; } |
這樣可以了。當分析didPlayerNotWin()的時候,我們同樣注意到它使用等於來決定贏家。我們必須設定值,一個都不能少,因為在我們測試的產品程式碼中值是增加的。
剩下的三個測試程式很容易寫。它們只是前兩個的變體。你可以在附帶的程式碼中找到它們。
提取並重新命名wasCorrectlyAnswered()方法直到我們放棄
最困惑的一個問題是具有誤導性的$winner變數名。那應該是$notWinner。
1 2 3 4 5 6 |
$notAWinner = $this->didPlayerNotWin(); $this->currentPlayer++; if ($this->shouldResetCurrentPlayer()) { $this->currentPlayer = 0; } return $notAWinner; |
我們看到$notWinner變數是隻用來返回值的。我們可以在return語句中直接呼叫didPlayerNotWin()方法嗎?
1 2 3 4 5 |
$this->currentPlayer++; if ($this->shouldResetCurrentPlayer()) { $this->currentPlayer = 0; } return $this->didPlayerNotWin(); |
這仍然使得我們的單元測試通過了,但如果我們執行金牌大師測試程式,將會以“沒有足夠記憶體”而失敗。事實上,修改使得遊戲永遠結束不了。
目前的情況是,當前玩家被更新為下一個玩家了。因為我們只有一個單一的玩家,所以我們總是複用相同的玩家。這就是測試發生的。你永遠不會知道什麼時候在一段很難的程式碼中你會發現一個隱藏的邏輯。
1 2 3 4 5 6 7 8 |
function testWasCorrectlyAnsweredAndGettingOutOfPenaltyBoxWhileBeingAWinner() { $this->setAPlayerThatIsInThePenaltyBox(); $this->game->add('Another Player'); $this->currentPlayerWillLeavePenaltyBox(); $this->setCurrentPlayerAWinner(); $this->assertTrue($this->game->wasCorrectlyAnswered()); } |
只通過在針對這個方法的每個測試方法中增加另一個玩家,我們可以確保邏輯被覆蓋到。測試將使得上述修改後的return語句失敗。
1 2 3 4 5 6 |
private function selectNextPlayer() { $this->currentPlayer++; if ($this->shouldResetCurrentPlayer()) { $this->currentPlayer = 0; } } |
我們可以馬上發現對於下一個玩家的選擇在兩個條件下都是相同的。我們可以將其移到一個方法中去。我們給這個方法的名字是selectNextPlayer()。這個名字幫助突出當前玩家的值將被修改這一事實。它同樣表明didPlayerNotWin()方法可以被重新命名為能反映當前玩家關係的名字。
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 |
function wasCorrectlyAnswered() { if ($this->inPenaltyBox[$this->currentPlayer]) { if ($this->isGettingOutOfPenaltyBox) { $this->display->correctAnswer(); $this->purses[$this->currentPlayer]++; $this->display->playerCoins($this->players[$this->currentPlayer], $this->purses[$this->currentPlayer]); $notAWinner = $this->didCurrentPlayerNotWin(); $this->selectNextPlayer(); return $notAWinner; } else { $this->selectNextPlayer(); return true; } } else { $this->display->correctAnswerWithTypo(); $this->purses[$this->currentPlayer]++; $this->display->playerCoins($this->players[$this->currentPlayer], $this->purses[$this->currentPlayer]); $notAWinner = $this->didCurrentPlayerNotWin(); $this->selectNextPlayer(); return $notAWinner; } } |
我們的程式碼變得更短並更具說明力了。接下來我們能做什麼?我們可以修改“not winner”邏輯的奇怪的名字並且將方法從負邏輯改為正邏輯。或者我們可以繼續提取並稍後處理混亂的負邏輯。我不認為有一種確定的方式去做這事。所以,我將把負邏輯的問題作為你的一個練習,我們將繼續提取。
1 2 3 4 5 6 7 |
function wasCorrectlyAnswered() { if ($this->inPenaltyBox[$this->currentPlayer]) { return $this->getCorrectlyAnsweredForPlayersInPenaltyBox(); } else { return $this->getCorrectlyAnsweredForPlayersNotInPenaltyBox(); } } |
作為一個經驗法則,試著將一行程式碼放到決策邏輯的每條分支上。
我們提取每個if表示式中的整塊程式碼。這是很重要的步驟,是你應該經常思考的。當你的程式碼中有決策路徑或者迴圈,它其中應該只有單一的表示式。閱讀這個方法的人大部分可能是不關心具體的實現的。他或者她會關心決策邏輯,就是if表示式。
1 2 3 4 5 6 7 |
function wasCorrectlyAnswered() { if ($this->inPenaltyBox[$this->currentPlayer]) { return $this->getCorrectlyAnsweredForPlayersInPenaltyBox(); } return $this->getCorrectlyAnsweredForPlayersNotInPenaltyBox(); } |
如果我們擺脫了任何額外的程式碼,我們應該這麼做。移除else並且在我們稍稍做修改的情況下,仍舊保持相同的邏輯。我更喜歡這個解決方案,因為它突出了函式“預設的”行為是什麼。該程式碼是直接在函式的內部(這裡的最後一行程式碼)。if表示式是加到預設函式的特殊的功能。
我聽說推論按照這種方式寫條件如果if表示式啟用的話,可能會隱藏預設沒有執行的事實。我只能贊同這點,如果你更願意保留else部分以更清晰,請這麼做。
1 2 3 4 5 6 7 |
private function getCorrectlyAnsweredForPlayersInPenaltyBox() { if ($this->isGettingOutOfPenaltyBox) { return $this->getCorrectlyAnsweredForPlayerGettingOutOfPenaltyBox(); } else { return $this->getCorrectlyAnsweredForPlayerStayingInPenaltyBox(); } } |
我們可以繼續在新建的私有方法中提取。對下一個條件表示式適用相同的規則將帶來上述的程式碼。
1 2 3 |
private function giveCurrentUserACoin() { $this->purses[$this->currentPlayer]++; } |
通過觀察私有方法getCorrectlyAnsweredForPlayerNotInPenaltyBox()和getCorrectlyAnsweredForPlayerGettingOutOfPenaltyBox(),我們可以馬上注意到簡單的任務重複了。那個任務可能對於像我們這樣已經知道錢包和錢幣的人來說是明顯的,但對一個新人來說不是。將那一行提取到方法giveCurrentUserACoin()中是個好主意。
它也對處理重複有利。如果將來我們修改給予玩家錢幣的方式,我們只需要修改這個私有方法的程式碼。
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 |
private function getCorrectlyAnsweredForPlayersNotInPenaltyBox() { $this->display->correctAnswerWithTypo(); return $this->getCorrectlyAnsweredForAPlayer(); } private function getCorrectlyAnsweredForPlayerGettingOutOfPenaltyBox() { $this->display->correctAnswer(); return $this->getCorrectlyAnsweredForAPlayer(); } private function getCorrectlyAnsweredForAPlayer() { $this->giveCurrentUserACoin(); $this->display->playerCoins($this->players[$this->currentPlayer], $this->purses[$this->currentPlayer]); $notAWinner = $this->didCurrentPlayerNotWin(); $this->selectNextPlayer(); return $notAWinner; } |
那麼兩個正確回答的方法是相同的,除了其中一個輸出了帶錯字的內容。我們提取了重複的程式碼並且將不同保持在各自方法裡。你可能會想我們可以在呼叫程式碼處使用提取的帶一個引數的方法,並且一次輸出正常,一次輸出帶錯字。然而,上述提出的解決方案有個優點:它使兩個概念分離,不在禁區和擺脫禁區。
這就是對wasCorrectlyAnswered()處理的結論。
1 |
wrongAnswer()方法怎麼處理?
1 2 3 4 5 6 7 8 9 10 11 12 |
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; } |
這個方法有11行不算巨大,但無疑很大。你還記得魔術數字7加負二的研究嗎?它指出,我們的大腦可以同時想7+-2這樣的事。也就是說,我們的能力有限。所以為了容易並完全得理解一個方法,我們需要它的內部邏輯適用其範圍。總共11行,包括9行內容,這個方法就是這種極限。你可能會說,其實也有一個空行,另外只是一個括號。這使得邏輯行只有7行。
雖然括號和空格所佔空間很短,但對於我們它們是有意義的。它們分離了邏輯部分,並且有意義,所以我們的大腦必須處理它們。是的,相對於全部的比較邏輯,它是容易的但仍需處理。
這就是為什麼方法中我們的目標邏輯行是4行的原因。那是對上述邏輯最低線之下的,這樣天才和平庸的程式設計師都應該能夠理解方法。
1 2 3 4 |
$this->currentPlayer++; if ($this->shouldResetCurrentPlayer()) { $this->currentPlayer = 0; } |
我們已經有了針對這段程式碼的一個方法,所以我們應該利用它。
1 2 3 4 5 6 7 8 |
function wrongAnswer() { $this->display->incorrectAnswer(); $currentPlayer = $this->players[$this->currentPlayer]; $this->display->playerSentToPenaltyBox($currentPlayer); $this->inPenaltyBox[$this->currentPlayer] = true; $this->selectNextPlayer(); return true; } |
更好了,但我們應該放棄還是繼續?
1 2 |
$currentPlayer = $this->players[$this->currentPlayer]; $this->display->playerSentToPenaltyBox($currentPlayer); |
我們可以從這兩行內聯變數。$this->currentPlayer很明顯返回當前玩家,所以沒必要重複這個邏輯。通過使用區域性變數,我們沒學到任何新東西或者抽象任何新東西。
1 2 3 4 5 6 7 |
function wrongAnswer() { $this->display->incorrectAnswer(); $this->display->playerSentToPenaltyBox($this->players[$this->currentPlayer]); $this->inPenaltyBox[$this->currentPlayer] = true; $this->selectNextPlayer(); return true; } |
降到5行了。還有什麼其他內容嗎?
1 |
$this->inPenaltyBox[$this->currentPlayer] = true; |
我們可以把上面這行提取到它自己的方法中。它將幫助解釋發什麼並且隔離在它自己的方法中的送當前玩家到禁區的邏輯裡。
1 2 3 4 5 6 7 |
function wrongAnswer() { $this->display->incorrectAnswer(); $this->display->playerSentToPenaltyBox($this->players[$this->currentPlayer]); $this->sendCurrentPlayerToPenaltyBox(); $this->selectNextPlayer(); return true; } |
還是5行,但所有都是方法呼叫。最初的兩個是顯示用的。接下來的兩個關係我們的邏輯。最後一行只返回true。不引入複雜的提取物,我看沒有辦法使這個方法更容易理解,例如通過提取兩個顯示方法到一個私有方法中的方式。如果我們要這麼做,方法應該怎麼提取?放入這個Game類,還是放入Display類?我認為這已經是一個太複雜的問題,針對我們方法純粹的簡單性來說,這點不值得考慮。
最後的思考和一些統計
讓我們通過從PHPUnit開發者那裡檢出這個強大的工具https://github.com/sebastianbergmann/phploc.git來做一些統計。
原始問答遊戲程式碼的統計
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63 64 65 66 67 68 69 70 71 72 73 74 75 76 77 78 79 80 81 82 83 84 85 86 87 88 89 90 91 92 93 94 95 96 97 98 99 100 101 102 103 104 105 106 107 108 109 110 111 |
./phploc ../Refactoring Legacy Code - Part 1: The Golden Master/Source/trivia/php/ phploc 2.1-gca70e70 by Sebastian Bergmann. Size Lines of Code (LOC) 232 Comment Lines of Code (CLOC) 0 (0.00%) Non-Comment Lines of Code (NCLOC) 232 (100.00%) Logical Lines of Code (LLOC) 99 (42.67%) Classes 88 (88.89%) Average Class Length 88 Minimum Class Length 88 Maximum Class Length 88 Average Method Length 7 Minimum Method Length 1 Maximum Method Length 17 Functions 1 (1.01%) Average Function Length 1 Not in classes or functions 10 (10.10%) Cyclomatic Complexity Average Complexity per LLOC 0.26 Average Complexity per Class 25.00 Minimum Class Complexity 25.00 Maximum Class Complexity 25.00 Average Complexity per Method 3.18 Minimum Method Complexity 1.00 Maximum Method Complexity 10.00 Dependencies Global Accesses 0 Global Constants 0 (0.00%) Global Variables 0 (0.00%) Super-Global Variables 0 (0.00%) Attribute Accesses 115 Non-Static 115 (100.00%) Static 0 (0.00%) Method Calls 21 Non-Static 21 (100.00%) Static 0 (0.00%) Structure Namespaces 0 Interfaces 0 Traits 0 Classes 1 Abstract Classes 0 (0.00%) Concrete Classes 1 (100.00%) Methods 11 Scope Non-Static Methods 11 (100.00%) Static Methods 0 (0.00%) Visibility Public Methods 11 (100.00%) Non-Public Methods 0 (0.00%) Functions 1 Named Functions 1 (100.00%) Anonymous Functions 0 (0.00%) Constants 0 Global Constants 0 (0.00%) Class Constants 0 (0.00%) |
重構後問答遊戲程式碼的統計
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 |
./phploc ../Refactoring Legacy Code - Part 11: The End?/Source/trivia/php phploc 2.1-gca70e70 by Sebastian Bergmann. Size Lines of Code (LOC) 371 Comment Lines of Code (CLOC) 0 (0.00%) Non-Comment Lines of Code (NCLOC) 371 (100.00%) Logical Lines of Code (LLOC) 151 (40.70%) Classes 145 (96.03%) Average Class Length 36 Minimum Class Length 8 Maximum Class Length 89 Average Method Length 2 Minimum Method Length 1 Maximum Method Length 14 Functions 0 (0.00%) Average Function Length 0 Not in classes or functions 6 (3.97%) Cyclomatic Complexity Average Complexity per LLOC 0.15 Average Complexity per Class 6.50 Minimum Class Complexity 1.00 Maximum Class Complexity 17.00 Average Complexity per Method 1.46 Minimum Method Complexity 1.00 Maximum Method Complexity 10.00 Dependencies Global Accesses 0 Global Constants 0 (0.00%) Global Variables 0 (0.00%) Super-Global Variables 0 (0.00%) Attribute Accesses 96 Non-Static 94 (97.92%) Static 2 (2.08%) Method Calls 74 Non-Static 74 (100.00%) Static 0 (0.00%) Structure Namespaces 0 Interfaces 1 Traits 0 Classes 3 Abstract Classes 0 (0.00%) Concrete Classes 3 (100.00%) Methods 59 Scope Non-Static Methods 59 (100.00%) Static Methods 0 (0.00%) Visibility Public Methods 35 (59.32%) Non-Public Methods 24 (40.68%) Functions 0 Named Functions 0 (0.00%) Anonymous Functions 0 (0.00%) Constants 3 Global Constants 0 (0.00%) Class Constants 3 (100.00%) |
分析
本身的資料一樣好,我們可以理解並分析它。
邏輯程式碼行數增加相當顯著從99行加到了151行。但這個數字不應該欺騙你讓你認為我們的程式碼變得更復雜了。這是一個良好重構程式碼的自然趨勢,因為增加了方法和對它們的呼叫程式碼。
只要我們看看類的平均長度,我們可以看到程式碼行數大幅下降,從88行降到36行。
這是多麼得驚人,方法長度從平均7行降到只有2行程式碼。
行數是每個計量單位程式碼量的良好指標,真正獲得的是圈複雜度的分析。每次我們對程式碼做了個決定,我們將增加了圈複雜度。當我們把if表示式放在另一個裡面時,該方法的圈複雜度呈指數上升。我們持續得提取導致方法中只有一個單一的決策,從而降低了它們每個方法的平均複雜度從3.18到1.00。你可以將其理解為“我們重構的方法比原始程式碼簡單3.18倍”。在類級別,複雜度的降低甚至更加驚人。它從25.00降到了6.50。
結束了?
好了,就這樣了。結束這個系列。如果你認為我們遺漏了任何重構話題,請在下面的評論中自由表述你的意見來詢問它們。如果它們很有趣那我將會把它們移到這個系列的額外篇幅中。
感謝你的全神貫注。