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.

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

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.

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

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.

Are there too many parameters in this constructor?

5 Answers5

Linked