Constructor injection and default overloads

Question

Let's say we have

public interface ITimestampProvider
{
    DateTime GetTimestamp();
}

and a class which consumes it

public class Timestamped
{
    private ITimestampProvider _timestampProvider

    public Timestamped(ITimestampProvider timestampProvider)
    {
        // arg null check

        _timestampProvider = timestampProvider;
    }

    public DateTime Timestamp { get; private set; }

    public void Stamp()
    {
        this.Timestamp = _timestampProvider.GetTimestamp();
    }
}

and a default implementation of:

public sealed class SystemTimestampProvider : ITimestampProvider
{
    public DateTime GetTimestamp()
    {
        return DateTime.Now;
    }
}

Is it helpful or harfmful to introduce this constructor?

public Timestamped() : this(new SystemTimestampProvider())
{}

This is a general question, i.e. timestamping is not the interesting part.

Answer 1

i wouldn't provide that constructor. Doing so makes it far too easy to call new TimeStamped and get an instance with new SystemTimestampProvider() when your IoC may be configured to use OtherTimestampProvider().

End of the day you'll end up with one hell of a time trying to debug why you're getting the wrong timestamp.

If you only provide the first constructor you can do a simple find usages of SystemTimestampProvider to find out who is (wrongly) using that provider instead of the IoC configured Provider.

Answer 2

In general I don't think so... It depends on what you're using Dependency Injection for. When I use DI for unit testing, I do the same thing (more or less) by instantiating the production version of the dependant object when the injected instance is null... And then I have an overload that does not take a parameter and delegates to the one that does... I use the parameterless one for production code, and inject a test version for unit test methods...

If you're talking about a IOC container application, otoh, you need to be careful about interfering in what the configuration settings are telling the container to do in a way that's not clear ...

   public class EventsLogic
   { 
       private readonly IEventDAL ievtDal;
       public IEventDAL IEventDAL { get { return ievtDal; } }

       public EventsLogic(): this(null) {}
       public EventsLogic(IIEEWSDAL wsDal, IEventDAL evtDal)
       {
          ievtDal = evtDal ?? new EventDAL();
       }
    }

Answer 3

I try to avoid this - there are a few places where I've found it to be a useful design but more often than not I've found it just leads to me making mistakes that can be a little puzzling to work out.

The need for the default injected objects is greatly reduced by using a dependency injection container (I use StructureMap) to manage all this wiring - the DI container makes sure that you always get a concrete instance you can use.

The only place where I'm still tempted to use the constructor you suggest is in my unit tests but recently I've been getting far greater value out of using fake or mocked objects.

There are places where having the default dependant objects is the correct and useful design, but in general I'd say that you are just introducing tight coupling that doesn't add a lot of value.

Answer 4

It's neither helpful nor harmful. It poses an aesthetic problem in that you are limiting your DI to constructor injection only when your design may allow for property setter injection.

Another option would be to implement a getter that returns a default implementation:

public DateTime Timestamp
{
    get { return _timestampProvider??new SystemTimestampProvider(); }
    set { _timestampProvider = value; }
}

Alternately, you can implement the above using a singleton if you're worried about creating too many objects in the heap.

Answer 5

I think it depends on the scenario, and is basically a function of who the consumer the code is (library vs. application) and whether you're using an IoC container or not.

• If you're using an IoC container, and this is not part of a public API, then let the container do the heavy lifting, and just have the single constructor. Adding the no-args constructor just makes things confusing, since you'll never use it.

• If this is part of a public API, then keep both. If you're using IoC, just make sure your IoC finds the "greediest" constructor (the one with the most arguments). Folks not using IoC, but using your API will appreciate not having to construct an entire dependency graph in order to use your object.

• If you're not using an IoC container, but just want to to unit test with a mock, keep the no-args constructor, and make the greedy constructor internal. Add InternalsVisibleTo for your unit test assembly so that it can use the greedy constructor. If you're just unit testing, then you don't need the extra public API surface.

Answer 6

My team has a great deal of success using this method. I recommend one change:
Make _timestampProvider readonly. This forces the provider to be deterministic at construction and will eliminate bugs.

public class Timestamped
{
    private readonly ITimestampProvider _timestampProvider;

    public Timestamped(ITimestampProvider timestampProvider)
    {
        _timestampProvider = timestampProvider;
    }

    public Timestamped(): this(new SystemTimestampProvider())
    { }
}

That said, we are always looking at new technologies, including DI frameworks. If we ever abandon this technique for something significantly better I'll let you know.