Рефакторинг компонента Битрикса

Шесть проблем (кейсов), с которыми я столкнулся на реальном проекте.

Далее - рефакторинг такого плохого кода. Не ради удовольствия, или нравоучений, а для исправления реальной ошибки на рабочем сайте, которую быстрым наскоком исправить было невозможно (и как всегда, времени в обрез).

До и после во вложении.

Проблема длинного скрипта

Эту тряпку тяжело читать, осмыслить и понять. Что делает произвольно взятый кусок кода?

:!: Кейс 1: нашли поиском переменную/метод, хотим в коде поправить, как это скажется на работе всего компонента? Смотрим по коду выше... Выше... Выше... Путаемся в куче if..else, пока окончательно не забываем, зачем это смотрим =)

Нужна разбивка этой простыни на логические элементы, чтобы увидеть картину в целом, и работать с небольшими участками кода. Я сделал ООП.

Вся работа компонента укладывается в эти понятные строки:

Bitrix компонент просто и понятно

:!: Кейс 2: от чего зависит поведение компонента? Какие зависимости у него?

В конструктор вынесены все зависимости:

Bitrix зависимости

Стиль Bitrix переменных сохранен, так как при переносе постоянно приходилось смотреть в исходный скрипт, и искать эти переменные. Просто, чтобы не запутаться. Исключение: $arTransIds переделана в $arBids, так как это название для ставок более выразительное. Читать: «Сила имен переменных» из «Совершенный код» Стива Макконнелла.

Сохраняем совместимость

Делаем Bitrix код читабельнее

Избавляемся от повторных вызовов и отрицаний

Весь код испещрен isUserLogTransporter($USER_ID).

  • :!: Кейс 3. Функция вызывается 7 раз, но фактически возвращает одно и то же значение (так как в контексте пользователь один и тот же). Это накладно по производительности, надо исправить.
  • :!: Кейс 4. Трудно понимать отрицание !isUserLogTransporter($USER_ID). Если пользователь не перевозчик, то кто он?

В конечном счете заменено на это:

$this->iAmTransporter = isUserLogTransporter($this->userID);
$this->iAmAdmin       = !$this->iAmTransporter;

Ведь проверка if($this->iAmAdmin) понятнее чем !isUserLogTransporter($USER_ID)?

Используем ссылки

:!: Кейс 5. Когда привык читать код «по-диагонали», а тут такое, весь мозг сломаешь:

Программирование - трудная для понимания строка

Такие ссылки качественно облегчают чтение.

Сокращаем код

Короткий код проще понять. И найти ошибки. Тут и баг пофиксил:

Короткий код быстрее понимается

Избавляемся от тупого кода Bitrix

:!: Кейс 6. Под действием каких веществ писался этот код? И я долго соображал, что же должно произойти, чтобы не выполнилось +3 часа:

Индусский код

Вот действительно заставили меня подумать, «а вдруг»? Может, конечно, сначала везде стояли разные цифры, потом их все перебили на одну, но зачем это держать? Изучайте git, товарищи, если хотите иметь историю правок.

Еще один случай, когда есть знания синтаксиса, но нет понимания «можно проще»: