Google Testing Blog: Dependency Injection Myth: Reference Passing

Dependency Injection Myth: Reference Passing

Wednesday, October 22, 2008

by Miško HeveryAfter reading the article on Singletons (the design anti-pattern) and how they are really global variables and dependency injection suggestion to simply pass in the reference to the singleton in a constructor (instead of looking them up in global state), many people incorrectly concluded that now they will have to pass the singleton all over the place. Let me demonstrate the myth with the following example.Let's say that you have a LoginPage which uses the UserRepository for authentication. The UserRepository in turn uses Database Singleton to get a hold of the global reference to the database connection, like this:class UserRepository {
private static final BY_USERNAME_SQL = "Select ...";

User loadUser(String user) {
Database db = Database.getInstance();
return db.query(BY_USERNAME_SQL, user);
}
}

class LoginPage {
UserRepository repo = new UserRepository();

login(String user, String pwd) {
User user = repo.loadUser(user);
if (user == null || user.checkPassword(pwd)) {
throw new IllegalLoginException();
}
}
}The first thought is that if you follow the advice of dependency injection you will have to pass in the Database into the LoginPage just so you can pass it to the UserRepository. The argument goes that this kind of coding will make the code hard to maintain, and understand. Let's see what it would look like after we get rid of the global variable Singleton Database look up.First, lets have a look at the UserRepository.class UserRepository {
private static final BY_USERNAME_SQL = "Select ...";
private final Database db;

UserRepository(Database db) {
this.db = db;
}

User loadUser(String user) {
return db.query(BY_USERNAME_SQL, user);
}
}Notice how the removal of Singleton global look up has cleaned up the code. This code is now easy to test since in a test we can instantiate a new UserRepository and pass in a fake database connection into the constructor. This improves testability. Before, we had no way to intercept the calls to the Database and hence could never test against a Database fake. Not only did we have no way of intercepting the calls to Database, we did not even know by looking at the API that Database is involved. (see Singletons are Pathological Lairs) I hope everyone would agree that this change of explicitly passing in a Database reference greatly improves the code.Now lets look what happens to the LoginPage...class LoginPage {
UserRepository repo;

LoginPage(Database db) {
repo = new UserRepository(db);
}

login(String user, String pwd) {
User user = repo.loadUser(user);
if (user == null || user.checkPassword(pwd)) {
throw new IllegalLoginException();
}
}
}Since UserRepository can no longer do a global look-up to get a hold of the Database it musk ask for it in the constructor. Since LoginPage is doing the construction it now needs to ask for the Databse so that it can pass it to the constructor of the UserRepository. The myth we are describing here says that this makes code hard to understand and maintain. Guess what?! The myth is correct! The code as it stands now is hard to maintain and understand. Does that mean that dependency injection is wrong? NO! it means that you only did half of the work! In how to think about the new operator we go into the details why it is important to separate your business logic from the new operators. Notice how the LoginPage violates this. It calls a new on UserRepository. The issue here is that LoginPage is breaking a the Law of Demeter. LoginPage is asking for the Database even though it itself has no need for the Database (This greatly hinders testability as explained here). You can tell since LoginPage does not invoke any method on the Database. This code, like the myth suggest, is bad! So how do we fix that?We fix it by doing more Dependency Injection.class LoginPage {
UserRepository repo;

LoginPage(UserRepository repo) {
this.repo = repo;
}

login(String user, String pwd) {
User user = repo.loadUser(user);
if (user == null || user.checkPassword(pwd)) {
throw new IllegalLoginException();
}
}
}LoginPage needs UserRepository. So instead of trying to construct the UserRepository itself, it should simply ask for the UserRepository in the constructor. The fact that UserRepository needs a reference to Database is not a concern of the LoginPage. Neither is it a concern of LoginPage how to construct a UserRepository. Notice how this LoginPage is now cleaner and easier to test. To test we can simply instantiate a LoginPage and pass in a fake UserRepository with which we can emulate what happens on successful login as well as on unsuccessful login and or exceptions. It also nicely takes care of the concern of this myth. Notice that every object simply knows about the objects it directly interacts with. There is no passing of objects reference just to get them into the right location where they are needed. If you get yourself into this myth then all it means is that you have not fully applied dependency injection.So here are the two rules of Dependency Injection:

Always ask for a reference! (don't create, or look-up a reference in global space aka Singleton design anti-pattern)

If you are asking for something which you are not directly using, than you are violating a Law of Demeter. Which really means that you are either trying to create the object yourself or the parameter should be passed in through the constructor instead of through a method call. (We can go more into this in another blog post)

So where have all the new operators gone you ask? Well we have already answered that question here. And with that I hope we have put the myth to rest!BTW, for those of you which are wondering why this is a common misconception the reason is that people incorrectly assume that the constructor dependency graph and the call graph are inherently identical (see this post). If you construct your objects in-line (as most developers do, see thinking about the new operator) then yes the two graphs are very similar. However, if you separate the object graph instantiation from the its execution, than the two graphs are independent. This independence is what allows us to inject the dependencies directly where they are needed without passing the reference through the intermediary collaborators.

4 comments :

Ross PattersonOctober 22, 2008 at 10:03:00 AM PDT
"Notice that every object simply knows about the objects it directly interacts with. There is no passing of objects reference just to get them into the right location where they are needed."

But that's not true. The caller of LoginPage(UserRepository repo) has no legitimate interest in the fact that LoginPage needs a UserRepository, but you're insisting on it doing so. I'll agree that it's not as bad as having everyone in the call stack above UserRepository() know about the Database, but it's still sub-optimal, especially if LoginPage() has a bazillion callers.
ReplyDelete
Replies
UnknownOctober 22, 2008 at 10:33:00 AM PDT
Um... how does LoginPage's caller have any knowledge of how LoginPage is constructed. The only caller of the constructor is the container. No application object should ever invoke new LoginPage(someUserRepo).

Sorry, the only way the above isn't true is if LoginPage is constructed many times - but if the "bazillions of callers" are performing construction, then you've missed the point. LoginPage should be provided to its callers, not constructed by its callers.

Tapestry is really nice, this way. As a web-framework, it provides all its page objects from its IoC container, so what I'm saying above is literally true for all pages. But even if you weren't using a web-framework that provided LoginPage from a container, you would still want to separate construction/wiring from invocation of functionality. If the code that constructs LoginPage knows about UserRepository, that's just fine, since it's an essential dependency. the point is the average calling class does NOT need to know about its internals, and wouldn't.
ReplyDelete
Replies
javin paulFebruary 17, 2012 at 10:49:00 PM PST
I agree with most of points but Singleton Pattern is very useful on many of situation and with Spring framework managing singleton for you its even more useful. but yes you do have point to thought about.
ReplyDelete
Replies
M2tMOctober 22, 2015 at 5:01:00 PM PDT
Singleton is terrible for these reasons (aside from hidden dependency which the author covered):

1) Singleton lifespan is not directly controlled. It is created on the first call to .Instance. This can create very strange situations where you simply call "MySingleton::instance();" without actually using the instance returned because the side effects of the singleton starting up are required (database connection, network connection, whatever).

2) Singleton objects cannot be inherited from because inside their own static method they directly bake in a new object of the base class. This makes it impossible to mock up a testing db connection, or even to set up either develop or production versions of these objects without trending towards monolithic base classes, or additional dependencies.

I've seen things like "MySingleton::Instance().Initialize(MySingletonDependencies)", seriously going down this path is actually terrifying, what happens if someone forgets that Initialize? What happens if you write this CORRECTLY, but later on someone else calls .instance before your call breaking the application. How does this thread? You can already detect the smell of rotting corpses.

3) What happens when you want more than one logger to handle debug cases? What happens when you need a second database connection? What happens when you need to spin up multiple instances of your application logic to run a montecarlo simulation? Oh yeah, fuck you, that's what happens.

Least importantly of all:

4) Your code has hidden dependencies baked in all over the place, classic global variable problem.

What do I suggest? Well, a ServiceLocator can help... It still has issues, it is still not ideal, but at least it supports inheritance:

http://stackoverflow.com/questions/4074154/when-should-the-singleton-pattern-not-be-used-besides-the-obvious/4074287#4074287
ReplyDelete
Replies

Add comment

The comments you read and contribute here belong only to the person who posted them. We reserve the right to remove off-topic comments.

Testing Blog

Dependency Injection Myth: Reference Passing

4 comments :

Labels

Archive

Feed