Контролер, который может нарушить принцип единой ответственности?

Ссылаясь на этот комментарий,

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

Что мне делать с этим классом контроллера ниже — он «пытается сделать слишком много»?

class Controller
{
    public $template;
    public $translation;
    public $auth;
    public $article;
    public $nav;

    public function __construct(Database $connection, $template) 
    {
        $this->template = $template;
        $this->translation = new Translator($connection);
        $this->nav = new Nav($connection);
        $this->article = new Article($connection);
        $this->auth = new Auth($connection);
    }

    public function getHtml() 
    {
        if(isset($_REQUEST['url'])) 
        {
            $item = $this->article->getRow(['url' => 'home','is_admin' => $this->auth->is_admin]);
            include $this->template->path;
        }
    }
}

Как я могу разбить его на более мелкие классы - если это контроллер, содержащий эти основные классы, которые мне нужны для вывода страницы?

И что мне сделать, чтобы он следовал принципу зависимости инъекция?


dependency-injection model-view-controller solid-principles php-5.5 single-responsibility-principle
person laukok    schedule 31.07.2014    source источник
comment
Два параметра - это нормально. Однако все эти экземпляры, вероятно, должны быть параметрами либо для конструктора, либо для отдельных сеттеров.   -  person halfer    schedule 01.08.2014
comment
@halfer: спасибо. Да, all those instantiations уродливы в моем контроллере, как я могу сделать их отдельными сеттерами, чтобы я мог вызывать их в своем классе контроллера?   -  person laukok    schedule 01.08.2014
comment
@halfer: если я сделаю их parameters either to the constructor, то a class has a very long list of arguments, it can be a "code smell" that your class is trying to do too much and possibly not following the single responsibility principle. не будет?   -  person laukok    schedule 01.08.2014
comment
(Хотя форматирование кода выглядит красиво, оно не очень читабельно в качестве общего маркера ни в вопросах, ни в комментариях. Люди, заинтересованные в редактировании вопросов здесь, обычно предполагают, что это подходит только для встроенного кода).   -  person halfer    schedule 01.08.2014
comment
Да, не добавляйте их все в конструктор. Конструктор может иметь 2 или 3 параметра, критически важных для построения объекта, а остальные должны быть в сеттерах. Сеттер — это функция, которая принимает параметр и сохраняет его в свойстве, возможно, после его проверки. Вы также можете ввести их в PHP, если они являются классами или массивами.   -  person halfer    schedule 01.08.2014
comment
В вашем случае, если все эти классы имеют одинаковую важность и если $connection не требуется отдельно, я мог бы использовать сеттеры для внедрения всех этих классов и просто иметь $template в конструкторе.   -  person halfer    schedule 01.08.2014
comment
Кстати, вы задали четыре вопроса по этой теме (SRP, DI, избыточные параметры) за последние четыре часа. Один или два вопроса могли бы охватить все это. В частности, этот вопрос по сути является той же проблемой, что и ваш последний вопрос .   -  person halfer    schedule 01.08.2014
comment
@halfer: да, извини за это. потому что я до сих пор не уверен, как удалить эти экземпляры из конструктора и превратить их в методы набора.   -  person laukok    schedule 01.08.2014

Ответы (2)


arrow_upward
1
arrow_downward

Примечание. это будет короткая версия, потому что я на работе. вечером уточню

Итак... ваш код имеет следующие нарушения:

  • SRP (и, соответственно, SoC): ваш контроллер отвечает за проверку ввода, авторизацию, сбор данных, заполнение шаблона данными и визуализацию указанного шаблона. Кроме того, ваш Article, похоже, отвечает как за абстракцию БД , так и за логику домена.

  • LoD: вы передаете $connection только, потому что вам нужно передать его другим структурам.

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

  • внедрение зависимостей: хотя ваш «контроллер» имеет несколько прямых зависимостей, вы только передаете шаблон (который на самом деле не должен управляться контроллерами в правильном MVC).

  • глобальное состояние: ваш код зависит от $_REQUEST superglobal.

  • слабая связь: ваш код напрямую связан с именами классов и размером конструкторов для тех классов, которые вы инициализируете в конструкторе.

person tereško    schedule 01.08.2014
comment
Спасибо за ответ. не могли бы вы объяснить, как я могу улучшить свой контроллер? Благодарность! - person laukok; 01.08.2014
comment
Кроме того, не могли бы вы взглянуть на мой новый пост здесь, чтобы увидеть, не нахожусь ли я в правильном направлении? codereview.stackexchange.com/questions/58709/ спасибо! - person laukok; 01.08.2014

arrow_upward
0
arrow_downward

TL;DR: я не вижу здесь нарушения SRP, но композиция объекта немного нарушена

Из того, что я вижу (это полный список классов?), $connection не используется напрямую в классе, поэтому его не следует внедрять.

И я нигде не вижу использования $this->translation и $this->nav. Это артефакт копипаст?

Вместо того, чтобы вводить $connection, я бы построил Article и Auth вне этого класса, а затем ввел бы только их, потому что ваш контроллер использует их напрямую, а не контроллер. Таким образом, весь класс будет таким:

class Controller
{
    private $article;
    private $auth;
    private $template;

    public function __construct(Article $article, Auth $auth, $template) 
    {
        $this->article = $article;
        $this->auth = $auth;
        $this->template = $template;
    }

    public function getHtml() 
    {
        if(isset($_REQUEST['url'])) 
        {
            $item = $this->article->getRow(['url' => 'home','is_admin' => $this->auth->is_admin]);
            include $this->template->path;
        }
    }
}

Если ваш Article не является объектом домена с шаблоном ActiveRecord, в этом случае я бы все равно ввел $connection и сохранил его в локальной переменной. И создавайте новый объект Article только тогда, когда он вам действительно нужен, т.е. в методе getHtml.

Таким образом, вы не слишком много работаете в конструкторе, а только назначаете локальные переменные. Состав объектов обрабатывается где-то еще. И если вам нужно, вы можете заменить реализацию вашего Auth.

Кроме того, когда вы не слишком много работаете в конструкторе, создание графа объектов обходится очень дешево. И это важно, если вы используете какой-то DI-контейнер, когда одновременно нужно создать много объектов.

person trailmax    schedule 31.07.2014
comment
Спасибо за ответ. если я буду следовать этому шаблону, мои инъекции в моем конструкторе будут очень длинными для определенных контроллеров. - person laukok; 01.08.2014
comment
для контроллеров со слишком большим количеством зависимостей это явный запах кода и возможное нарушение SRP. Но не в этом образце. - person trailmax; 01.08.2014