Are there too many parameters in this constructor?

Question

Check this out:

public function __construct(
        \Magento\Framework\Model\Context $context,
        \Magento\Framework\View\DesignInterface $design,
        \Magento\Framework\Registry $registry,
        \Magento\Store\Model\App\Emulation $appEmulation,
        \Magento\Store\Model\StoreManagerInterface $storeManager,
        \Magento\Framework\App\RequestInterface $request,
        \Magento\Newsletter\Model\Template\Filter $filter,
        \Magento\Framework\App\Config\ScopeConfigInterface $scopeConfig,
        \Magento\Newsletter\Model\TemplateFactory $templateFactory,
        \Magento\Framework\Filter\FilterManager $filterManager,
        array $data = []
    ) {
        parent::__construct($context, $design, $registry, $appEmulation, $storeManager, $data);
        $this->_storeManager = $storeManager;
        $this->_request = $request;
        $this->_filter = $filter;
        $this->_scopeConfig = $scopeConfig;
        $this->_templateFactory = $templateFactory;
        $this->_filterManager = $filterManager;
    }

It's from https://github.com/magento/magento2/blob/develop/app/code/Magento/Newsletter/Model/Template.php#L107

The function declaration takes more space than the body.

I think it has something to do with dependency injection.

What's the benefit of structuring your code as above instead of something like this:

public function __construct(Magento $magento, array $data = []
    ) {
        parent::__construct($magento->context, $magento->design, $magento->registry, $magento->appEmulation, $magento->storeManager, $data);
        $this->_storeManager = $magento->storeManager;
        $this->_request = $magento->request;
        $this->_filter = $magento->filter;
        $this->_scopeConfig = $magento->scopeConfig;
        $this->_templateFactory = $magento->templateFactory;
        $this->_filterManager = $magento->filterManager;
    }

Notice that I only need to create the Magento instance once. Then I pass it to all the classes that need stuff.

but my 2nd example is not a static factory. It's just a top level class that provides dependencies to other classes. Nothing static there :/ — user1806244, Mar 19 '15 at 15:54
Did you read my answer on that former question? Should apply to your case perfectly. — Doc Brown, Mar 19 '15 at 15:58
ok i guess groping certain dependencies in another class could be the 3rd example. I'm only asking why ppl bloat their constructor with 20 arguments when they can just pass some kind of "application" object to the constructor that provides all the required stuff. I get the "because it hides the dependencies" reason. But is that worth the extra bloat and complexity? Any other reasons, preferably good? — user1806244, Mar 19 '15 at 16:05

score 4 · Answer 1 · answered Mar 18 '15 at 23:16

There isn't anything significantly wrong with the first piece of code. Though having more than half a dozen dependencies may indicate a violation of the SRP, there is nothing inherently wrong with having a large constructor.

In fact, I would much prefer seeing a constructor with a large number of arguments than a method. Constructors are for getting our ducks in a row so that our class can do what it needs to do.

Your replacement code is actually more dangerous as it is hiding the true dependencies of the class and it requires another object to be constructed just so that you can construct this object.

score 3 · Answer 2 · answered Mar 18 '15 at 22:29

3

How is your code easier than the first one?

What would Magento constructor look in your example? It would take the same parameters as the actual constructor from the first piece of code, and the only difference would be that instead of one class, you now have two classes.

Refactoring should improve code, not move the difficult part to a different class.

On the other hand, if you group logically related arguments together and replace them by a class, it may make sense. Looking at the first piece of code, I don't see any candidates for grouping. The code seems quite readable.

answered Mar 18 '15 at 22:29

Arseni Mourzenko

134,780
31
343
513

+1 There isn't anything significantly wrong with the first piece of code. Though having more than half a dozen dependencies may indicate a violation of the SRP, there is nothing inherently wrong with having a large constructor. – Stephen Mar 18 '15 at 23:10
What do you mean "instead of one class, you now have two classes". My code has one class, the original has like 10! – user1806244 Mar 18 '15 at 23:19
I think he meant object. – Stephen Mar 18 '15 at 23:28

score 1 · Answer 3 · answered Mar 19 '15 at 06:03

The first approach you mentioned is absolutely perfect and there is no need for code refactoring. Dependency Injection is very useful for writing test cases on your class. Because you can easily mock the injected class functionality while you are writing test cases as per your need.

The second approach you mentioned is a wrong one. Because, you are passing entire Magento framework object to your class. Which means that your are exposing all unwanted features inside your class (No Encapsulation!).

If you are planning to pass the variables to this class i will definitely suggest you do create one wrapper class for that. But here you are passing the objects so there is no need for wrapper class. If we do so, It will increase the complexity of your code.

score 0 · Answer 4 · answered Mar 19 '15 at 14:01

Having a base class with 6 dependencies is pushing it but acceptable under some circumstances especially if it's rare to have to extend the class. The PHP Symfony 2 framework has a security class with a bunch of dependencies. Extending it is painful but you only need to do so when implementing a completely new authentication system.

On the other hand, the Magento example looks more like a constroller class and one assumes that extending it is quite frequent. Symfony 2 also has a base controller class with useful methods like getUser, createForm, render etc. The base class would take about 8 dependencies in it's constructor. It would be very painful and very fragile if a developer had to support several dozen controller classes in their application and keep track of all these dependencies. There would also be performance issues as all the these dependencies would need to be created (or at least proxies) for each controller.

So Symfony 2 does something that I think works well. It injects the dependency injection container and uses it to access these helper services as needed. At the same time, the constructor is reserved for controller specific services. So you have something like:

class PersonController extends BaseController
{
    private $container; // Actually in BaseController
    private $personRepository;

    // Also in base controller
    public function setContainer($container) { $this->container = $container; }

    // Nice easy constructor
    public function __construct($personRepository)
    {
        $this->personRepository;
    }

    // An action method
    public function showPersonAction($request)
    {
        $person = $this->personRepository->find($request->get('id'));

        // render uses a templating service from the container
        return $this->render(['person' => $person],'person.template.html');
    }
}
$controller = new PersonController($container->get('person_repository');
$controller->setContainer($container);
$response = $controller->showPersonAction($request);

I think this is a good compromise. No large constructor. Easy to extend and read. You do need to remember to inject the controller but this is usually handled by the front controller.

score -1 · Answer 5 · answered Mar 18 '15 at 22:40

-1

It seems like your software could benefit from Dependency Injection. This way, the parent constructor could have its dependencies provided directly to it without having to burden all child classes with constructors that do little more than pass arguments along until they reach the constructor that actually needs them.

answered Mar 18 '15 at 22:40

Kent A.

880
4
9

1

More details please. Are you suggesting method injection? I don't see how you can pass dependencies directly to a parent constructor when instantiating an extended class. – Cerad Mar 18 '15 at 23:50
I am not a PHP a expert, and have not used DI in PHP, but this tutorial seems to do a good job explaining the basics : http://code.tutsplus.com/tutorials/dependency-injection-in-php--net-28146. The basic idea of DI is that there is an extra runtime component (usually called a "container") that can assign objects to variables in your classes so they are there and ready for use when you need them. Long-lived, independent objects can be injected where they are needed, instead of being passed along the call chains of constructors and methods. – Kent A. Mar 19 '15 at 01:11
Hmmm. Not to be rude but have you read the link? The basic idea of DI is unrelated to containers and is pretty much language independent. – Cerad Mar 19 '15 at 01:39
@Cerad, I did read the link. The general convention in DI is to call the object that manages the "injectables" and the "injectees", a "container". You are correct. The principle of DI is language independent, and there are implementations for most languages available, including PHP, as this question is focused on PHP. The reason I thought to introduce DI into this question is because I noticed the code required the constructor of the child class to receive parameters that it did nothing with, except to pass them along to the superclass. DI can help simplify this interface. – Kent A. Mar 19 '15 at 02:05
Here's another link (this is for a specific implementation). Again, I do very little PHP work, but this introduction page does a good job of explaining the concept: http://php-di.org/doc/understanding-di.html – Kent A. Mar 19 '15 at 02:36

Are there too many parameters in this constructor?

5 Answers5

Linked