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

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

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

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

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

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

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

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

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

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

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

Весь код испещрен 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. Когда привык читать код «по-диагонали», а тут такое, весь мозг сломаешь:

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

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

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

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

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