SOLID на практике. Принцип открытости-закрытости и ActiveQuery Yii2

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

  • Реализация принципов и паттернов требует слишком много времени. 

  • Код становится тяжеловесным и сложным для понимания. 

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

Отправная точка

Работаем над проектом на Yii2, в котором для доступа к данным используется ActiveRecord. Клиентский код загружает некий набор данных, используя метод ActiveRecord::find()

class ClientClass  
{ 

    // Много кода... 

    public function buildQuery(): ActiveQueryImplementation 
    { 
        $query = ActiveRecordModel::find(); // Получаем экземпляр ActiveQuery из модели 
        $query->active()->unfinished(); // Применяем условия, реализованные в конкретном классе ActiveQuery         

        return $query; // Далее результаты построения ActiveQuery используются для получения выборки из БД, например $query->all(); 
    } 

    // Много кода... 

} 

Этот код применяет к экземпляру, реализовывающему  ActiveQueryInterface, фиксированный набор условий и возвращает сконфигурированный таким образом экземпляр для дальнейшего использования. 

А если нужно добавить новые условия в запрос?

Придерживаясь традиционного подхода, мы добавим вызовы методов с новыми условиями прямо в строку, применяющую эти условия к запросу. 

$query->active()->unfinished()->newConditionA()->newConditionB(); 

Вуаля! Пять секунд работы, и мы получили изящный, легко читаемый код. 

Но что если эти условия нужны не во всех случаях, когда вызывается наш метод? Что если условия к запросу нужно применять динамически? 

Тут нас ждут определенные трудности. Очевидно, что при таком подходе весь клиентский код, использующий наш метод, будет получать запрос, к которому уже применены новые условия. Почему так получилось? Потому что мы... 

Нарушаем принцип открытости-закрытости

 Напомню, что принцип открытости-закрытости SOLID гласит: 

Код должен быть открытым для дополнения и закрытым для изменения. 

Мы изменили код нашего метода таким образом, что он начал выдавать другие результаты.  

А как сделать правильно? 

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

Для этого изменим код нашего метода следующим образом. 

class ClientClass  
{ 
    /** 
     * @var ActiveQueryFilter[] 
     */ 
    public $filters = []; // Добавляем возможность сконфигурировать экземпляр класса массивом фильтров 

    // Много кода... 

    public function buildQuery(): ActiveQueryImplementation
    { 
        $query = ActiveRecordModel::find(); 
        $query->active()->unfinished();   
        $this->applyFilters($query); // Применяем фильтры к запросу 

				return $query; 
    } 

    private function applyFilters(ActiveQueryImplementation &$query): void  
    { 
        foreach ($this->filters as $filter) { 
            $filter->applyTo($query); 
        } 
    } 

    // Много кода... 

} 

Определим интерфейс ActiveQueryFitler предполагает единственный метод applyTo(), применяющий дополнительные условия к запросу в качестве побочного эффекта. 

interface ActiveQueryFilter 
{ 
    public function applyTo(ActiveQuery $query): void; 
} 

Теперь для добавления в запрос новых условий нам достаточно добавить соответствующую реализацию интерфейса ActiveQueryFilter

class NewConditionsAAndBFilter implements ActiveQueryFilter 
{ 
    public function applyTo(ActiveQuery $query): void  
    { 
        $query->newCondtionA()->newConditionB(); 
    } 
} 

Что мы имеем в итоге 

  • Мы решили стоящую перед нами задачу, дополнив первоначальный метод всего одним вызовом (минимальное вмешательство в первоначальный код). 

  • Поведение по умолчанию исходного метода не изменилось. 

  • Вызываемый из исходного метода новый метод applyFilters() не реализовывает собственной логики — вся логика делегируется классам фильтров. Таким образом, мы оставили неизменным и поведение всего исходного класса. 

Комментарии 39

    0
    Остались нераскрытыми вопросы, как правильно заполнять filters (например фабричный метод) и как защититься от того, что в него можно добавить экземпляр не реализующий этот интерфейс.
      0
      Очень резонные вопросы.
      как правильно заполнять filters (например фабричный метод)

      Тут есть варианты. Например, в нашем проекте, который я брал за основу, фильтры заполняет именно фабричный метод. Можно конфигурировать через конструктор. Можно сделать сеттеры. Можно сделать абстрактную фабрику и т. д.
      как защититься от того, что в него можно добавить экземпляр не реализующий этот интерфейс

      Тут опять же есть варианты. Метод типа addFilter(ActiveQueryFilter $filter). Либо сеттер с проверкой типов. Либо проверка предусловий перед применением фильтров. Этот вопрос, пожалуй, мог бы протянуть на отдельную статью.
        0
        Подскажите, пожалуйста, а как у вас реализуется фабричный метод?
        $filter = ArticleFilterFactory->active();
        

        Или например:
        $filter = ActiveFilter::create();
        

          0
          В моем случае фабричный метод работает не для фильтров, а для ClientClass. Он выбирает необходимые фильтры и конфигурирует ими экземпляр клиентского класса.

          $client = ClientClass::create();
            0
            Создание экземпляра класса ClientClass происходит в его статическом методе create? А как его в таком случае инициализировать, если зависимости инжектятся в конструктор? Брать зависимости напрямую из контейнера? Но ведь это антипаттерн.
              0
              Зависимости можно прокидывать, например, через аргументы фабричного метода.
                0
                Брать зависимости напрямую из контейнера? Но ведь это антипаттерн.

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


                $container->register(
                  ClientClassInterface::class,
                  function (\Psr\Container\ContainerInterface $container): ClientClassInterface {
                    return ClientClass::create(
                      $container->get(Psr\SimpleCache\CacheInterface::class),
                      $container->get(Psr\Log\LoggerInterface::class)
                    );
                 }
                );

                вполне нормально

                  0
                  Но если использовать для инициализации объекта контейнер, то надобность в статическом методе create отпадает, тогда проще сделать так:
                  $container->register(
                      ClientClassInterface::class,
                      function (\Psr\Container\ContainerInterface $container): ClientClassInterface {
                  	    return new ClientClass(
                  		    Psr\SimpleCache\CacheInterface::class,
                  		    Psr\Log\LoggerInterface::class
                  	    );
                      }
                  );
                  
                    0

                    В данном конкретном случае почти дело вкуса, использовать именованный конструктор (статический фабричный метод) или обычный, но не всегда это так.

                      0
                      А можете привести пример, когда в контейнере оправдано создание экземпляра класса именно статическим фабричным методом?
                        +1

                        Когда этот метод содержит не просто проброс параметров в конструктор, а более-менее сложную логику по формированию параметров конструктора и/или другой инициализации, особенно если нужны разные логики для разных кейсов. От "параметрической перегрузки" типа createForCaseA(param1, param2, param3) и createForCaseB(param3) до сколь угодно сложной логики, которая усложнит код и конструктора (особенно в свете нового сахара для конструкторов), и контейнера.


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

        0
        С открытостью для расширения понятно, а где закрытость для изменения?
          –1
          Закрытость — она больше в голове, на уровне соглашений.
          В данном случае мы делаем удобнее сделать правильно (написать класс фильтра), чем неправильно (лезть с правками в исходный метод). Но, конечно, при большом желании накосячить никто никому не запретит.
          0
          Основных причин две
          Есть еще третья, наверное даже более влиятельная чем эти. Кривые архитектура/API сторонних систем, фреймворков, и библиотек с которыми приходится работать. Очень сложно написать красивую иерархию классов, если строительные блоки это косплей на спагетти. Причем зачастую они созданы такими не просто так, а например в угоду эффективности выполнения.
            0
            Внешний код — не большая проблема. Мы всегда можем изолироваться от него. А вот поддержание чистоты в собственной кодовой базе — это уже наша ответственность.
              +1

              Сложно бывает изолироваться от "моделей" — типовых таких реализаций ActiveRecord, by design нарушающих SRP, а частенько и DIP, ISP

                0
                Можно. В комментарии ниже, например, предлагают использовать репозитории.
                  0

                  Я не сказал "нельзя", я сказал "сложно". И репозиторий обычно возвращает значение как инстанс ActiveRecord, на котором можно вызывать все методы типа save(), delete(), а в некоторых реализациях и всякие newQuery и получить доступ к коннекшену с БД — это изоляция?

                    –1
                    репозиторий обычно возвращает значение как инстанс ActiveRecord

                    Да, часто именно так и происходит. Потому что так удобно.

                    Тут вопрос приоритетов: либо нам нужно быстро, тогда ActiveRecord, либо нам нужно надолго, тогда делаем модели пассивными, а вся работа с хранилищем идёт через репозитории.
                  0
                  Кстати, Samdsrk на одной из недавних конференций признавал ActiveRecord не самым удачным патентном с точки зрения чистоты кода.
                    0

                    Так он нарушает SRP по определению. Две ответственности: представлять сущность и управлять хранением этой сущности в базе.

                      0
                      Так и есть. Поэтому, если мы придерживаемся принципов чистого кода, нам приходится изолировать нашу систему от грязного внешнего мира. А для этого абстракции нам в помощь.
                        0

                        Чтобы изолироваться от ActiveRecord нам нужно чтобы ActiveRecord не покидала пределов репозитория, а не просто писать что-то вроде function get(id) {return User::find(id);}

                          0
                          Да, все так.
              0

              Мне не очень нравится подход из yii2, он как раз делает приложение безумно связным.
              На наших проектах правила следующие:


              1. Клиентский код никогда не работает напрямую с провайдером данных. Используйте для этого репозитории.
                Если у вас есть сущность Book, то должен быть BookRepository, который в идеале реализует BookRepositoryInterface с описанием всех нужных методов по получению этих самых книг. Т.е. отделяйте клиентский код от деталей реализации. Тут тонкий момент, что если в одном месте клиентского кода нужно получить что-то из репозитория, то проще прописать это в методе репозитория, вроде getTopTenBooks(), а вот если фильтры планируются разные и в разных сочетаниях, то тут уже подход с фильтрами.


              2. По поводу фильтров, мы юзаем https://github.com/Happyr/Doctrine-Specification
                У репозитория есть метод match(), в который можно передать массив фильтров, а сами фильтры описаны в классе BookFilters и представляют из себя набор методов, возвращающих спецификацию. Например activeBook(), userBook(User $user) и т.п.



              В клиентском коде их можно комбинировать и всей кучей передать в репозиторий, чтобы получить набор книг по кастомному фильтру:
              В клиентском коде это будет примерно так выглядеть:
              $books = $this->$bookRepository->match([BookFilters::activeBook(), BookFilters::userBook($user)]);


              Плюсы:


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

              Минусы:


              • иногда фильтров может не хватить. Тогда в класс BookFilters придется чего-нить дописать. Но в этом я не вижу беды, потому что, во-первых, можно не дописывать, а отнаследоваться, а, во-вторых, все методы статические и BookFilters это, по сути, просто библиотека и дописывание туда новых методов не сделает весь подход менее SOLID
                0
                Для начала, Yii2 не мешает использовать репозитории. В нашем проекте они тоже применяются.
                Клиентский код никогда не работает напрямую с провайдером данных. Используйте для этого репозитории.

                Предполагается, что код клиентского класса в моем примере как раз работает на уровне репозитория.
                По поводу фильтров, мы юзаем github.com/Happyr/Doctrine-Specification
                У репозитория есть метод match()...

                Насколько я вижу, это плюс-минус тот же подход, что и в моем примере, только в вашем случае фильтры представлены в виде функций.
                Плюсы:
                клиентский код полностью отделен от реализации получения данных

                Да, как в вашем примере, так и в моем.
                в клиентском коде все фильтры имеют человекопонятные названия, т.е. мы мысли доменными категориями, а не категориями реализации

                Да. Классы фильтров тоже можно называть человекопонятно.
                иногда фильтров может не хватить. Тогда в класс BookFilters придется чего-нить дописать.

                А вот эту граблю мы можем обойти, используя ООП: с классами разделение получается несколько лучше. Но с другой стороны, у вас изоляция на уровне функций, у меня — на уровне классов. Так что по сути, повторюсь, ваш пример почти полностью аналогичен моему.
                0

                Массив фильтров можно заменить названиями методов и не плодить сущности. Методы привязаны к специфичному классу запроса для модели, а классы висят в общем пространстве имен. Эти методы и нужны, чтобы писать в них специфичную бизнес-логику, какой смысл еще одну абстракцию добавлять. А потом мы придем к выводу, зачем добавлять их в массив и вызывать потом, если можно вызывать сразу и передавать настроенный ActiveQuery. Они там внутри и так хранятся в массиве, только в виде условий для where.


                Очевидно, что при таком подходе весь клиентский код, использующий наш метод, будет получать запрос, к которому уже применены новые условия.

                Пример вообще слишком обобщенный, непонятно в каких случаях это требуется. Если есть класс ClientClass, который строит запросы, то это и есть клиентский код. Если у нас есть другой клиентский код, который пользуется методами ClientClass, то он и не должен влиять на фильтры, у нас в этих методах и написана реализация бизнес-логики, которая должна быть скрыта от всех остальных.


                Раньше доставали все записи из таблицы something, а потом добавили поле is_deleted, и надо доставать только те, где is_deleted = 0. Метод getSomethingList() находится в ClientClass, добавили туда вызов метода isActive(), в вызывающем коде менять ничего не надо, и уж тем более они не должны передавать фильтры по этому полю.

                  0
                  Вот конкретный пример из моего проекта.

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

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

                    Да я в общем-то веду к тому, что так вообще быть не должно, и поэтому массив функций тоже не нужен. Вы вызываете старый метод из нового? Тогда получается вы же все равно изменили старый метод, чтобы он обрабатывал фильтры из нового. Ну вот вместо массива можно было ActiveQuery передавать. Вообще наверно наоборот должно быть, старый метод должен вызывать новый, который более гибкий, с нужными настройками.


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

                    Ну так это ж стандартная настройка ActiveQuery, как в любой форме с фильтрами. Тут не требуется писать поверх этого дополнительную абстракцию.

                      0
                      Вы вызываете старый метод из нового?

                      Нет. Я говорю о методах контроллера, о тех, которые action. Вызвать один из другого не предполагается. Они должны работать параллельно, вызывая наш ClientClass. Причем функциональность первого action'а должна остаться как есть, а функциональность второго необходимо расширить.

                      Вообще наверно наоборот должно быть, старый метод должен вызывать новый, который более гибкий, с нужными настройками.

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

                      Ну так это ж стандартная настройка ActiveQuery, как в любой форме с фильтрами. Тут не требуется писать поверх этого дополнительную абстракцию.

                      Дополнительная абстракция в нашем случае необходима, чтобы изолировать изменяемую часть кода (применение дополнительных условий к ActiveQuery) от неизменяемой (код ClientClass). Поэтому мы вносим ее в отдельную группу классов filter.
                        0
                        Они должны работать параллельно, вызывая наш ClientClass. Причем функциональность первого action'а должна остаться как есть, а функциональность второго необходимо расширить.

                        Выглядит так, как будто у вас в контроллерах много логики.
                        Тут все то же самое, передаем валидированный Form в ClientClass, там по нему настраиваем ActiveQuery. В первом данные для Form фиксированные, во втором берутся из конфига или из реквеста.


                        Это как раз будет нарушением принципа открытости-закрытости, поскольку потребует изменения кода старого метода.

                        Так вы же все равно существующий код поменяли, добавив обработку массива filters.


                        чтобы изолировать изменяемую часть кода (применение дополнительных условий к ActiveQuery) от неизменяемой (код ClientClass)

                        В этом случае часть бизнес-логики находится вне слоя бизнес-логики, в контроллерах.


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

                          0
                          Нужно исходить из нашей задачи. А состоит она, в соответствии с принципом открытости-закрытости, в том, чтобы исключить необходимость изменять класс ClientClass при добавлении нового функционала. Обрабатывая входные данные прямо в классе ClientClass, каждое изменение функционала будет приводить к изменению этого класса. Поэтому наша задача в том, чтобы вынести из ClientClass все, что будет изменяться.

                          Так вы же все равно существующий код поменяли, добавив обработку массива filters.

                          Необходимо различать два вида изменений. Первый — это изменения, связанные с обновлением функционала. Второй — рефакторинг с целью устранения нарушения принципа ОЗ. Если первых изменений мы должны избегать, то изменения второго рода как раз и являются нашей основной целью.

                          В этом случае часть бизнес-логики находится вне слоя бизнес-логики, в контроллерах.

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

                          Выглядит так, как будто у вас в контроллерах много логики.

                          Как раз наоборот. Логики в контроллерах у меня нет вообще — только конфигурация.
                            0
                            Обрабатывая входные данные прямо в классе ClientClass, каждое изменение функционала будет приводить к изменению этого класса.

                            Естественно, это же изменение функицонала ClientClass. Можно его не менять, а создать наследника, это и есть расширение. А вы получается выносите логику, связанную с классом, вне его.


                            рефакторинг с целью устранения нарушения принципа ОЗ

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


                            "Идея была в том, что однажды разработанная реализация класса в дальнейшем требует только исправления ошибок, а новые или изменённые функции требуют создания нового класса."


                            Поддержка разных фильтров это новая функция, в соответствии с этим принципом она должна быть в новом классе.


                            Логики в контроллерах у меня нет вообще — только конфигурация.

                            Конфигурация в соответствии с некоторыми правилами это и есть бизнес-логика. Особенно если она касается фильтров с названиями, соответствующими терминам бизнес-логики.

                  0
                  В ActiveRecord в Yii нарушение OCP заложено на уровне архитектуры.
                  $query = ActiveRecordModel::find(); — это прямое нарушение сразу SRP, OCP и IoC.
                  Чтобы уйти от связанности кода, надо использовать AR как persistance layer в своих доменных моделях. Композиция — считать AR сервисом персистентности, и передавать его в модели как зависимости.
                    0
                    В ActiveRecord в Yii нарушение OCP заложено на уровне архитектуры. $query = ActiveRecordModel::find(); — это прямое нарушение сразу SRP,

                    В ActiveRecord нарушение SRP заложено на уровне паттерна


                    Композиция — считать AR сервисом персистентности, и передавать его в модели как зависимости.

                    Нарушение IoC, имхо, и того же SRP ) Модели нужно передавать в сервис персистентности и получать из него, а не наоборот. Или связывать их через DTO. Ну или у меня мозгов не хватило как с AR внутри модели обеспечить непротекание этого факта в публичном хотя бы интерфейсе.

                      0
                      Термин «модель» я использую в оригинальном значении — domain, бизнес-логика.

                      В Yii «model» — мапинг на таблицу, это можно считать слоем персистентности. По терминологии clean architecture это Interface Adapters.
                      Если для слоя персистентности создать дополнительный уровень абстракции — да, AR надо передавать в него, но я бы не называл это моделями.
                      Между слоями передаются value object, конечно.

                      habr.com/ru/company/mobileup/blog/335382

                      Нарушение IoC у автора — статический вызов. Внедрение чего угодно как зависимости — это и есть IoC ;)
                        0

                        Я тоже в оригинальном. :) Но единственным местом куда можно внедрять AR типа Yii или Eloquent по умолчанию считаю инфраструктурные сервисы персистентности типа репозиториев. В слое модели может быть интерфейс объявлен, работающий с чистыми моделями или DTO и всё

                          0
                          С этим есть одна проблема. Чтобы внедрять AR-объекты в инфраструктурные сервисы персистентности типа репозиториев, нужно создавать их где-то еще. А негде. Приходится жертвовать независимостью слоя персистентности, и хардкодить ActiveRecordModel::find(); в нем. А что делать, хотя бы по слоям не размазывается.
                            0

                            "Внедрять" я не в смысле "инжектить", а в смысле "использовать в принципе". а так, да, согласен полностью. В Eloquent можно извратиться и прокидывать из контейнера инстанс модели пустой, но смысла ноль.

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

                  Самое читаемое