Don’t overuse dependency injection

The other day, I stumbled upon the following code that I wrote a few days before.

<?php
// src/Foo/BarBundle/Manager/ReportManager.php

namespace Foo\BarBundle\Manager;

use Symfony\Component\Security\Core\SecurityContextInterface;
use Symfony\Component\Finder\Finder;

class ReportManager
{
    /**
     * @var \Symfony\Component\Security\Core\SecurityContextInterface
     */
    protected $securityContext;

    /**
     * @var String
     */
    protected $baseDir;

    /**
     * @param string                                                    $directory
     * @param \Symfony\Component\Security\Core\SecurityContextInterface $context
     */
    public function __construct($directory, SecurityContextInterface $context)
    {
        $this->baseDir         = $directory;
        $this->securityContext = $context;
    }

    /**
     * Returns files for a specific year.
     *
     * @param int $year
     *
     * @return array
     */
    public function getFiles($year)
    {
        if (null == $this->securityContext->getToken()) {
            throw new \Exception('There is no token in the security context');
        }

        $company = $this->securityContext->getToken()->getUser()->getSelectedCompany();

        $finder = new Finder();
        $finder->files()->in($this->baseDir."/$year/{$company->getCode()}");

        return iterator_to_array($finder, false);
    }
}

Here is the service definition in the FooBarBundle.

# src/Foo/BarBundle/Resources/config/services.yml

parameters:
    manager.report.class: Foo\BarBundle\Manager\ReportManager

services:
    manager.report:
        class: %manager.report.class%
        arguments:
            - 'path/to/reports'
            - @security.context

The usage of this class is really simple: you call the method
getFiles of the service in any part of your application an you’ll get an
array of files. Here is an example in a controller:

// src/Foo/BarBundle/Controller/ReportController.php

// ... lot of code in your controller
public function listAction($year)
{
    $files = $this->get('manager.report')->getFiles(date('Y'));

    return new Response($this->renderView(
        'FooBarBundle:Report:list.html.twig',
        array('files' => $files)
    ));
}

Here is my question, can you guess what’s wrong in all this code? … Ok that’s
not really obvious at first. The problem in this code is the ReportManager
class.

First of all it is in the Namespace Foo\BarBundle\Manager. "Manager" does
not mean anything except that every classes contained in this namespace
manages things. The name is not really well chosen, it could have been
Foo\BarBundle\FileManager instead, or the file could have been directly in
the Foo\BarBundle namespace.

But it’s not the main problem… It appeared to me when I decided to test it
(I should have done it really earlier) after adding some code to this class.
Here is the test I started to write:

<?php

namespace Foo\BarBundle\Tests\Manager;

use Foo\BarBundle\Manager\ReportManager;

class ReportManagerTest extends \PHPUnit_Framework_TestCase
{
    public function testGetFiles()
    {
        $company = $this->getMock('Foo\BarBundle\Entity\Company');
        $company->expects($this->once())
            ->method('getCode')
            ->will($this->returnValue('foo'));

        $user = $this->getMock('Symfony\Component\Security\Core\User\UserInterface');
        $user->expects($this->once())
            ->method('getSelectedCompany')
            ->will($this->returnValue($company));

        $token = $this->getMock('Symfony\Component\Security\Core\Authentication\Token\TokenInterface');
        $token->expects($this->once())
            ->method('getUser')
            ->will($this->returnValue($user));

        $securityContext = $this->getMock('Symfony\Component\Security\Core\SecurityContextInterface');
        $securityContext->expects($this->exactly(2))
            ->method('getToken')
            ->will($this->returnValue($token));

        $reportManager = new ReportManager('bar', $securityContext);
        $this->assertCount(0, $reportManager->getFiles(2013));
    }
}

Do you see what the problem is with the ReportManager class now?

Have you ever heard about the Law of Demeter? Well that’s the perfect moment to
introduce it to you. The Law of Demeter is a development design principle
which states that an object’s method should only interact with:

  • the object itself
  • the method’s parameters
  • any object created within the method or the object’s component objects (parents etc.).

Basicaly, an object A can interact with an object B but cannot use it to get an object C.

Obviously, the code I wrote did not respect this principle at all! I injected
the SecurityContext in the constructor to use it in the getFiles method
I needed it because the context allowed me to have the current connected User through
the Token and then, calling the getSelectedCompany, I could have the company’s code.

Why did I do that? Well because injecting the SecurityContext is the only
way to retrieve the connected user. And with this user I can retrieve the
current selected company’s code.

So do you think that the injection was well chosen? Surely not, and it’s not
needed either. The unit test shows that because I need to mock 4 objects to
have a simple string (the code) in the getFiles method.

Seeing this, I immediatly refactored it this way:

<?php

namespace Foo\BarBundle\Manager;

use Symfony\Component\Finder\Finder;

class ReportManager
{

    /**
     * @var String
     */
    protected $baseDir;

    /**
     * @param string $directory
     */
    public function __construct($directory)
    {
        $this->baseDir = $directory;
    }

    /**
     * Returns files for a specific year.
     *
     * @param int    $year
     * @param string $code The company code
     *
     * @return array
     */
    public function getFiles($year, $code)
    {
        $finder = new Finder();
        $finder->files()->in($this->baseDir."/$year/{$code}");

        return iterator_to_array($finder, false);
    }
}

Then I updated the service definition accordingly to these changes, I removed
the dependency to the security.context service:

services:
    manager.report:
        class: %manager.report.class%
        arguments:
            - 'path/to/reports'

And I moved the logic that retrieves the company’s code into the controller:

// src/Foo/BarBundle/Controller/ReportController.php

// ... lot of code in your controller
public function listAction($year)
{
    $company = $this->getUser()->getSelectedCompany();
    $files = $this->get('manager.report')->getFiles(date('Y'),$company->getCode());

    return new Response($this->renderView(
        'FooBarBundle:Report:list.html.twig',
        array('files' => $files)
    ));
}

Then the test looks much cleaner:

<?php

namespace Foo\BarBundle\Tests\Manager;

use Foo\BarBundle\Manager\ReportManager;

class ReportManagerTest extends \PHPUnit_Framework_TestCase
{
    public function testGetFiles()
    {
        $reportManager = new ReportManager('bar');
        $this->assertCount(0, $reportManager->getFiles(2013, 'foo'));
    }
}

This mistake led me to 2 conclusions:

First, it is another point in favor of TDD: had I TDD’ed the whole thing, I would never
have written such over-complicated code… And second, you should not overuse dependency
injection, its ease of use makes it sometimes the obvious solution, but not the simplest
nor the best one. So next time you add a dependency, just pause for half a second and
ask yourself: is it really necessary ?


You liked this article? You'd probably be a good match for our ever-growing tech team at Theodo.

Join Us

  • Hello,

    Nice article!

    In the end, after the refactoring, you’re still passing a SecurityContextInterface to your constructor (both in the class definition and unit test) while you’re not using it.

    Cheers.

  • Thank you Hugo for reporting this, it’s fixed now :)

  • Frank

    For me there is no reason to refactor it this way. Due to your refactoring you move business logic from your service to your controller and you never should do that.
    Besides you could not realize DRY in this way. Everywhere you need this “manager” service you have to call
    “$company = $this->getUser()->getSelectedCompany();” first and pass it to your manager.

    My refactoring would be:
    Create a service which takes the SecurityContext and provides a method for accessing the company name from the user, name it like CompanyCodeProvider or whatever.

    Inject this provider into your manager and get the company code from this injected service.

    This new provider could be mocked very easy and your problems are gone :)

  • Very well written article and a great example on refactoring!

    I wanted to share that your final refactor still violates the Law of Demeter in the controller, where the Controller knows about the Company object:

    $company = $this->getUser()->getSelectedCompany();
    $files = $this->get(‘manager.report’)->getFiles(date(‘Y’),$company->getCode());

    You are only obfuscating this violation by assigning it to a variable for readability:

    [Controller] –> [User] –> [Company] –> [Company.code]

    When construction object relationships it’s all too common to expose the entire related object via a getter. In PHP we have a harmful history of ORMs like Doctrine 1 where all getters and setters are generated by default when defining your object relationships.

    For example, if a User is related to a Company object, User provides a “getCompany()” method that returns the full Company object to the outside world. This can be harmful and limits your ability to refactor! This is exactly what you’re doing here. Providing full objects via getters makes it very easy to violate the Law of Demeter later when trying to display info about that object (usually in your View template.)

    Instead, your User object should only expose getter methods for properties of Company that your application needs when working with the User object:

    // User.php
    public function getCompanyCode() {
    return $this->company->getCode();
    }

    This will help you ten-fold if needing to refactor later because it isolating object relationship changes from the outside world.

    When getting the User’s company code, your outside application logic should not care/know how User implements its relationship with the Company object (or if a Company object even exists at all!) And User must not care how Company implements its relationships with Company Code (it could be another object, a database lookup, derived from other info inside the Company object, etc). User should simply ask Company to provide its Code, and the resulting implementation details within Company are kept private to the outside world.

  • Frank

    @John
    Doing it this way your User entity has to know everything about the Company as during project lifetime almost every field of the company needs to be accessed through the user entity.

    Then you will end up in

    // User.php
    public function getCompanyCode();
    public function getCompanyId();
    public function getCompanyAnotherRelatedEntityId();
    // dozens other getters of related objects
    ….

    You will bloat totally the user entity. If you really think the other way will prevent you from refactoring then you might provide a UserView class or work with interfaces.

  • I’m not sure that the conclusion to draw here is that there was too much dependency injection. The issue with the original class was that were too many dependencies, that they were made explicit by being injected made this easier to recognise.

    That there were too many dependencies was a symptom of the class breaking the Single Responsibility Principle, it was responsible for getting the files for a year and company code combination and also for finding out the company code for the currently logged in user. You change resolved this by removing the responsibility for finding the current user’s company code. This also makes this class much more useful as it can be used regardless of where the company code comes from.

    The issue pointed about by previous commenters is that the responsibility for finding out the current user’s company code has been given to the controller and not to a different service. It is good to remove it from this class but not so good to just make the controller do it instead.

    The problem is not with dependency injection but with the SRP violation leading to too many dependencies. This would have been the case whether or not DI was used, DI just made it easier to diagnose.

  • Thank you guys for your comments.

    @Richard you are right, SRP was broken and removing the dependency solved the problem. But what I wanted to highlight is that we inject dependencies that are useless too often, and thus beak the SRP as you said.
    However I don’t think DI helped me to diagnose the problem in my code, but unit test did it.

    @John as @Frank said the class would end up with too mush code and I don’t think it is a good idea. I can’t imagine adding getters (and setters/hassers or issers) in the User class for any property of the Company entity. Nevertheless you are absolutely right, I just moved the problem with the Law of Demeter to the controller but this one is necessary as it is the only way I had to fetch the code of the selected company of the connected user.

    @Frank creating a provider may be a good solution, but it would only move the dependency to another object and the SRP would still be broken as, thanks to @Richard to highlight it, was a problem initially, don’t you think ?

  • My 2 cents: you should pass the full company object to your service, to let service decide of what he needs from your Company.

  • How about this situation: http://stackoverflow.com/questions/29661141/creating-dry-symfony2-controller

    is this an overuse of the DI of symfony? hope you won’t find my comment as inappropriate :)

  • Léo Le Taro

    That’s not what I’d call “an overuse of DI”. I’d call it a violation of the Single Responsibility Principle. Your class had both the responsibilities of fetching files AND getting a company code.

    Actually, the Law of Demeter is part of DI best practices, so your LoD violation ($this->securityContext->getA()->getB()->….) went AGAINST proper DI.

    I’m guessing your controllers are in the uppermost layer of the object graph (they are no one’s dependency, they are not injected anywhere) so it’s fine: they act as Factories. So, doing the wiring there is fine. Be careful however should you want to test them someday.