Junior: питання на співбесіді з теми «Рефакторинг і зв'язність»

Code smell («запах коду») - ознака, що в коді, ймовірно, є проблема дизайну. Сам по собі не баг - код працює, - але його важко читати, змінювати й тестувати.

Найпоширеніші:

• Довгий метод. Метод на 100+ рядків, який робить кілька речей. Лікується виділенням методів з іменами, що пояснюють намір.
• Великий клас («God object»): тисячі рядків, десятки залежностей. Розділити за відповідальностями.
• Дублювання: однакова логіка в кількох місцях - зміну доведеться робити скрізь, і одне місце точно забудуть.
• Довгий список параметрів: createUser($name, $email, $phone, $role, $team, $sendEmail, $isAdmin). Об'єкт-параметр (DTO) чи кілька методів.
• Прапорець-параметр: render(true) - незрозуміло без заглядання всередину. Два методи чи іменований аргумент.
• Одержимість примітивами: гроші як float, email як string, статус як магічний рядок. Value objects і enum.
• Заздрість до функцій (feature envy): метод постійно звертається до даних іншого класу - можливо, він має жити там.
• Розгалуження за типом: однаковий switch ($type) у кількох місцях - кандидат на поліморфізм.
• Магічні числа: if ($status === 3), sleep(86400).
• Коментар, що пояснює заплутаний код, - часто краще переписати код так, щоб пояснення стало зайвим.

Як з ними працювати: запах - привід придивитися, а не вимога негайно переписувати. Рефакторять тоді, коли код доводиться змінювати: «правило бойскаута» - залишити код трохи кращим, ніж знайшли.

Частину запахів знаходять інструменти: PHPStan/Larastan, PHP Mess Detector, метрики складності в IDE.

Докладніше в документації: Code smells

Рефакторинг - зміна структури коду без зміни поведінки. Без тестів неможливо переконатися, що поведінка справді не змінилася. Тому перший крок - створити страховку.

1. Тести-характеристики (characterization tests). Не «як має працювати», а «як працює зараз», включно з дивацтвами. Викликати код з різними входами, записати результати й зафіксувати їх у тестах.

it('рахує знижку так само, як до рефакторингу', function (array $order, int $expected) {
    expect(DiscountCalculator::calculate($order))->toBe($expected);
})->with([
    [['total' => 1000, 'vip' => false], 0],
    [['total' => 1000, 'vip' => true], 100],
    [['total' => 5000, 'vip' => false], 250],
]);

2. Тести на верхньому рівні, якщо код важко тестувати окремо: feature-тест HTTP-ендпоінту, що перевіряє відповідь і зміни в базі. Грубо, але надійно.

3. Маленькі кроки. Перейменувати змінну, виділити метод, перенести метод - по одній зміні, з прогоном тестів після кожної. Невдалий крок легко відкотити.

4. Автоматичні рефакторинги IDE та Rector. «Rename», «Extract Method», «Move» у PhpStorm змінюють усі посилання коректніше, ніж ручні правки. Rector автоматизує масові зміни.

5. Окремо рефакторинг, окремо нові функції. Не змішувати в одному коміті: якщо щось зламалося, видно, що саме.

6. Статичний аналіз (PHPStan/Larastan) ловить зламані виклики й типи, яких тести можуть не покрити.

Чого не робити: «великого переписування» без тестів - найризикованішого сценарію. Краще поступово: нова структура поруч зі старою, поступове перенесення (патерн «Strangler Fig»), старий код видаляється, коли на нього вже ніхто не посилається.

Докладніше в документації: Як рефакторити

Рефакторинг - зміна внутрішньої структури коду без зміни його поведінки. «Виділити метод» (Extract Method / Extract Function) - найчастіший з них: фрагмент коду переноситься в окремий метод з назвою, що пояснює навіщо він.

До:

public function checkout(Cart $cart, User $user): Order
{
    // перевіряємо, що всі товари в наявності
    foreach ($cart->items as $item) {
        if ($item->product->stock < $item->qty) {
            throw new OutOfStock($item->product);
        }
    }

    // рахуємо суму зі знижкою
    $total = $cart->items->sum(fn ($i) => $i->product->price * $i->qty);
    if ($user->isVip()) {
        $total = (int) round($total * 0.9);
    }

    return Order::create(['user_id' => $user->id, 'total' => $total]);
}

Після:

public function checkout(Cart $cart, User $user): Order
{
    $this->ensureInStock($cart);

    return Order::create(['user_id' => $user->id, 'total' => $this->totalFor($cart, $user)]);
}

private function ensureInStock(Cart $cart): void { /* ... */ }
private function totalFor(Cart $cart, User $user): int { /* ... */ }

Коли виділяти:

• коментар пояснює, що робить блок коду - назва методу може замінити коментар;
• метод не вміщується на екран чи має кілька рівнів вкладеності;
• той самий фрагмент повторюється в кількох місцях;
• змішано рівні абстракції: бізнес-кроки поруч із деталями (цикли, форматування, SQL);
• блок потрібно тестувати окремо.

Як робити безпечно:

1. переконатися, що є тести (або написати їх для поточної поведінки);
2. виділити метод - автоматично в IDE (PhpStorm: Extract Method), щоб не помилитися з параметрами й поверненим значенням;
3. дати назву за наміром («чи в наявності», «сума зі знижкою»), а не за реалізацією («цикл по товарах»);
4. запустити тести.

Коли виділення не допомагає: якщо виділеному фрагменту потрібно передати шість параметрів і повернути три значення - це ознака, що код хоче стати окремим класом (Extract Class), а не методом.

Зворотний рефакторинг - Inline Method - коли метод-обгортка нічого не пояснює і лише ускладнює читання.

Докладніше в документації: Каталог рефакторингів: Extract Function

Код читають набагато частіше, ніж пишуть. Перейменування - найдешевший рефакторинг з найбільшим ефектом на читабельність: назва, що точно описує сутність, позбавляє потреби читати реалізацію.

Ознаки поганих назв:

$data = $this->get($id);         // які дані? що «отримати»?
$flag = true;                    // який прапорець?
$tmp, $res, $arr, $obj;          // назви за типом, а не за змістом
function process($x) {}          // «обробити» - що саме?
$d = 30;                         // 30 чого? днів? хвилин?

Як краще:

$invoice = $this->findInvoice($invoiceId);
$isRegisteredForDiscounts = true;
function sendOverdueReminders(Collection $invoices): void {}
$gracePeriodInDays = 30;

Правила, що допомагають:

• за наміром, а не за реалізацією: activeSubscribers(), а не getUsersWhereStatusIsOneAndPaidIsTrue();
• булеві значення - питанням: isPaid, hasAccess, canPublish, shouldRetry;
• методи - дієсловом: calculateTotal(), sendInvoice(); класи й змінні - іменником;
• одиниці виміру в назві, якщо тип їх не передає: timeoutSeconds, priceCents, sizeInBytes;
• мова предметної області: якщо бізнес каже «замовлення», «відвантаження», «повернення» - у коді Order, Shipment, Refund, а не Item, Process, Operation;
• послідовність: не fetch/get/retrieve/load для того самого типу дії в різних місцях;
• довжина пропорційна області видимості: $i у короткому циклі - нормально; змінна, що живе в усьому класі, - описова назва.

Перейменування безпечне з інструментами: IDE (Rename у PhpStorm) змінює всі використання, включно з рядковими посиланнями й документацією; статичний аналіз (PHPStan) і тести ловлять пропущене.

Пастки:

• публічні API (маршрути, поля JSON, назви колонок, події) - перейменування ламає клієнтів і потребує міграції чи періоду сумісності;
• рядкові посилання (config('services.old_name'), назви в Blade, $model->getAttribute('field')) IDE може не знайти;
• магічні методи й динамічні виклики - перевіряти тестами.

Корисне правило: якщо назву важко придумати, часто проблема не в назві, а в коді - метод чи клас робить забагато різного.

Докладніше в документації: Каталог рефакторингів: Rename Variable

DRY (Don't Repeat Yourself) - кожне знання в системі має мати одне джерело. Дублювання шкодить, бо зміну доводиться вносити в кількох місцях, і рано чи пізно одне з них пропускають.

Але не всяке схоже код - дублювання знання. Два фрагменти можуть виглядати однаково випадково, а змінюватися з різних причин:

// валідація реєстрації
'name' => ['required', 'string', 'max:255'],

// валідація назви товару
'name' => ['required', 'string', 'max:255'],

Це не одне знання: правила для імені користувача й назви товару завтра розійдуться. Спільна функція nameRules() їх штучно зв'яже.

«Неправильна абстракція» (Сенді Метц) - типовий сценарій:

1. програміст бачить повтор і виносить код у спільну функцію чи базовий клас;
2. з'являється новий випадок, що «майже підходить» - додають параметр;
3. ще один випадок - ще параметр, умова всередині;
4. через рік абстракція має прапорці $isAdmin, $skipValidation, $legacyMode, і ніхто не розуміє, як вона працює, а змінювати страшно, бо вона використовується скрізь.

Висновок Метц: «Дублювання значно дешевше за неправильну абстракцію». Якщо абстракція обросла параметрами й умовами, вигідніше повернути код назад (вбудувати в місця використання) і заново подивитися, що справді спільне.

Практичні правила:

• правило трьох: перший раз - пишемо, другий - терпимо дублювання, третій - виносимо абстракцію. До третього разу видно, що справді спільне;
• дублюється знання чи текст? Якщо обидва місця змінюватимуться разом з тієї самої причини - це знання, його варто об'єднати;
• ознаки поганої абстракції: булеві параметри, що вмикають гілки; if ($type === ...) усередині «спільного» коду; коментарі «для X не викликати»;
• дублювання в тестах часто корисне: тест, який читається сам по собі без переходів у допоміжні функції, - кращий тест.

WET («Write Everything Twice») і AHA («Avoid Hasty Abstractions») - жартівливі назви того самого принципу: не поспішати з абстракціями, доки не стане зрозуміло, яка вона має бути.

Докладніше в документації: Сенді Метц: The Wrong Abstraction