Maybe Don't Inject That

I’m actually always suspicious when I’m writing up a pattern or a technique if I can’t talk about reasons when you shouldn’t use and trade-offs against it. If I can’t find arguments against it, then I’m worrying that I’m not really analyzing things properly.

–Martin Fowler, “Is TDD Dead?”, 35:00

DI is useful. It’s great. It makes your code more flexible, testable, etc. Dagger? Also useful. Great. My favorite DI library.

However, in my reflection on the past 2+ years of dagger usage, I said that if I had a do-over for my dagger implementation, I would probably use DI less. Here I want to say a little more about why. Here’s the gist: DI costs something: it complicates calling code, and in many cases, I and my team were ignoring that cost. I also want to say a bit about how you can not use DI and still have testable code.

Let’s look at an easy example where I think we’ll all agree that DI isn’t worth it. It’s a piece of code from the Google I/O app source:

open class SnackbarMessageManager @Inject constructor(
    //...
) {
  //...

  private val messages = mutableListOf<Event<SnackbarMessage>>()

  fun addMessage(msg: SnackbarMessage) {
    // Bunch of code that calls methods on this.messages
  }
}

Although this is technically a violation of the dependency inversion principle (this class depends on an ArrayList instead of List), I don’t think anyone would have a problem with this.

Why isn’t the lack of DI a problem here? Well, a part of the reason is that its far-fetched to think we’d ever use another implementation of List here. Another reason: its not worth complicating the calling code by asking every client to pass in their own implementation of a List when that implementation should always be an ArrayList. It’s not even worth the trouble to add a default parameter that gives clients the option.

So, we did a simple cost benefit analysis here: Even though there’s an extremely remote possibility that somehow, someday it will be useful for someone to be able to swap out the implementation of a list (good), the DI complicates the calling code (bad), so using DI is a bad trade here.

Now, let’s look at another example from the Google I/O codebase:

class FeedViewModel @Inject constructor(
    private val loadCurrentMomentUseCase: LoadCurrentMomentUseCase,
    //...
) : ViewModel(),

The LoadCurrentMomentUseCase is injected. Clean coders rejoice! Hexagonal Architecture fans high-five each other! But wait a minute: how different is this class from SnackbarMessageManager? How likely is it that clients will need to pass in another implementation of LoadCurrentMomentUseCase?

Not likely. Its so unlikely, in fact, that the authors didn’t even think to make LoadCurrentMomentUseCase an interface. Needing another implementation of LoadCurrentMomentUseCase is probably about as likely as SnackbarMessageManager needing another implementation of List.

This leaves us with a puzzle 🤔: Why is it worth complicating the calling code by injecting this dependency in this case, but not the previous case?

“BECAUSE TESTING!”

Someone’s brain is shouting that right now. Yep. I get it. Using DI helps with testing, but here’s the thing: SnackbarMessageManager’s interaction with messages is actually more complicated and therefore more in need of testing than FeedViewModel’s interaction with LoadCurrentMomentUseCase. Compare and see for yourself:

class FeedViewModel {
  init {
    //...
    currentMomentResult.addSource(conferenceStateLiveData) {
    // This is the only use of the dependency!
    loadCurrentMomentUseCase(timeProvider.now(), currentMomentResult)
    }
  }

// vs.

class SnackbarMessageManager {
  fun addMessage(msg: SnackbarMessage) {
    //...      
    val sameRequestId = messages.filter {
        it.peekContent().requestChangeId == msg.requestChangeId && !it.hasBeenHandled
    }
    if (sameRequestId.isNotEmpty()) {
        messages.removeAll(sameRequestId)
    }
    
    val alreadyHandledWithSameId = messages.filter {
        it.peekContent().requestChangeId == msg.requestChangeId && it.hasBeenHandled
    }

    if (alreadyHandledWithSameId.isEmpty()) {
        messages.add(Event(msg))
        loadNextMessage()
    }

    if (messages.size > MAX_ITEMS) {
        messages.retainAll(messages.drop(messages.size - MAX_ITEMS))
    }
  }
}

EDIT: Ryan Harter is pointed out that this example isn’t very good since testing the interaction SnackbarMessageManager with messages would be a bad way to test this class. I think its easy to imagine a better pair of examples. If you’re having trouble, feel free to tweet at me. I’ll probably update this post eventually with a better set of examples.

In this particular case, testing can’t be the reason why we should use DI in one case and not the other. Setting aside these examples, in general, I suspect that we don’t consistently apply our DI rule:

If this class is likely to need another implementation of a dependency OR if we need to test this class’ interaction with a dependency use DI, otherwise, its not worth it to complicate the calling code.

Instead, I think a lot of us, (myself included until recently) use DI by default when creating a class without even thinking about the tradeoff.

Why does this matter?

Making bad trades over and over means object graph creation will be more and more complicated. In the short-term, this isn’t great. Needless complication is bad. But as we use more and more DI, manual object graph creation becomes impractical and this pushes in the direction of adopting DI libraries like Dagger, which have a cost. The team has to learn them, and if you’re using Dagger specifically, you have to deal with slower build times.

I think its possible that a more judicious use of DI may lead some teams to conclusion that they don’t need a DI library yet.

Alternatives for DI for testing

Even if we don’t want to swap out implementations of dependencies, sometimes we get excited about dependency injection because of how it helps with testing. There are ways of supporting testing without using DI. One suggested way of doing this actually comes from Growing Object Oriented Software Guided by Tests, a book blessed by Kent Beck, and it involves the use of something they call an “adjustment”:

Peers that adjust the object’s behavior to the wider needs of the system. This includes policy objects that make decisions on the object’s behalf (the Strategy pattern in GoF) and component parts of the object if it’s a composite.¹

Here’s an example of turning LoadCurrentMomentUseCase from FeedViewModel into an adjustment:

class FeedViewModel @Inject constructor(    
    //...
) : ViewModel() {
  var loadCurrentMomentUseCase = LoadCurrentMomentUseCase()    
}

If you want to swap out a mock, you can, no problem:

@Test test() {
  val model = FeedViewModel().apply {
    loadCurrentMomentUseCase = mock(LoadCurrentMomentUseCase::class.java)
  }
  // Test stuff
}

Some of you are 😱right now. I get it. The mutability offends my sensibilities too. But, the calling code is simpler now. What’s the better trade here? It’s not obvious that the answer is always immutability.

You sure immutability is needed? No worries. You still have options. What about a default constructor parameter:

class FeedViewModel @Inject constructor(    
    val loadCurrentMomentUseCase = LoadCurrentMomentUseCase()
) : ViewModel()

Calling code is simpler. Test code still has the hook it needs to pass in a mock. Technically DI, is still be used in the test case, but we aren’t using it for application code.

There’s one more clever trick to get testable code without DI that I’ve seen in Working Effectively with Legacy Code and the mockito wiki:

class FeedViewModel @Inject constructor(    
    //...
) : ViewModel() {
  init {
    val loadCurrentMomentUseCase = makeLoadCurrentMomentUseCase()
  }

  fun makeLoadCurrentMomentUseCase() = LoadCurrentMomentUseCase()
}

In your test, you can just override makeLoadCurrentMomentUseCase or create a spy of FeedViewModel and stub out makeLoadCurrentMomentUseCase.

Some folks may feel that these alternatives mix testing infrastructure code with normal application code. Remember: if the only reason you’re introducing DI is to support testing, you’re already doing this.

So, the next time you’re writing up a class, maybe consider not injecting that Foo. You’ve got alternatives that allow you to test your code without complicating your object graph creation to the point where you need to use a DI library.

Notes