Другой взгляд на эволюцию гадкого утёнка или рефакторинг спагетти

Я с большим энтузиазмом отношусь к любым попыткам улучшить общую культуру разработки в таком неоднозначном сообществе, как сообщество PHP-разработчиков. Но видя некоторые инициативы хочется убиться об стену их немного поправить, чтобы не дай бог не пришлось работать с кодом, сильно отличающимся от моих представлений о совершенном коде.

Сегодня почему-то не сдержался, увидев топик Из гадкого утёнка в лебедя, или как исправить криворукий код и решил его исправить по-своему. Тем более автор попросил дать ссылки на другие варианты решений.


Напомню, что стоит задача причесать код

<h1>Пользователи</h1>
<?
$DB = new DBConnector;
$DB->query(‘SELECT * FROM users LIMIT 10’);
if($DB->get_num_rows()){
    while($user = $DB->fetch_row()){
        echo ‘<p>’.$user[‘name’].’</p>’;
    }
}
?>

С чего, по-моему, должен начинаться рефакторинг — с покрытием тестами кода, который собираемся рефакторить. Без этого мы не можем быть уверены, что всё делаем правильно и поведение кода не изменяется. Поскольку пример учебный, то не будет обрабатывать особые случаи (нет базы данных и т. п.) и будем считать, что код выводит имена четырех пользователей из БД: Username1, Username2,…

Пишем простенький тест прямо на PHP без использования фреймворков типа PHPUnit.

<?php
$expected = <<<'EOT'
<h1>Пользователи</h1>
<p>Username1</p><p>Username2</p><p>Username3</p><p>Username4</p>
EOT;

ob_start();
include 'index.php';
$actual = ob_get_clean();

echo $actual === $expected ? 'OK' : 'Failed', PHP_EOL;

Запускаем… и получаем ошибку «компиляции» из-за отсутствия класса DBConnector. Не мудрствуя лукаво пишем заглушку для него (на «боевом» рефакторинге пришлось бы его подключать, создавать тестовые таблицы и т. п., но пост не о методиках тестирования). Приблизительно так я представляю работу оригинального класса DBConnector :)

<?php
class DBConnector
{
    private $users;

    public function query($query)
    {
        for ($i=1; $i<=4; $i++) {
            $this->users[] = array('name' => 'Username' . $i);
        }
    }

    public function get_num_rows()
    {
        return count($this->users);
    }

    public function fetch_row()
    {
        $each = each($this->users);
        return $each['value'];
    }
}

Опять запускаем тест — получаем ОК. Поведение скрипта мы зафиксировали, теперь если нарефакторим что-то не то, то сразу получим Failed. Напомню, что на настоящем рефакторинге нам надо будет покрыть тестами все возможные потоки исполнения, скажем исключение или ошибки при работе с БД. Тут же ограничились одним.

Смотрим на наш код. Первое что лично мне бросается в глаза — всё перемешано заголовок, запрос к БД, вывод. Решено, разделяем на получение записей и вывод html.

<?php
include 'DBConnector.php';
$DB = new DBConnector;
$DB->query('SELECT * FROM users LIMIT 10');
if($DB->get_num_rows()){
    while($user = $DB->fetch_row()){
        $users[] = $user;
    }
} else {
    $users = array();
}
?>
<h1>Пользователи</h1>
<?
foreach ($users as $user) {
    echo '<p>'.$user['name'].'</p>';
}
?>

Запускаем тест — OK, ничего не сломали. Разделяем для удобства на два файла, иными словами выносим всё что связано с html в отдельный файл index.php.html:

<?php
// index.php
include 'DBConnector.php';
$DB = new DBConnector;
$DB->query('SELECT * FROM users LIMIT 10');
if($DB->get_num_rows()){
    while($user = $DB->fetch_row()){
        $users[] = $user;
    }
} else {
    $users = array();
}
include 'index.php.html';

<h1>Пользователи</h1>
<?
// index.php.html
foreach ($users as $user) {
echo '<p>'.$user['name'].'</p>';
}
?>

Проверяем — ОК. Некоторые умные люди то, что у нас сейчас в новом файле называют представлением или видом. Но про него пока забываем. Хотя нет, сделаем его чуть посимпатичнее (на вкус и цвет...).

<h1>Пользователи</h1>
<?php foreach ($users as $user): ?>
    <p><?=$user['name']?></p>
<?php endforeach; ?>

Запускаем тест — Failed! Мы изменили представление и тест не проходит. Но мы знаем, что в HTML пробелы не значимы и добавляем их в тест (никогда так не делайте! :) ). Теперь всё ОК. Возвращаемся к нашему скрипту. Бросается в глаза, что он сильно зависит от базы данных. И не исключено, что в других скриптах нашего приложения такие выборки повторяются часто. Да и вообще как-то все эти $DB мельтешат в глазах и надо разбираться что они делают. Ну что же, следуем как бы DRY и выносим работу с таблицей users в отдельный метод отдельного класса, соединение с БД передадим через конструктор, не глобальную же переменную делать. Назовём класс, ну, скажем, UserRepository.

<?php
class UserRepository
{
    private $db_connector;

    public function __construct(DBConnector $db_connector)
    {
        $this->db_connector = $db_connector;
    }

    public function getAll($limit=10)
    {
        $this->db_connector->query("SELECT * FROM users LIMIT {$limit}");
        if($this->db_connector->get_num_rows()){
            while($user = $this->db_connector->fetch_row()){
                $users[] = $user;
            }
        } else {
            $users = array();
        }
        return $users;
    }
}

Код перенесён почти без изменений, добавлен только параметр limit. Машинально добавил :(, по-хорошему нельзя было. Но изменение простейшее и надеемся, что ничего не сломается. Но сначала нам надо изменить наш скрипт

<?php
require_once 'DBConnector.php';
require_once 'UserRepository.php';

$user_repository = new UserRepository(new DBConnector);
$users = $user_repository->getAll();

include 'index.php.html';

Тест? OK! (На каждом шагу рефакторинга нужно запускать тесты, чтобы потом не было обидно за бесцельно прожитые годы в поисках «где же напортачили»). Если отбросить «служебные» require_once, то файл нашего скрипта сократился до трех строчек: создаём репозиторий, получаем из него всех пользователей, выводим их в представлении. Ежу, наверное понятно будет с первого взгляда. Умные люди такой скрипт называют контроллером, причём тонким. Ну это так, для справки.

Взглянем ещё раз на наш репозиторий. Ни у кого не возникает диссонанс — создали класс «Репозиторий пользователей», объекты которого возвращают массив ассоциативных массивов (aka хэшей)? У меня возникает. Пускай он возвращает хотя бы массив объектов с оригинальным именем User.

            while($user = $this->db_connector->fetch_row()){
                $users[] = new User($user);
            }

Ну и сам класс User, который умные люди называют классом модели предметной области или просто классом модели.

<?php
class User
{
    private $name;

    public function __construct(array $properties)
    {
        $this->name = $properties['name'];
    }

    public function getName()
    {
        return $this->name;
    }
}

Изменяем представление для работы с массивом объектов

<h1>Пользователи</h1>
<?php foreach ($users as $user): ?>
    <p><?= $user->getName() ?></p>
<?php endforeach; ?>

и добавляем декларацию класса модели в контроллер.

<?php
require_once 'DBConnector.php';
require_once 'User.php';
require_once 'UserRepository.php';

$user_repository = new UserRepository(new DBConnector);
$users = $user_repository->getAll();

include 'index.php.html';

Запускаем тест — всё ОК! Пожалуй на этом можно пока остановиться. Подведём итоги:

— поведение скрипта практически гарантированно не изменилось. Не считая нескольких проблеов появление которых мы заметили и задокументировали. Где? В тесте! Тест — это и документация к основному коду.

— представление у нас отделено от всего остального, всё что ему нужно, чтобы был в его области видимости массив users из объектов с методом getName(). Можем тестировать его отдельно.

— сложная (хе-хе) работа с базой данных у нас инкап��улирована в репозитории что не отвлекает от изучения логики приложения, но само соединение создаётся вне его, что даёт возможность а) подставлять соединения с разными БД и б) подставлять по настоящему фэйковые (стабы и моки) экземпляры соединений для тестирования и даже в) почти ничего не изменяя использовать любое другое хранилище — файлы, NoSQL СУБД и даже файлопомойки облачные файлохостинги.

— объекты модели по сути не зависят от БД вообще, да и вообще ни от чего, это так называемые POPO — Plain Old PHP Objects (плоские старые PHP объекты). Пока по сути никакой логики в них нет, но когда появится её также можно будет тестировать отдельно от всего остального.

— работа основного скрипта почти очевидна, только последний include почти ни о чём не говорит, но выделять его в функцию/метод мне уже было лень

Что ещё можно сделать с кодом для улучшения усложнения архитектуры:

— ещё больше абстрагироваться от БД, а то и вообще типа хранилища.

— абстрагироваться от типа представления (нашего include) — можно сделать, чтобы без проблем оно рендерилось каким-нибудь шаблонизатором — Smarty, Twig и т. п.

— сделать контроллер тоже объектом класса

— использовать паттерн FrontController

— без особого труда прикрутить другие сторонние компоненты (ORM, роутеры, конфиги и т. п.)

— перевести код на фреймворк, например Symfony2 :)

— покрыть нормальными тестами, от модульных (юнит) до приёмочных.

Если есть интерес, то это может быть первой статьёй небольшого цикла. Только сначала бы не помешало усложнить исходный пример говнокода до чего-то более-менее работающего и хоть немного неочевидного и говнистого :) Если есть желающие то реп на гитхабе github.com/VolCh/refactor-sample