Как избежать спагетти-кода в result_modifier?

Question

[ragnar_ok]

Работа с result_modifier.php вынуждает писать спагетти-код. Поддерживать эту портянку становится невозможно. Как провести рефакторинг?

Кажется, что нормальным решением может быть использование фабрики компонентов (паттерн "Фабричный метод"). Создать фабрику для каждого компонента (bitrix:news.list, bitrix:news.detail ...). В result_modifier.php шаблона компонента получать из фабрики нужный класс со статическими методами для конкретно этого result_modifier.php.

Есть какой-то общепринятый подход?

Comments

[PetrPo]

паттерн "Фабричный метод" - это ООП паттерн, предполагает наличие интерфейса выполняющего какую-то задачу с объектами классов одного типа, но когда заранее неизвестно с каким именно объектом - это определяют его подклассы. Как это клеется с result_modifier остается только догадыватьься, где такую чушь вычитал?

Делай примерно также как делают битриксоиды, большая часть логики скрыта в модуле, а в result_modifier вызываются нужные методы, например можно посмотреть шаблон catalog.section bootstrap_v4

$component = $this->getComponent();
$arParams = $component->applyTemplateModifications();

Метод applyTemplateModifications определен в модуле iblock - /bitrix/modules/iblock/lib/component/base.php
Хоть здесь и присваивается возвращаемое знчение в переменную $arParams, но по факту он также модифицирует $arResult

Т.е. делаешь свой модуль, в котором описываешь свои кастомизации в классах и потом вызываешь методы в result_modifier, будет ли этот код переиспользуемым во всех компонентах или для каждого индивидуально, будет он в ООП стиле написан или это будет несколько методов в процедурном стиле - зависит уже от тебя, но по крайней мере не будет размазано по всему проекту

[ragnar_ok]

Как это клеется с result_modifier остается только догадыватьься

Подготовил вариант фабрики, чтобы показать, что я имел в виду: https://github.com/iddqdoom/bitrix-result-modifier

Прокомментируете такой подход?

// объект и di будет получен из service locator
$component = new \App\Common\Component\Component(
    [
        'news' => new App\Common\Component\Bitrix\NewsList\ComponentFactory(),
    ]
);

/** @var array $arResult */
$arResult = $component->create('news')->modifyResult($arResult);

[PetrPo]

ragnar_ok, фабрика и фабричный метод - это разные вещи.
Фабрика - это так скажем некоторая идиома ООП программирования, фабричный метод - это паттерн ООП.
Эти вещи 99% людей путают (я тоже долго путал), но тем не менее разница достаточна явная:

Рассмотрим два класса
1.

abstract class AbstractComponentFactory implements ComponentFactoryInterface
{
    public function supports(string $component): bool
    {
        return in_array($component, $this->getSupportedComponents());
    }

    abstract protected function getSupportedComponents(): array;
}

2.

final class ComponentFactory extends AbstractComponentFactory
{
    public function create(string $component): ComponentInterface
    {
        switch ($component) {
            case 'news':
                return new NewsComponent();

            default:
                throw new InvalidArgumentException(sprintf('No component supports the given name "%s".', $component));
        }
    }

    protected function getSupportedComponents(): array
    {
        return ['news'];
    }
}

1. В классе AbstractComponentFactory методы supports и getSupportedComponents не совершают никаких действий над объектами, по факту это неоправданно усложненный if
2. Т.е. для дочернего класса ComponentFactory его родительский класс AbstractComponentFactory - это такое себе непонятное зачем наследование, не несущее в себе логического профита, т.е. остается метод create и использованный в нем оператор switch/case однозначно трактует его как фабрику (не фабричный метод!!!)
3. Исходя из выше сказанного абстракция - интерфейс ComponentFactoryInterface и абстрактный класс AbstractComponentFactory - это абстракция ради абстракции

Т.е. в рамках фабрики ничего не изменится, если оставить просто

final class ComponentFactory
{
    public function create(string $component): ComponentInterface
    {
        switch ($component) {
            case 'news':
                return new NewsComponent();

            default:
                throw new InvalidArgumentException(sprintf('No component supports the given name "%s".', $component));
        }
    }
}

Остальное выглядит, как неоправданное усложнение для восприятия, возможно в реальном формате разработки какой-то профит от этого есть, но так сходу выглядит бесполезным

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

abstract class AbstractComponentFactory
{
    public function modifyResult(array $result): array
    {
        $component = $this->create();
        
        return $component->modifyResult($result);
    }

    abstract protected function create(): ComponentInterface;
}

class NewsListComponentFactory extends AbstractComponentFactory
{
    protected function create(): ComponentInterface
    {
        return new NewsListComponent();
    }
}

class NewsDetailComponentFactory extends AbstractComponentFactory
{
    protected function create(): ComponentInterface
    {
        return new NewsDetailComponent();
    }
}

class NewsListComponent implements ComponentInterface
{
    public function modifyResult(array $result): array
    {
        return [];
    }
}

class NewsDetailComponent implements ComponentInterface
{
    public function modifyResult(array $result): array
    {
        return [];
    }
}

(new NewsListComponentFactory())->modifyResult($arResult);

UPD
Добавлю, что все это не отвечает на твой изначальный вопрос "Как избежать спагетти-кода в result_modifier?", а отвечает на вопрос кто? где? как? должен создавать объекты.
А ответ на твой вопрос - это то что я написал в своем первом комменте или как предложил человек в ответах, что собственно является аналогией моего ответа. Все остальное на твоей совести, помни только что каждый созданный класс усложняет понимание твоих намерений для других программистов

[Александр Маджугин]

Все остальное на твоей совести, помни только что каждый созданный класс усложняет понимание твоих намерений для других программистов

Это очень важное замечание - попадались проекты, в которых наследования было столько что пока продерёшься сквозь них - сойдёшь с ума.
С одной стороны сложно их было все назвать бесполезными, как в данном примере - для каждого из них можно было найти оправдание, но такое иногда складывается ощущение, что люди пишут ООП ради ООП и считают чем больше они всего отнаследуют, тем круче их код.
Может быть, но рефакторить это мучительно больно, когда открываешь класс, за классом и видишь там очередное наследование. По факту получается то же спагетти, только ООП.

[PetrPo]

Александр Маджугин, полностью согласен и также согласен с суждением

ничего плохого в процедурном подходе, если все идеи реализованы в виде простых, коротких и минимально функциональных чистых статических методах

тем более что битрикс так и так заставляет в какой-то момент писать процедурный код, например в событиях

Answer 1

[Александр Маджугин]

Нет необходимости делать в виде модуля нечто под отдельный проект.
Я делаю так (и не редко встречают похожий подход):
В /local/php_interface/lib размещаю классы реализующие некоторую функциональность конкретного проекта, и подключаю его с помощью автозагрузки composer:

Тут можно разместить и классы реализующие модуль и просто обычные хелперы - не вижу ничего плохого в процедурном подходе, если все идеи реализованы в виде простых, коротких и минимально функциональных чистых статических методах.

Comments

[ragnar_ok]

Но чем такой подход отличается от функций по неймспейсам в result_modifier.php?

// local/templates/main/components/bitrix/news.list/archive/result_modifier.php

namespace App\Component\Bitrix\NewsList\Archive;

if (!defined('B_PROLOG_INCLUDED') || B_PROLOG_INCLUDED !== true) {
    die;
}

function modifyResult(array &$result): void
{
    $result = [];
}

modifyResult();

[Александр Маджугин]

Да по сути ничем.
А чем он вам не нравится?