Our dev department is spread across two campuses: at our head office in Dublin, and the one I'm based at in London.
Yesterday (Friday) the London bods had our weekly Dev Team catch-up, to discuss various issues that cropped up during the week which need more concerted discussion than an "ad hoc make a decision and get on with it" situation might resolve.
One of the chief discussion points is our dependency injection strategy, and the question was raised if we're doing it right, as it seems there were a few code smells creeping in in places.
As coincidence would have it... when we got out of the meeting a few of us had an email in our inbox from one of our mates in Dublin who was amidst much the same dilemma in some code he was working on.
This resulted in a further discussion between the London people, rounding out the afternoon. I think we clarified a few things really well, re-formalised our DI strategy in our heads, and identified some code we need to look at to work out what the actual cause of the smell is. What was really good is at the end of it we all agreed with where we got to, and the direction to take. As we have a few people who like holding fair strong opinions in the team ([raises his hand gingerly]), this is encouraging.
The gist of the conversation is worth repeating I reckon, and I hope the relevant parties don't mind. And I reckon it shows our team in pretty good light so I reckon the bosses won't mind either. I'm sure I'll find out if not!
OK, so - as you probably know - we're a PHP shop. For the main project I work on (www.hostelbookers.com) - we have just finished retiring the old ColdFusion-based site ("The end of ColdFusion"), replacing it with a PHP one running on the Silex micro-framework. Silex is built around Pimple, Sensio's DI Container. I've written a bit about Silex and Pimple if you want to get the gist of it.
Very quickly, Pimple implements DI via the notion of service providers, one of which might look like this (this is sample code from an earlier blog article: it's not code from our own app, nor is any code in this article):
namespace \dac\silexdemo\providers;
class ServiceProvider implements ServiceProviderInterface {
use Silex\ServiceProviderInterface;
use Silex\Application;
use \dac\silexdemo\beans;
public function register(Application $app){
$app["services.user"] = $app->share(function($app) {
return new services\User($app["factories.user"], $app["services.guzzle.client"]);
});
$app["services.guzzle.client"] = function() {
return new Client();
};
}
public function boot(Application $app){
// no booting requirements
}
}
There's a
register() method within which one defines all the objects and their dependencies, and a boot() method for calling code on a a defined object to be run before it's used. As all of these are defined using closures, no objects are actually created until they need to be used. Cool.In this example I'm defining some services, but we define everything via service providers: services, controllers, repositories, factories, helpers etc.
We're defining the user service object there, and you can see from that that its constructor expects a coupla dependencies:
function __construct($userFactory, $guzzleClient){
$this->userFactory = $userFactory;
$this->guzzleClient = $guzzleClient;
}
From there, all the code in UserService can use the dependency to help out with its logic. Fairly standard stuff.
The chief conceit here is that we definitely use configuration over convention, and we configure our objects explicitly with the dependencies they need. UserService needs a UserFactory and it needs a GuzzleClient, so we specifically pass those in.
Aside: a word on configuration over convention
I really dislike the "convention" approach to pretty much anything like this... DI config... routing config etc. The reason being that the framework should simply busy itself with frameworking stuff, it should not be dictating to me how I write my code. That is not the job of the framework (be it a DI one or an MVC one). Also conventions are very opinionated, and we all know what is said about opinions. Frameworks that implement convention over configuration position the code as if the framework is the central element to the application, wherein it really ought to simply be some code that sits off to one side, and just gets on with it. My application is what is central to the application.People will claim that it's easier to use convention of configuration, but I don't believe "easier" should be a primary consideration when designing one's app. Especially when it's "easier to use the framework", rather than "intrinsically good design".
But I digress.
This is fine, and there's no alarm bells ringing there.
Well one slight alarm might that say the object we're defining might have some actual OO type properties. This is a really contrived example (not to mention wrong) example, but this illustrates it:
class Person {
private $firstName;
private $lastName;
private $personRepository;
function __construct($firstName, $lastName, $personRepository){
$this->firstName = $firstName;
$this->lastName = $lastName;
$this->personRepository = $personRepository;
}
}
(This implies our Person model is also charged with persisting itself. Hmmm. Like I said: not a good example).
From an OO perspective we have what are clearly properties of a Person: first and last names. But we also have what is an "infrastructure" property in the personRepository property. Does this smell a bit? Does it smell as much as the fact we need a repo in a data modelling class? Hmmm.
OK, but the real alarm bells - the ones that started our conversation in the meeting, was we were having situations like this (this is really contrived as I don't want to use our own code here - not least of all cos I don't have it to hand - but I'm stuffed if I can come up with a half-decent figurative example):
class UserService {
private $userFactory;
private $userRepository;
private $validationHelper;
private $loggingService;
private $authenticationService;
private $stringHelper;
private $emailService;
public __construct($userFactory, $userRepository, $validationHelper, $loggingService, $authenticationService, $stringHelper, $emailService){
$this->userFactory = $userFactory;
$this->userRepository = $userRepository;
$this->validationHelper = $validationHelper;
$this->loggingService = $loggingService;
$this->authenticationService = $authenticationService;
$this->stringHelper = $stringHelper;
$this->emailService = $emailService;
}
}
That's a lot of frickin' dependencies there. We try to follow Clean Code guidelines, which predicates keeping function arguments to a minimum, and we pretty much position "if you've got more than two... yer probably doing it wrong". We wondered if it was legit to ignore that rule for constructors, but also figured if we needed to discuss it, then it was probably less than ideal as a "rule". So we have a code smell there.
I reflected for a bit and came up with this metaphor:
class Account {
private $accountId;
private $firstName;
private $lastName;
private $phoneNumber;
function __construct($accountId, $firstName, $lastName, $phoneNumber){
$this->accountId = $accountId;
$this->firstName = $firstName;
$this->lastName = $lastName;
$this->phoneNumber = $phoneNumber;
}
}
Let's pretend for a sec it's legit for an Account object to have all that info. So we've got too many arguments there... what's the problem? Well it's cos we've not decomposed things properly. What it should be is this:
class Account {
private $accountId;
private $contactPerson;
function __construct($accountId, $contactPerson){
$this->accountId = $accountId;
$this->contactPerson = $contactPerson;
}
}
The issue is the properties of the Account were too granular for what the account should be interested in. I wondered if we could apply this approach to our dependencies, ending up with this:
class UserService {
private $userFactory;
private $userRepository;
private $validationHelper;
private $loggingService;
private $authenticationService;
private $stringHelper;
private $emailService;
public __construct($dependencies){
$this->userFactory = $dependencies['factory.user'];
$this->userRepository = $dependencies['repository.user'];
$this->validationHelper = $dependencies['helper.validation'];
$this->loggingService = $dependencies['service.logging'];
$this->authenticationService = $dependencies['service.authentication'];
$this->stringHelper = $dependencies['helper.string'];
$this->emailService = $dependencies['service.email'];
}
}
And in our provider, we'd have this:
$app['service.user'] = $app->share(function($app) {
return new service\User([
'userFactory' => $app['factory.user'],
'userRepository' => $app['repository.user'],
'validationHelper' => $app['helper.validation'],
'loggingService' => $app['service.logging'],
'authenticationService' => $app['service.authentication'],
'stringHelper' => $app['helper.string'],
'emailService' => $app['service.email']
]);
});
So we kinda make an on-the-fly dependency container to pass into the constructor.
We were spit-balling about this before the meeting, and it suddenly occurred to use... why are we making our own "dependency container"? We've got one...
$app is a dependency container. Why don't we just use that?$app['service.user'] = $app->share(function($app) {
return new service\User($app);
});
(UserService stays the same).
We decided this was something to bring up in the meeting, so we discussed it.
In the meeting we found ourselves spending too much time going "oooh... but... is that right? Something about it seems... wrong". We actually went further in the discussion in the meeting, discussing this sort of approach:
class UserService {
$private app;
public __construct($app){
$this->app = $app;
}
// [more code here, snipped for brevity]
public emailThem(){
$this->app['service.email']->send($someArgs);
}
}
This engendered even more pursed lips and "oooh" noises, and a general unease that 'service.email' seemed to appear out of nowhere, and seemed a bit smelly in and of itself.
Then the boss had to head to another meeting, so we disbanded and went back to our desks. At which point we saw the email from our mate over in Dublin coincidentally wondering about the same sort of thing, specifically passing
$app into constructors, and using it directly like that last example.A bunch of us decided to continue the analysis and get to a point where we weren't pursing our lips and going "oooh... dunno about that".
After looking at some of our code, it suddenly occurred to us why this is wrong: taking that approach couples our code to the DI implementation. Which is... wrong. It's fine for our Service to require dependencies: that's a decoupling of concerns consideration and completely legit. But it's not fine for use to be writing code specific to the DI framework implementation within our classes. It means our code is not transportable, and indeed means it must be used with a DI framework, and - worse - one that works in the way Pimple does, by building an associative array of dependencies exposed from a base object. I'm not saying there's anything wrong with the way Pimple effects this, btw (on the contrary: it's excellent), but it should not be bleeding into our code like that.
Seen in that light, even passing
$app directly into the constructor and then breaking out the dependencies we need is equally wrong; just on a smaller scale. So we won't be doing that. What we will be doing is... exactly what we are already doing!This does leave us though wondering about the miasma emanating from classes that need more than one or two dependencies. We had a quick look at some of the mid-weight examples of this in our code base (so say four dependencies rather than... gulp... 8-10), rather than the worst examples. We already knew the most egregious examples had some architectural and implementation issues and are flagged for re-implementation anyhow. What it seems to boil down to is that we've got a bunch of code in the wrong place. Sometimes methods in one class should really be in another class, and if code was relocated it would remove the need for a dependency. Sometimes we've trying to do too much in the class having too many dependencies, and the entire bit of functionality should be moved elsewhere. Sometimes we seem to be missing some intermediary service which should busy itself with some functionality. It then would be the one that would need 2-3 of the other dependencies, and it should be the dependency of the class under examination. All variations of "code in the wrong place", really. There was no one silver bullet to solve this problem, but on the other hand it wasn't taking us so long to work out "yeah... I think we need to do something about [that bit] there".
So - bottom line - our DI approach is correct, but our actual domain design could do with some work in places. Fortunately because we've got really solid test coverage, refactoring this stuff will actually be pretty safe. It'll also simplify a bunch of our code, and make for smaller class files. This is a win too.
This was a good way to end the week really. It was also good to see all the people of the team pitching in and being enthusiastic in the discussion too. I said it only a few days ago, but I'll say it again: we've got a good dev team.
Update:
I meant to include this link, but it was in my work email so wasn't accessible at time of writing: "When Dependency Injection goes Wrong" (Richard Miller). One of the lads circulated this towards close of play on Fri. It kinda bears out where we got to with our conversation. This is reassuring.Righto.
--
Adam
