ЗАДАЧА СОИСКАТЕЛЮ

Применяя принципы SOLID и заветы чистого кода

1. Отрефакторить метод parseDataLayerEvent

Полученный результат должен соответствовать DRY, KISS Очевидно что рефакторинг абстрактный и как-то запускаться/тестироваться не должнен. Важно понимание говнокодинка и правил написания чистого кода.

2. Рассказать о проблемах данного класса в частности и о подходе который привел к его появлению в общем.

Проблемы данного класса

Проблем много:

• Нарушения в логической структуре программы (выход из цикла по return). То есть цикл выполняется всегда 1 раз, не больше, не понятно возможен ли вариант когда цикл не выполняется вообще - сделал предположение что да, в таком случае цикл не выполняется ни разу и метод возвращает void

• Нарушение основных принципов ООП, таких как инкапсуляция ( получаем доступ к внутренним данным сторонних объектов )

• Есть подозрения, что модификатор public для всех методов просто так (вряд ли методы вызываются снаружи, но не видя другой код гарантировать это не возможно )

• из метода возвращается либо true либо void (null) - при этом внутри класса нигде не проверяется значение возврата, в связи с чем не понятно производятся ли где-то в других классах проверки возврата, из за чего приходится тащить данное поведение в рефакторинг ( Делать возврат true)

• Проблемы с неймингом, конкретно метод parseDataLayerEvent делает все что угодно, но это сложно назвать парсингом

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

• Нарушение GRASP принципа устойчивости к изменениям, low coupling и hi cohesion - в изначальном классе все перемешано в кашу - методы знают об устройстве остальных классов, получают и изменяют данные напрямую

• Нарушение принципов SOLID - пожалуй всех, класс имеет множество отвественностей, про OPEN/CLOSED - говорить нечего, заменить зависимости что-либо на классы-потомки 100% не получиться. Что касается DI - то оно используется ( передача модели в конструктор ), но лучше бы не использовалось, судя по названию - назначение данного - обработка некоторых данных, то есть это сервис-layer, который должен быть stateless и иметь и зависеть только от таких же stateless сервисов.

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

После рефакторинга

В исходном классе изменим конструктор и целевой метод

    public function __construct(
        PixelLog $pixel_log,
        PurchaseEventsProcessor $purchaseEventProcessor
    )
    {
	$this->pixel_log = $pixel_log;
        $this->purchaseEventsProcessor = $purchaseEventProcessor;
    }


    public function parseDataLayerEvent()
    {

        return $this->pиurchaseEventProcessor
            ->process($this->pixel_log);
    }