To constructor or to property dependency?

Us, developers, are a bit like that comic strip (from always great xkcd):

Crazy Straws

We can endlessly debate over tabs versus spaces (don’t even get me started), whether to use optional semicolon or not, and other seemingly irrelevant topics. We can have heated, informed debates with a lot of merit, or (much more often) not very constructive exchanges of opinions.

I mention that to explicitly point out, while this post might be perceived as one of such discussions, the goal of it is not to be a flamewar bait, but to share some experience.

So having said that, let’s cut to the chase.

Dependencies, mechanics and semantics

Writing software in an object oriented language, like C#, following established practices (SOLID etc) we tend to end up with many types, concentrated on doing one thing, and delegating the rest to others.

Let’s take an artificial example of a class that is supposed to handle scenario where a user moved and we need to update their address.

public class UserService: IUserService
{
   // some other members

   public void UserMoved(UserMovedCommand command)
   {
      var user = session.Get<User>(command.UserId);

      logger.Info("Updating address for user {0} from {1} to {2}", user.Id, user.Address, command.Address);

      user.UpdateAddress(command.Address);

      bus.Publish(new UserAddressUpdated(user.Id, user.Address));
   }
}

There are four lines of code in this method, and three different dependencies are in use: session, logger and bus. As an author of this code, you have a few options of supplying those dependencies, two of which that we’re going to concentrate on (and by far the most popular) are constructor dependencies and property dependencies.

Traditional school of thought

Traditional approach to that problem among C# developers goes something like this:

Use constructor for required dependencies and properties for optional dependencies.

This option is by far the most popular and I used to follow it myself for a long time. Recently however, at first unconsciously, I moved away from it.

While in theory it’s a neat, arbitrary rule that’s easy to follow, in practice I found it is based on a flawed premise. The premise is this:

By looking at the class’ constructor and properties you will be able to easily see the minimal set of required dependencies (those that go into the constructor) and optional set that can be supplied, but the class doesn’t require them (those are exposed as properties).

Following the rule we might build our class like that:

public class UserService: IUserService
{
   // some other members

   public UserService(ISession session, IBus bus)
   {
      //the obvious
   }

   public ILogger Logger {get; set;}
}

This assumes that session and bus are required and logger is not (usually it would be initialised to some sort of NullLogger).

In practice I noticed a few things that make usefulness of this rule questionable:

  • It ignores constructor overloads. If I have another constructor that takes just session does it mean bus is an optional or mandatory dependency? Even without overloading, in C# 4 or later, I can default my constructor parameters to null. Does it make them required or mandatory?
  • It ignores the fact that in reality I very rarely, if ever, have really optional dependencies. Notice the first code sample assumes all of its dependencies, including logger, are not null. If it was truly optional, I should probably protect myself from NullReferenceExceptions, and in the process completely destroy readability of the method, allowing it to grow in size for no real benefit.
  • It ignores the fact that I will almost never construct instances of those classes myself, delegating this task to my IoC container. Most mature IoC containers are able to handle constructor overloads and defaulted constructor parameters, as well as making properties required, rendering the argument about required versus optional moot.

Another, more practical rule, is this:

Use constructor for not-changing dependencies and properties for ones that can change during object’s lifetime.

In other words, session and bus would end up being private readonly fields – the C# compiler would enforce that once we set them (optionally validating them first) the fields in the constructor, we are guaranteed to be dealing with the same (correct, not null) objects ever after. On the other hand, Logger is up in the air, since technically at any point in time someone can swap it for a different instance, or set it to null. Therefore, what usually logically follows from there, is that property dependencies should be avoided and everything should go through the constructor.

I used to be the follower of this rule until quite recently, but then it does have its flaws as well.

  • It leads to some nasty code in subclasses where the base class has dependencies. One example I saw recently was a WPF view model base class with dependency on dispatcher. Because of that every single (and there were many) view model inheriting from it, needed to have a constructor declared that takes dispatcher and passes it up to the base constructor. Now imagine what happens when you find you need event aggregator in every view model. You will need to alter every single view model class you have to add that, and that’s a refactoring ReSharper will not aid you with.
  • It trusts the compiler more than developers. Just because a setter is public, doesn’t mean the developers will write code setting it to some random values all over the place. It is all a matter of conventions used in your particular team. On the team I’m currently working with the rule that everybody knows and follows is we do not use properties, even settable ones, to reassign dependencies, we’re using methods for that. Therefore, based on the assumption that developers can be trusted, when reading the code and seeing a property I know it won’t be used to change state, so readability and immediate understanding of the code does not suffer.

In reality, I don’t actually think the current one, or few of my last projects had even a requirement to swap service dependencies. Generally you will direct your dependency graphs from more to less volatile objects (that is object will depend on objects with equal or longer lifespan). In few cases where that’s not the case the dependencies would be pulled from a factory, and used only within the scope of a single method, therefore not being available to the outside world. The only scenario where a long-lived dependency would be swapped that I can think of, is some sort of failover or circuit-breaker, but then again, that would be likely dealt with internally, inside the component.

So, looking back at the code I tend to write, all dependencies tend to be mandatory, and all of them tend to not change after the object has been created.

What then?

This robs the aforementioned approaches from their advertised benefits. As such I tend to draw the division along different lines. It does work for me quite well so far, and in my mind, offers a few benefits. The rule I follow these days can be explained as follows:

Use constructor for application-level dependencies and properties for infrastructure or integration dependencies.

In other words, the dependencies that are part of the essence of what the type is doing go into the constructor, and dependencies that are part of the “background” go via properties.

Using our example from before, with this approach the session would come via constructor. It is part of the essence of what the class is doing, being used to obtain the very user whose address information we want to update. Logging and publishing the information onto the bus are somewhat less essential to the task, being more of an infrastructure concerns, therefore bus and logger would be exposed as properties.

  • This approach clearly puts distinction between what’s essential, business logic, and what is bookkeeping. Properties and fields have different naming conventions, and (especially with ReSharper’s ‘Color identifiers’ option) with my code colouring scheme have significantly different colours. This makes is much easier to find what really is important in code.
  • Given the infrastructure level dependencies tend to be pushed up to base classes (like in the ViewModel example with Dispatcher and EventAggregator) the inheritors end up being leaner and more readable.

In closing

So there you have it. It may be less pure, but trades purity for readability and reduces amount of boilerplate code, and that’s a tradeoff I am happy to make.

13 Thoughts.

  1. I like it. It makes things much cleaner. I think that you said aloud something that many people were thinking of but were afraid to question so well established ‘principles’ (thout shall inject via constructor). 

    What I would like even more is language-level support for IoC so that you don’t define constructors but rather then dependencies of each class (possibly in inheritance chanin). Say Foo inherits from Bar, Foo depends on ILog and Bar depends on IBus. I would like to able to express Foo’s depencies without need to think about Bar’s dependencies.

  2. Instead of injecting infrastructure dependencies through property injection, wouldn’t you want to add those dependencies using decorators or interception? This keeps the service clean of any infrastructural dependencies at all, and allows us to add cross-cutting concerns without making changes to the original services, thus adhering to the open/closed principle. Those infrastructure dependencies can in that case simply be injected as constructor arguments of the decorator (or interceptor).

    •  That was my first idea too. But have a look at the data that is needed for the single steps. You would need to load the user from the repository once for logging and once for the notification event sent via the bus (given that you want to separate these aspects completely as they don’t belong together). That wouldn’t hurt if you have an in-memory collection (I think that was the original definition of the repository pattern) but if you load from some persistent store it would hurt a lot. If you define decorators you would need to inject the repository there as well (same instance? same in-memory collection? initialization costs of new instance? …). Getting this kind of dependency (repository, bus) inside an interceptor might or might not be a problem depending on the container you use. Decorators would mean a lot of extra coding.

    • Leveraging AOP techniques can provide a powerful solution in some scenarios. However, pulling infrastructure dependencies out this way requires a high level of abstraction in the interceptor. In the case of logging, I don’t always want that particular dependency to not only be required but also automatically implemented on every class that happens to implement an interface.

      Even if I were clever enough to glean every bit of information I may want to log from an invocation (specific properties, exception priority, etc …) that interceptor could become a daunting bit of code and entirely out of context in the eyes of a new developer to the team.From Kozmic’s closing points I can see he doesn’t claim the advantages include strictly adhering to a well known pattern.

  3.  Instead of injecting infrastructure dependencies through property
    injection, wouldn’t you want to add those dependencies using decorators
    or interception? This keeps the service clean of any infrastructural
    dependencies at all, and allows us to add cross-cutting concerns without
    making changes to the original services, thus adhering to the
    open/closed principle. Those infrastructure dependencies can in that
    case simply be injected as constructor arguments of the decorator (or
    interceptor).

  4. I prefer constructor DI because it provides a kind of design feedback that can be easily overlooked with property injection. Specifically, if my class has a number of dependencies, I have to deal with those any time I want to create an instance (which happens when I write tests). The pain associated with creation is a direct result of a cohesion problem in my class. Too many dependencies, too many responsibilities, perhaps missing abstractions.

    In your example, the service has a cohesion problem, hence the feeling that we need treat the logger dependency differently. If it is not fundamental to the nature of the class, it doesn’t belong there. In fact, I might even go so far as to say that there is a missing abstraction, implementations of which could handle writing to the bus (infrastructure) and logging (infrastructure).

    It is encouraging to me that I can rely on this kind of feedback using constructor DI. If it becomes painful, I know that I have a problem.

  5. I’m not sure how this cleans anything up. You still have to litter your code with null checks. I’d much rather do null checks in the constructor only, and make every dependency required. If we want to support null-like behavior, it’s best simply to create an implementation that follows the null object pattern. This convention supports the cleanest code.

    • In the case of the ILogger dependency, do we need to “litter our code” with null checks? The protected property may still be initialized to a empty/null implementation. This is, in fact, what you’re referring to as the null-object pattern at work.

      • Hmm. My concern with that is coupling. How does each component know which null ILogger implementation to initialize to?

        Must each component that depends on ILogger implement their own null ILogger? That would move the litter to the namespace level.

        Do we have to provide a null implementation alongside every service interface? More litter. Or does a service interface know that it is “infrastructural” and will be populated via setters, and therefore only those will provide a null implementation? Now we’re leaving it in the hands of the component developers to know that if a null implementation is provided, they need to use setter injection and initialize to the null implementation.

        Also, what about performance? If we’re resolving a lot of these components with a ILogger dependency and setting a custom ILogger each time, we’re wasting cycles on the unnecessary NullLogger instantiation. To get around this, the component author will statically hold onto a single NullLogger. More boilerplate that must be implemented anywhere you have “infrastructural” dependencies. And technically, we’re still wasting cycles with the reassignment to the custom ILogger.

        All of this creates a more complicated convention than simply having constructor injection, and allowing the composition root to define a null implementation only when necessary.

        • Argh. I re-read what you wrote and I see that you meant the container would populate a null implementation via the setter. I tried to delete my comment but disqus sucks too much and only removed my name.

          Sure, you can have the container populate the null implementation. But how can the component be *guaranteed* of that?  If you practice defensive coding, you will have to insert null checks everywhere. The alternative could be that the component uses a getter for the ILogger which could return its own null implementation if the setter was never used. But this returns us to the problem of more boilerplate.

  6. Well done and I like the re-examination of old and possible flawed ideas. There is nothing worse than having a lot of inheritors and the having to change the base constructor just because everyone needs an event aggregator.

  7. Pingback: Property Injection for infrastructural dependencies in practice, part 1 « A place for spare thoughts

Comments are closed.