舊程式碼,醜陋的程式碼,複雜的程式碼,義大利麵條似的程式碼,鬼話廢話……就是四個字:遺留程式碼。這是一個系列文章,將有助於你處理並解決它。
我們第一次見到遺留程式碼是在我們的上一篇教程。然後我們執行了程式碼來對其基本功能形成一個認識,並且建立了金牌大師測試套件來為未來的程式碼修改修建一張安全網。我們將繼續處理這個程式碼,你可以在trivia/php_start
這個目錄下找到它。另一個目錄trivia/php_start
則包含了這次教程的完成程式碼。
做出第一次改變的時機已經來到,比起理解一段艱難的程式碼庫來說,著手將魔術常量和字串抽出到變數中是不是不失為一種更好的方式?這看似簡單的任務將帶給我們對於遺留程式碼的內在作用機制更廣泛的有時是意想不到的洞見。我們需要弄清楚原始碼開發者的意圖,並且為我們之前不曾見過的程式碼片段找個合適的名稱。
魔術字串
魔術字串是沒有被賦值給變數的,直接用在各種表示式中的字串。這種型別的字串對於原開發者的程式碼來說有特殊意義,但取代把它們賦值給一個適當命名的變數來說,原開發者認為該字串的含義是顯而易見的。
識別第一個需要修改的字串
讓我們開始在Game.php
中尋找並識別字串。如果你在用IDE(並且你應該用)或者有高亮顯示程式碼功能的智慧文字編輯器,定位字串將會容易些。這裡是在我的顯示器上程式碼的樣子。
每一個橘色的都是字串。通過這種方式找到我們程式碼中的每個字串是很容易的。所以,無論你的應用程式是什麼,確保你的編輯器支援高亮並且可用。
我們程式碼中第一塊橘色的部分立即顯示在第三行。但是這個字串僅僅包含一個換行符。這在我看來應該顯而易見,所以我們繼續。
當要決定什麼該抽取,什麼該保留不變時,很少有贊成的,但最後你的專業意見必須佔上風。基於這點,你將必須決定對你所分析的每一塊程式碼做什麼。
1 2 3 4 5 6 7 8 9 10 11 |
for ($i = 0; $i < 50; $i++) { array_push($this->popQuestions, "Pop Question " . $i); array_push($this->scienceQuestions, ("Science Question " . $i)); array_push($this->sportsQuestions, ("Sports Question " . $i)); array_push($this->rockQuestions, $this->createRockQuestion($i)); } } function createRockQuestion($index) { return "Rock Question " . $index; } |
那麼讓我們來看看32到42行,就是你看到的上面的片段。對於pop,science和sports questions,僅僅是一個簡單的拼接。然而,為rock question組成字串的動作被抽出到一個方法中了。在你看來,這一系列的字串足夠清晰以至於我們可以將它們保留在我們的for迴圈中嗎?或者,你認為抽取所有的字串到它們的方法中將證明那些方法的存在?如果這樣的話,你將如何命名那些方法?
更新金牌大師和測試程式
不管結果如何,我們都需要修改程式碼。是時候讓我們的金牌大師執行,編寫能實際執行的測試程式,並將我們的程式碼和現有的內容進行比較了。
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 |
class GoldenMasterTest extends PHPUnit_Framework_TestCase { private $gmPath; function setUp() { $this->gmPath = __DIR__ . '/gm.txt'; } function testGenerateOutput() { $times = 20000; $this->generateMany($times, $this->gmPath); } function testOutputMatchesGoldenMaster() { $times = 20000; $actualPath = '/tmp/actual.txt'; $this->generateMany($times, $actualPath); $file_content_gm = file_get_contents($this->gmPath); $file_content_actual = file_get_contents($actualPath); $this->assertTrue($file_content_gm == $file_content_actual); } private function generateMany($times, $fileName) {...} private function generateOutput($seed) {...} } |
我們建立了另一個測試程式去與輸出比較,確保testGenerateOutput()
僅產生一次輸出並且不再做別的。我們也把金牌大師的輸出檔案"gm.txt"
放到當前目錄,因為"/tmp"
目錄當系統重啟時可能會清空。對於我們實際的結果,我們仍將使用它。在大多數UNIX類系統中"/tmp"
被安裝到RAM使得它比檔案系統快得多。如果你做的足夠好,測試程式將正常通過。
為將來的修改,記得將我們的生成器程式標記為“跳過”是很重要的。如果註釋它或甚至是全部刪除它能讓你感覺更舒坦的話,那麼請這麼做。重要的是,當我們修改程式碼時,金牌大師將不會變。它是一次生成的並且我們不想修改它,永遠不,這樣我們就能保證新生成的程式碼總能和原始的進行比較。如果你覺得對它做個備份更舒適的話,請繼續這麼做。
1 2 3 4 5 |
function testGenerateOutput() { $this->markTestSkipped(); $times = 20000; $this->generateMany($times, $this->gmPath); } |
我將僅標記測試程式為跳過。這將使我們的測試結果變為"yellow"
,意味著所有的測試程式都通過但是一些是跳過或者標記為未完成的。
做出第一個修改
測試程式就位,我們可以開始改程式碼了。根據我的專業意見,所有的字串都可以被保留在for
迴圈裡。我們將把createRockQuestion()
方法的程式碼移到for
迴圈中,並且刪除這個方法。這種重構稱為內聯方法。
“把方法程式碼放到它的呼叫者的方法體中並刪除該方法” ~ Martin Fowler
有一套具體的步驟去做這種型別的重構,如同Marting Fowler在《重構:改善既有程式碼的設計》中定義的那樣:
- 檢查該方法沒有多型性。
- 找到所有呼叫該方法的呼叫者。
- 用方法程式碼替換每一個呼叫者的程式碼。
- 編譯並測試。
- 刪除方法定義。
我們沒有子類繼承 Game
,所以第一步有效。
只有一個使用我們方法的呼叫者,在for
迴圈內。
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 |
function __construct() { $this->players = array(); $this->places = array(0); $this->purses = array(0); $this->inPenaltyBox = array(0); $this->popQuestions = array(); $this->scienceQuestions = array(); $this->sportsQuestions = array(); $this->rockQuestions = array(); for ($i = 0; $i < 50; $i++) { array_push($this->popQuestions, "Pop Question " . $i); array_push($this->scienceQuestions, ("Science Question " . $i)); array_push($this->sportsQuestions, ("Sports Question " . $i)); array_push($this->rockQuestions, "Rock Question " . $i); } } function createRockQuestion($index) { return "Rock Question " . $index; } |
在構造器中,我們將方法createRockQuestion()
的程式碼放到for
迴圈中。我們仍保留舊程式碼。現在是時候執行我們的測試了。
我們的測試通過了。可以刪除createRockQuestion()
方法了。
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 |
function __construct() { // ... // for ($i = 0; $i < 50; $i++) { array_push($this->popQuestions, "Pop Question " . $i); array_push($this->scienceQuestions, ("Science Question " . $i)); array_push($this->sportsQuestions, ("Sports Question " . $i)); array_push($this->rockQuestions, "Rock Question " . $i); } } function isPlayable() { return ($this->howManyPlayers() >= 2); } |
最後我們應該再次執行測試程式。如果我們漏了對方法的呼叫,那麼測試將會失敗。
測試程式應該再次通過。恭喜!我們完成了第一個重構。
另一些需要考慮的字串
add()
和roll()
方法中字串僅僅通過呼叫echoln()
方法輸出它們。askQuestions()
方法比較字串型別。這看起來也能接受。另一方面,currentCategory()
基於數字返回字串。這個方法中,有許多重複的字串。僅僅在這個方法中,改變除了Rock以外的任何類別都需要在三處修改它的名字。
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 |
function currentCategory() { if ($this->places[$this->currentPlayer] == 0) { return "Pop"; } if ($this->places[$this->currentPlayer] == 4) { return "Pop"; } if ($this->places[$this->currentPlayer] == 8) { return "Pop"; } if ($this->places[$this->currentPlayer] == 1) { return "Science"; } if ($this->places[$this->currentPlayer] == 5) { return "Science"; } if ($this->places[$this->currentPlayer] == 9) { return "Science"; } if ($this->places[$this->currentPlayer] == 2) { return "Sports"; } if ($this->places[$this->currentPlayer] == 6) { return "Sports"; } if ($this->places[$this->currentPlayer] == 10) { return "Sports"; } return "Rock"; } |
我認為我們能做得更好。我們可以使用一種稱為引入區域性變數的重構方法並清除重複。下面是方針:
- 增加一個變數並賦上想要的值。
- 找到所有使用這個值的地方。
- 替換所有使用這個值的為該變數。
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 |
function currentCategory() { $popCategory = "Pop"; $scienceCategory = "Science"; $sportCategory = "Sports"; $rockCategory = "Rock"; if ($this->places[$this->currentPlayer] == 0) { return $popCategory; } if ($this->places[$this->currentPlayer] == 4) { return $popCategory; } if ($this->places[$this->currentPlayer] == 8) { return $popCategory; } if ($this->places[$this->currentPlayer] == 1) { return $scienceCategory; } if ($this->places[$this->currentPlayer] == 5) { return $scienceCategory; } if ($this->places[$this->currentPlayer] == 9) { return $scienceCategory; } if ($this->places[$this->currentPlayer] == 2) { return $sportCategory; } if ($this->places[$this->currentPlayer] == 6) { return $sportCategory; } if ($this->places[$this->currentPlayer] == 10) { return $sportCategory; } return $rockCategory; } |
這好很多了。你可能已經注意到一些我們可以對程式碼在未來能做的改進,但我們才剛開始我們的作業。你可以立即修正你發現的一切這點是很誘人的,但請別這麼做。許多時候,特別是在程式碼被很好地理解之前,這種修改會引向死衚衕甚至更爛的程式碼。如果你覺得你有可能會忘掉你的想法,就在便利貼上或你的工程管理軟體中建立一個任務記下來。現在我們需要接著處理我們的字串相關問題了。
在接下來的檔案中,所有的字串都是輸出相關的,放入echoln()
方法。目前這個階段,我們不會去碰它們。修改它們可能會影響我們的應用程式的列印和傳遞邏輯。它們是表示層和業務邏輯混合的一部分。我們將在將來的教程中分別處理不同的關注點。
魔術常量
魔術常量和魔術字串很像,但是值不同。它們的值可以是布林值或者數字。我們將專注於在if表示式或者 return 表示式或其他的表示式中使用數字。如果這些數字沒有明確的意義,我們需要將它們抽出到變數或者方法中。
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 |
include_once __DIR__ . '/Game.php'; $notAWinner; $aGame = new Game(); $aGame->add("Chet"); $aGame->add("Pat"); $aGame->add("Sue"); do { $aGame->roll(rand(0, 5) + 1); if (rand(0, 9) == 7) { $notAWinner = $aGame->wrongAnswer(); } else { $notAWinner = $aGame->wasCorrectlyAnswered(); } } while ($notAWinner); |
這次我們將以GameRunner.php開始,我們首先關注提供一些隨機數的roll()方法。之前的開發者沒有在意給那些數字以意義。我們可以嗎?如果我們分析程式碼:
1 |
rand(0, 5) + 1 |
它會返回1到6之間的數字。隨機部分返回0到5之間的數字,我們總是加1。所以可以肯定在1到6之間。現在我們需要考慮我們的應用程式的內容了。我們開發了一個問答遊戲。我們知道有某種型別的告示板標示我們的參與者必須移動。為了做到這點,我們需要擲骰子。骰子有六個面並且它可以產生1到6之間的數字。這似乎是個合理的推論。
1 2 |
$dice = rand(0, 5) + 1; $aGame->roll($dice); |
這不是很美妙嗎?我們又一次使用了引入區域性變數的重構概念。我們命名新的變數$dice並且它代表產生1到6之間的隨機數。這也使得我們接下來的表述聽起來很自然:玩遊戲,擲骰子。
你執行你的測試程式了嗎?我沒提到它,但我們需要儘可能頻繁得執行它們。如果你還沒有,那麼此時將是個很好的時機跑起來。它們應該是通過的。
所以,這無非只是一個交換數字變數的例子。我們讓一整個表示式代表數字並把它提取到一個變數中。這從技術上可以認為是魔術常量的例子,但並不是一個完全的例子。我們下面的隨機表示式怎麼樣?
1 |
if (rand(0, 9) == 7) |
這個更棘手。0,9和7在這個表示式中是什麼?也許我們可以給它們命名。乍一看,我對0和9毫無辦法,那麼我們試試7。如果我們隨機方法返回的數字等於7,那麼我們將進入if表示式產生錯誤答案的第一個分支。所以也許這裡的7可以命名為$wrongAnswerId。
1 2 3 4 5 6 |
$wrongAnswerId = 7; if (rand(0, 9) == $wrongAnswerId) { $notAWinner = $aGame->wrongAnswer(); } else { $notAWinner = $aGame->wasCorrectlyAnswered(); } |
我們的測試程式仍能通過並且程式碼有點更具表達力了。現在我們試圖給數字7命名,它將條件引入不同的上下文中。我們也可以考慮為0和9起一些合適的名字了。它們僅僅是rand()方法的引數,所以變數可能命名為min-什麼和max-什麼。
1 2 3 4 5 6 7 8 |
$minAnswerId = 0; $maxAnswerId = 9; $wrongAnswerId = 7; if (rand($minAnswerId, $maxAnswerId) == $wrongAnswerId) { $notAWinner = $aGame->wrongAnswer(); } else { $notAWinner = $aGame->wasCorrectlyAnswered(); } |
現在就很有表達力了。我們有了一個minAnswerID,一個maxAnswerID和另一個wrongAnswerId。謎題解決了。
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 |
do { $dice = rand(0, 5) + 1; $aGame->roll($dice); $minAnswerId = 0; $maxAnswerId = 9; $wrongAnswerId = 7; if (rand($minAnswerId, $maxAnswerId) == $wrongAnswerId) { $notAWinner = $aGame->wrongAnswer(); } else { $notAWinner = $aGame->wasCorrectlyAnswered(); } } while ($notAWinner); |
但注意所有的程式碼都在do-while迴圈中。我們需要每次重設回答ID變數嗎?我認為不需要。讓我們試著將它們移出迴圈然後看看我們的測試程式能不能通過。
1 2 3 4 5 6 7 8 9 10 11 12 13 14 |
$minAnswerId = 0; $maxAnswerId = 9; $wrongAnswerId = 7; do { $dice = rand(0, 5) + 1; $aGame->roll($dice); if (rand($minAnswerId, $maxAnswerId) == $wrongAnswerId) { $notAWinner = $aGame->wrongAnswer(); } else { $notAWinner = $aGame->wasCorrectlyAnswered(); } } while ($notAWinner); |
是的,測試程式這樣也能通過。
是時候轉向Game.php並且看看那兒的魔術常量了。如果你有程式碼高亮功能,你一定有些鮮亮的顏色突顯常量。我的是藍色的並且他們很容易找到。
在這段for迴圈中找到50這個魔術常量是非常容易的。如果我們看一下程式碼的作用,我們可以發現在for迴圈中,元素被放入幾個陣列中。所以我們有了一些列表,每個都有50個元素。每個列表代表一個問題集,變數實際上是在上面陣列類的欄位。
1 2 3 4 |
$this->popQuestions = array(); $this->scienceQuestions = array(); $this->sportsQuestions = array(); $this->rockQuestions = array(); |
那麼,50能代表什麼?我猜你已經有了些想法。命名是程式設計中最難的任務之一,如果你有不止一個想法而你不確定該選擇哪一個的時候,不要感到難為情。我的腦海中也有許多名字並且我正在評估選擇最好的一個的可能性,甚至是在寫這段的時候。我認為我們可以為50選擇一個保守的名字。有時是$questionsInEachCategory或者$categorySize或者一些相似的。
1 2 3 4 5 6 7 |
$categorySize = 50; for ($i = 0; $i < $categorySize; $i++) { array_push($this->popQuestions, "Pop Question " . $i); array_push($this->scienceQuestions, ("Science Question " . $i)); array_push($this->sportsQuestions, ("Sports Question " . $i)); array_push($this->rockQuestions, "Rock Question " . $i); } |
這看起來很像樣。我們可以保留它。測試程式當然也是通過的。
1 2 3 |
function isPlayable() { return ($this->howManyPlayers() >= 2); } |
2是什麼?我確信這時候答案對你已經顯而易見了。很容易:
1 2 3 4 |
function isPlayable() { $minimumNumberOfPlayers = 2; return ($this->howManyPlayers() >= $minimumNumberOfPlayers); } |
你這麼認為嗎?如果你有更好的想法,請在下面自由評論。你的測試程式呢?他們仍然能通過嗎?
現在,在roll()方法中我們也有一些數字:2,0,11和12。
1 |
if ($roll % 2 != 0) |
這非常清晰。我們將把表示式放進方法中,不過不在這篇教程中。我們仍然在理解和搜尋魔術常量和字串的階段。那麼11和12怎麼樣?它們被埋在if表示式的第三層。很難理解它們代表什麼。也許我們該看看它們周圍的程式碼。
1 2 3 |
if ($this->places[$this->currentPlayer] > 11) { $this->places[$this->currentPlayer] = $this->places[$this->currentPlayer] - 12; } |
如果當前玩家的地方或位置大於11,那麼它的位置將減少為現在的數字減去12。這聽起來像是當玩家到達告示板或遊戲區的末尾,並且它會被重新定位到初始位置。也許是位置0。或者,如果我們的遊戲告示板是圓形的,那麼走到最後一個位置的將使玩家置於相對的第一個位置。所以11可能是告示板的長度。
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 |
$boardSize = 11; if ($this->inPenaltyBox[$this->currentPlayer]) { if ($roll % 2 != 0) { // ... // if ($this->places[$this->currentPlayer] > $boardSize) { $this->places[$this->currentPlayer] = $this->places[$this->currentPlayer] - 12; } // ... // } else { // ... // } } else { // ... // if ($this->places[$this->currentPlayer] > $boardSize) { $this->places[$this->currentPlayer] = $this->places[$this->currentPlayer] - 12; } // ... // } |
別忘了在方法中要替換兩處11。這將迫使我們將變數賦值移到if表示式外面,就在第一個縮排級別。
那麼如果11是告示板的長度,那麼12是什麼?我們從當前玩家位置減12,而不是11。並且為什麼我們不直接將位置設為0來替代減法?因為這可能使測試程式失敗。我們之前的猜想,當在if表示式中的程式碼執行後,玩家將在位置0終結是錯誤的。讓我們假設玩家在位置10,骰子是4。14比11大,那麼減法就會執行。然後玩家的終結位置是10+4-12=2。
這將促使我們走向對11和12的另一個可能的命名。我認為將12命名為$boardSize更合適。但這麼做對於11來說留給我們的是什麼?也許是$lastPositionOnTheBoard?有點長,但它至少告訴了我們這個魔術常量的真相。
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 |
$lastPositionOnTheBoard = 11; $boardSize = 12; if ($this->inPenaltyBox[$this->currentPlayer]) { if ($roll % 2 != 0) { // ... // if ($this->places[$this->currentPlayer] > $lastPositionOnTheBoard) { $this->places[$this->currentPlayer] = $this->places[$this->currentPlayer] - $boardSize; } // ... // } else { // ... // } } else { // ... // if ($this->places[$this->currentPlayer] > $lastPositionOnTheBoard) { $this->places[$this->currentPlayer] = $this->places[$this->currentPlayer] - $boardSize; } // ... // } |
我知道,我知道!有些程式碼是重複的。很明顯,特別是剩下的程式碼隱藏了。但請記住我們在處理魔術常量。也會有時間處理重複程式碼的,但不是現在。
寫在最後
我在程式碼裡留了最後一個魔術常量。你能找到它嗎?如果你檢視最終程式碼,它已被替換掉,不過當然這是作弊了。祝你好運找到它並感謝你的閱讀。