A controller that might violate Single Responsibil

Question

Referring to this comment,

When 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. If your class is trying to do too much, consider refactoring your code into a number of smaller classes that consume each other.

What should I do about this controller class below - is it "trying to do too much"?

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;
        }
    }
}

How can I break it up into smaller classes - if it is a controller that holds these basic classes that I need to output a page?

And what should I do so that it is follows the principle of dependency injection?

Answer 1

TL;DR: I don't see violation of SRP here, but object composition is slightly broken

From what I see (is this the full class listing?), $connection is not directly consumed in the class, so it should not be injected.

And I don't see usage of $this->translation and $this->nav anywhere. Is that a copy-paste artifact?

Rather inject $connection, I would construct Article and Auth outside of this class, then inject only these, because your controller only uses these directly, not controller. So the whole class would be like this:

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;
        }
    }
}

Unless your Article is a domain object with ActiveRecord pattern, in that case I'd still inject $connection and store it in local variable. And create new Article object only when you actually need it, i.e. in getHtml method.

This way you don't do too much work in constructor, only assigning local variables. Objects composition is handled somewhere else. And if you need to, you can replace implementation of your Auth.

Also when you don't do too much work in the constructor, your object graph creation is very cheap. And this is important if you use some sort of DI container, when a lot of objects must be created at the same time.

Answer 2

_{Note: this will be the short version, because I'm at work. I will elaborate in the evening}

So ... your code has following violations:

• SRP (and by extension - SoC): your controller is responsible for validation of input, authorization, data gathering, populating template with data and rendering said template. Also, your Article seems to be responsible for both DB abstraction and domain logic.

• LoD: you are passing the $connection only because you need to pass it on to other structures.

• encapsulation: all your class attributes have public visibility and can be altered at any point.

• dependency injection: while your "controller" has several direct dependencies, you are only passing in the template (which actually shouldn't be managed by controllers in proper MVC).

• global state: your code depends on $_REQUEST superglobal.

• loose coupling: your code is directly tied to the names of classes and the footprint of constructors for those classes, that you initializing in the constructor.