Старый код Гадкий код. Сложный код. Код спагетти. Бредовая чепуха. В двух словах, Кодекс Наследия . Это серия, которая поможет вам работать и справляться с этим.
Мы впервые встретили наш унаследованный исходный код в нашем предыдущем уроке . Затем мы запустили код, чтобы составить мнение о его базовой функциональности, и создали тестовый набор Golden Master, чтобы иметь сырую сеть безопасности для будущих изменений. Мы продолжим работу над этим кодом, и вы можете найти его в папке trivia/php_start . Другая папка trivia/php_start содержит готовый код этого урока.
Настало время первых изменений, и что может быть лучше для понимания сложной кодовой базы, чем начинать извлекать магические константы и строки в переменные? Эти, казалось бы, простые задачи дадут нам лучшее, а иногда и неожиданное понимание внутренней работы унаследованного кода. Нам нужно выяснить намерения автора исходного кода и найти правильные имена для фрагментов кода, которые мы никогда раньше не видели.
Волшебные Струны
Волшебные строки — это строки, используемые непосредственно в различных выражениях, без привязки к переменной. Этот тип строки имел особое значение для первоначального автора кода, но вместо того, чтобы назначать их переменной с хорошим именем, автор думал, что значение строки достаточно очевидно.
Определите первые строки для изменения
Давайте начнем с просмотра нашего Game.php и попробуем определить строки. Если вы используете IDE (и вам следует) или более умный текстовый редактор, способный подсвечивать исходный код, обнаружить строки будет легко. Вот изображение того, как код выглядит на моем дисплее.
Все с оранжевым — это строка. Найти каждую строку в нашем исходном коде очень просто. Поэтому убедитесь, что подсветка поддерживается и включена в вашем редакторе, каким бы ни было ваше приложение.
Первая оранжевая часть в нашем коде находится сразу в третьей строке. Однако строка содержит только символ новой строки. На мой взгляд, это должно быть достаточно очевидно, чтобы мы могли двигаться дальше.
Когда дело доходит до принятия решения о том, что извлечь и что оставить без изменений, у нас мало упоминаний, но в конце концов ваше профессиональное мнение должно превалировать. Исходя из этого, вам придется решить, что делать с каждым фрагментом кода, который вы анализируете.
|
01
02
03
04
05
06
07
08
09
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 » .
}
|
Итак, давайте проанализируем строки с 32 по 42, фрагмент, который вы видите выше. Для поп-музыки, науки и спорта есть простое объединение. Тем не менее, действие для составления строки для вопроса рок извлекается в метод. По вашему мнению, достаточно ли ясны эти конкатенации и строки, чтобы мы могли держать их все внутри нашего цикла for? Или вы думаете, что извлечение всех строк в их методы оправдало бы существование этих методов? Если так, как бы вы назвали эти методы?
Обновите Золотой Мастер и Тесты
Независимо от ответа нам нужно будет изменить код. Настало время заставить нашего Golden Master работать и написать наш тест, который фактически запускает и сравнивает наш код с существующим контентом.
|
01
02
03
04
05
06
07
08
09
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__ .
}
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" монтируется в ОЗУ, поэтому он намного быстрее файловой системы. Если вы справились хорошо, тесты должны пройти без проблем.
Очень важно помнить, чтобы пометить наш тест генератора как «пропущенный» для будущих изменений. Если вам удобнее комментировать или вообще удалять его, сделайте это. Важно, что наш Золотой Мастер не изменится, когда мы изменим наш код. Он был сгенерирован один раз, и мы не хотим его модифицировать никогда, поэтому мы можем быть уверены, что наш недавно сгенерированный код всегда сравнивается с оригиналом. Если вам удобнее делать резервную копию, перейдите к этому.
|
1
2
3
4
5
|
function testGenerateOutput() {
$this->markTestSkipped();
$times = 20000;
$this->generateMany($times, $this->gmPath);
}
|
Я просто отмечу тест как пропущенный. Это приведет к тому, что наш результат теста станет "yellow" , что означает, что все тесты пройдены, но некоторые пропускаются или помечаются как неполные.
Делая наше первое изменение
С тестами мы можем начать менять код. По моему профессиональному мнению, все строки могут быть сохранены внутри цикла for . Мы возьмем код из createRockQuestion() , переместим его в цикл for и полностью удалим метод. Этот рефакторинг называется встроенным методом .
«Поместите тело метода в тело его вызывающих и удалите метод». Мартин Фаулер
Существует определенный набор шагов для выполнения этого типа рефакторинга, как определено Мартингом Фаулером в Рефакторинге: Улучшение дизайна существующего кода :
- Убедитесь, что метод не полиморфный.
- Найти все вызовы метода.
- Замените каждый вызов на тело метода.
- Скомпилируйте и протестируйте.
- Удалите определение метода.
У нас нет подклассов, расширяющих Game , поэтому первый шаг проверяется.
В цикле for наш метод используется только один раз.
|
01
02
03
04
05
06
07
08
09
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 » .
}
|
Мы помещаем код из createRockQuestion() в цикл for в конструкторе. У нас все еще есть наш старый код. Настало время запустить наш тест.
Наши тесты проходят. Мы можем удалить наш createRockQuestion() .
|
01
02
03
04
05
06
07
08
09
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);
}
|
Наконец, мы должны снова запустить наши тесты. Если мы пропустили вызов метода, они потерпят неудачу.
Они должны пройти снова. Congrats! Мы закончили с нашим первым рефакторингом.
Другие строки для рассмотрения
Строки в методах add() и roll () используются только для их вывода с использованием echoln() . askQuestions() сравнивает строки с категориями. Это кажется приемлемым также. currentCategory() с другой стороны, возвращает строки на основе числа. В этом методе много дублирующихся строк. Изменение любой категории, кроме Rock, потребует изменения ее названия в трех местах, только в этом методе.
|
01
02
03
04
05
06
07
08
09
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»;
}
|
Я думаю, что мы можем сделать лучше. Мы можем использовать метод рефакторинга под названием « Ввести локальную переменную» и устранить дублирование. Следуйте этим рекомендациям:
- Добавьте переменную с желаемым значением.
- Найдите все варианты использования значения.
- Замените все случаи использования переменной.
|
01
02
03
04
05
06
07
08
09
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 или других выражениях. Если эти числа имеют неясное значение, нам нужно извлечь их в переменные или методы.
|
01
02
03
04
05
06
07
08
09
10
11
12
13
14
15
16
17
18
19
20
21
|
include_once __DIR__ .
$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 этот раз мы начнем с GameRunner.php и наш первый фокус — метод roll() который получает случайные числа. Предыдущий автор не хотел придавать этим числам смысл. Мы можем? Если мы проанализируем код:
|
1
|
rand(0, 5) + 1
|
Он вернет число от одного до шести. Случайная часть возвращает число от нуля до пяти, к которому мы всегда добавляем единицу. Так что, безусловно, между одним и шестым. Теперь нам нужно рассмотреть контекст нашего приложения. Мы разрабатываем игру пустяков. Мы знаем, что есть какая-то доска, на которой наши игроки должны двигаться. И для этого нам нужно бросить кости. У кубика есть шесть лиц, и он может производить числа от одного до шести. Это похоже на разумный вывод.
|
1
2
|
$dice = rand(0, 5) + 1;
$aGame->roll($dice);
|
Разве это не мило? Мы снова использовали концепцию рефакторинга «Ввести локальную переменную». Мы назвали нашу новую переменную $dice и она представляет случайное число, сгенерированное от одного до шести. Это также сделало наше следующее высказывание действительно естественным: игра, бросок костей.
Вы запускали свои тесты? Я не упоминал об этом, но мы должны запускать их как можно чаще. Если у вас нет, это было бы хорошее время для их запуска. И они должны пройти.
Таким образом, это был случай не более чем просто обмен числами с переменной. Мы взяли целое выражение, представляющее число, и извлекли его в переменную. Технически это можно считать случаем Магической Константы, но не чистым случаем. Как насчет нашего следующего случайного выражения?
|
1
|
if (rand(0, 9) == 7)
|
Это более сложно. Что такое ноль, девять и семь в этом выражении? Может быть, мы можем назвать их. На первый взгляд, у меня нет хороших идей для нуля и девяти, поэтому давайте попробуем семь. Если число, возвращаемое нашей случайной функцией, равно семи, мы введем первую ветвь оператора if которая даст неправильный ответ. Так что, возможно, наша семерка может быть названа $wrongAnswerId .
|
1
2
3
4
5
6
|
$wrongAnswerId = 7;
if (rand(0, 9) == $wrongAnswerId) {
$notAWinner = $aGame->wrongAnswer();
} else {
$notAWinner = $aGame->wasCorrectlyAnswered();
}
|
Наши тесты все еще проходят, и код несколько более выразителен. Теперь, когда нам удалось назвать наше число семь, это ставит условное в другой контекст. Мы можем думать о некоторых приличных именах для нуля и девяти также. Это просто параметры rand() , поэтому переменные, вероятно, будут называться min -thing и max-something.
|
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();
}
|
Теперь это выразительно. У нас есть минимальный идентификатор ответа, максимум один и другой для неправильного ответа. Тайна разгадана.
|
01
02
03
04
05
06
07
08
09
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 while. Нужно ли нам каждый раз переопределять переменные идентификатора ответа? Думаю, нет. Давайте попробуем вывести их из цикла и посмотреть, проходят ли наши тесты.
|
01
02
03
04
05
06
07
08
09
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 и искать там Магические Константы. Если у вас есть подсветка кода, у вас наверняка есть константы, выделенные ярким цветом. Мои голубые, и их довольно легко обнаружить.
Найти магическую константу 50 в цикле for было довольно легко. И если мы посмотрим, что делает код, то обнаружим, что внутри цикла 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);
}
|
Что два? Я уверен, что на данный момент ответ вам ясен. Это просто:
|
1
2
3
4
|
function isPlayable() {
$minimumNumberOfPlayers = 2;
return ($this->howManyPlayers() >= $minimumNumberOfPlayers);
}
|
Вы согласны? Если у вас есть идея получше, не стесняйтесь комментировать ниже. А ваши тесты? Они все еще проходят?
Теперь в методе roll() у нас также есть некоторые числа: два, ноль, 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. Это похоже на случай, когда игрок достигает конца поля или игрового поля, и он перемещается в исходное положение. позиция. Вероятно, положение ноль. Или, если наша игровая доска круговая, переход через последнюю отмеченную позицию ставит игрока в относительную первую позицию. Таким образом, 11 может быть размером доски.
|
01
02
03
04
05
06
07
08
09
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. И почему бы нам просто не установить позицию на ноль вместо вычитания? Потому что это провалит наши тесты. Наше предыдущее предположение, что игрок окажется в нулевой позиции после выполнения кода внутри оператора if , было неверным. Допустим, игрок находится на позиции десять и бросает четверку. 14 больше 11, поэтому произойдет вычитание. Игрок окажется в положении 10+4-12=2 .
Это подталкивает нас к другому возможному именованию 11 и 12. Я думаю, что более уместно назвать 12 $boardSize . Но что это оставляет нас на 11? Может быть, $lastPositionOnTheBoard ? Немного долго, но, по крайней мере, это говорит нам правду о магической константе.
|
01
02
03
04
05
06
07
08
09
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;
}
// … //
}
|
Я знаю я знаю! Там есть некоторое дублирование кода. Это вполне очевидно, особенно если скрыть остальную часть кода. Но, пожалуйста, помните, что мы были после магических констант. Будет также время для дубликата кода, но не сейчас.
Последние мысли
Я оставил последнюю магическую константу в коде. Вы можете это заметить? Если вы посмотрите на окончательный код, он будет заменен, но, конечно, это будет обманом. Удачи в поиске и спасибо за чтение.