Ревью кода на PHP: что скажет senior и как не опозориться

Введение: Почему ваш код заставляют переписывать?

Вы написали код. Он работает. Вы гордитесь собой. Но проходит code review, и senior-разработчик оставляет 15 комментариев, половина из которых начинается с «А что, если...» или «Давай перепишем это на...». Знакомо?

Ревью кода на PHP — это не про придирки. Это про выживание проекта. Senior-ы смотрят не просто на «работает/не работает», а на то, как код поведёт себя под нагрузкой, как его будет поддерживать другой разработчик и сколько времени вы потратите на поиск бага через полгода.

В этой статье мы разберём самые частые замечания, которые вы услышите на code review от опытного PHP-разработчика. Я покажу реальные примеры кода и объясню, как их исправить. После прочтения вы сможете предвосхитить 80% замечаний и писать код, который не стыдно показать.

1. «Магические числа» и строки: код должен говорить сам за себя

Самое первое, что заметит senior — это непонятные числа и строки, разбросанные по коду. Магические числа — это антипаттерн, который убивает читаемость.

Плохой пример

if ($user->getAge() > 18) {
    $discount = $price * 0.1;
}

Что скажет senior?

• Почему именно 18? Это возраст совершеннолетия или порог скидки?
• Почему 0.1? Это 10%? А если скидка изменится?

Хороший пример

const ADULT_AGE = 18;
const DISCOUNT_RATE = 0.1; // 10%

if ($user->getAge() > ADULT_AGE) {
    $discount = $price * DISCOUNT_RATE;
}

Теперь код сам рассказывает историю. Senior одобрит: константы можно переиспользовать, менять в одном месте, и они не требуют комментариев.

2. Отсутствие типов: PHP 8 — не оправдание для динамической типизации

PHP — язык со слабой типизацией, но это не значит, что нужно писать как в 2005 году. Строгая типизация спасает от багов, которые сложно отловить.

Плохой пример

function calculateTotal($items, $tax) {
    $total = 0;
    foreach ($items as $item) {
        $total += $item['price'] * $item['quantity'];
    }
    return $total * (1 + $tax);
}

Замечания senior-а

• Что такое $items? Массив? Объект? Коллекция?
• Что за структура у элемента? Почему нет DTO?
• А если передать $tax строкой "0.2"? Будет конкатенация?

Хороший пример

declare(strict_types=1);

class OrderItem {
    public function __construct(
        public readonly float $price,
        public readonly int $quantity
    ) {}
}

function calculateTotal(array $items, float $taxRate): float {
    $total = 0.0;
    foreach ($items as $item) {
        if (!$item instanceof OrderItem) {
            throw new InvalidArgumentException('Expected OrderItem');
        }
        $total += $item->price * $item->quantity;
    }
    return $total * (1 + $taxRate);
}

Senior скажет: «Молодец, используешь DTO, readonly свойства и строгую типизацию. Код предсказуем». Это best practice PHP 8+.

3. Глубокие вложенности и ранний выход (Early Return)

Когда вы видите 5 уровней if...else внутри одного метода, senior хватается за голову. Такой код невозможно читать, тестировать и дебажить.

Плохой пример

public function processOrder($order) {
    if ($order->isPaid()) {
        if ($order->hasStock()) {
            if ($order->isShippable()) {
                // 50 строк логики
                return $this->ship($order);
            } else {
                throw new \Exception('Order cannot be shipped');
            }
        } else {
            throw new \Exception('Out of stock');
        }
    } else {
        throw new \Exception('Order not paid');
    }
}

Что скажет senior?

«Это «пирамида ужаса». Используй Guard Clauses (ранний выход). Упрости логику.»

Хороший пример

public function processOrder(Order $order): void {
    if (!$order->isPaid()) {
        throw new \Exception('Order not paid');
    }

    if (!$order->hasStock()) {
        throw new \Exception('Out of stock');
    }

    if (!$order->isShippable()) {
        throw new \Exception('Order cannot be shipped');
    }

    $this->ship($order);
}

Код стал линейным, его легко читать. Каждый if — это защита, которая завершает выполнение. Senior оценит.

4. SQL-запросы в контроллерах: нарушение слоёв

Это классика. Молодые разработчики часто лепят запросы прямо в контроллеры или шаблоны. Разделение ответственности — это то, чему учат на курсах, но на практике забывают.

Плохой пример

// В контроллере
public function index() {
    $db = DB::connection();
    $users = $db->query("SELECT * FROM users WHERE active = 1");
    return view('users', ['users' => $users]);
}

Замечание senior-а

«Ты смешал бизнес-логику с инфраструктурой. Как ты будешь юнит-тестировать этот контроллер? А если мы сменим БД? Вынеси в репозиторий или модель.»

Хороший пример

class UserRepository {
    public function getActiveUsers(): Collection {
        return User::where('active', true)->get();
    }
}

// В контроллере
public function index(UserRepository $repository) {
    $users = $repository->getActiveUsers();
    return view('users', ['users' => $users]);
}

Теперь контроллер — это просто «клей» между слоями. Senior скажет: «Ты следуешь принципу Single Responsibility. Отлично.»

5. Игнорирование исключений и пустых catch-блоков

Пустой catch — это бомба замедленного действия. Вы «съедаете» ошибку, а потом ищете баг несколько часов.

Плохой пример

try {
    $this->processPayment($order);
} catch (\Exception $e) {
    // Ничего не делаем
}

Что скажет senior?

«Ты только что проглотил исключение. Если платеж не пройдет, пользователь увидит «успех», а деньги не спишутся. Либо логируй, либо