Don't overuse dependency injection
Benjamin Grandfond4 min read
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 ?
