What do you look for in a dependency to determine if it should be an injected dependency?

Question

I am having difficult figuring out when a dependency should be injected. Let's just work with a simple example from my project:

class CompanyDetailProvider : ICompanyDetailProvider {
    private readonly FilePathProvider provider;
    public CompanyDetailProvider(FilePathProvider provider) {
        this.provider = provider;
    }

    public IEnumerable<CompanyDetail> GetCompanyDetailsForDate(DateTime date) {
        string path = this.provider.GetCompanyDetailFilePathForDate(date);
        var factory = new DataReaderFactory();
        Func<IDataReader> sourceProvider = () => factory.CreateReader(
            DataFileType.FlatFile, 
            path
        );
        var hydrator = new Hydrator<CompanyDetail>(sourceProvider);
        return hydrator;
    }
}

(Not production quality!)

ICompanyDetailProvider is responsible for providing instances of CompanyDetails for consumers. The concrete implementation CompanyDetailProvider does it by hydrating instances of CompanyDetail from a file using a Hydrator<T> which uses reflection to populate instances of T sourced from an IDataReader. Clearly CompanyDetailProvider is dependent on DataReaderFactory (which returns instances of OleDbDataReader given a path to a file) and Hydrator. Should these dependencies be injected? Is it right to inject FilePathProvider? What qualities do I examine to decide if they should be injected?

Answer 1

How to determine if a class should use dependency injection

---

Does this class require an external dependency?

If yes, inject.

If no, has no dependency.

To answer "Is it right to inject FilePathProvider?" yes it is right.

Edit: For clarification any external dependency is where you call to an unrelated but dependent class, especially when it involves a physical resources such as reading File Pathes from disk, but this also implies any kind of service or model class that does logic indepedent to the core functionality of the class.

Generally this be surmised with anytime you call the new operator. In most circumstances you want to refactor away all usages of the new operator when it has to deal with any class other than a data transfer object. When the class is internal to the usage location then a new statement can be fine if it reduces complexity such as the new DataReaderFactory() however this does appear to be a very good candidate for constructor injection also.

Answer 2

I tend to be on the more liberal side of injecting dependencies so I would definitely want to inject both IDataReader to get rid of the new DataFactoryReader and the Hydrator. It keeps everything more loosely coupled which of course makes it easier to maintain.

Another benefit that is easy to attain right away is better testability. You can create mocks of your IDataReader and Hydrator to isolate your unit tests to just the GetCompanyDetailsForDate method and not have to worry about what happens inside the datareader and hydrator.

Answer 3

I evaluate dependencies' points of use through the intent/mechanism lens: is this code clearly communicating its intent, or do I have to extract that from a pile of implementation details?

If the code indeed looks like a pile of implementation details, I determine the inputs and outputs and create an entirely new dependency to represent the why behind all the how. I then push the complexity into the new dependency, making the original code simpler and clearer.

When I read the code in this question, I clearly see the retrieval of a file path based on a date, followed by an opaque set of statements which don't clearly communicate the goal of reading an entity of a certain type at a certain path. I can work my way through it but that breaks my stride.

I suggest you raise the level of abstraction of the second half of the calculation, after you get the path. I would start by defining a dependency which implements the code's inputs/outputs:

public interface IEntityReader
{
    IEnumerable<T> ReadEntities<T>(string path);
}

Then, rewrite the original class using this intention-revealing interface:

public sealed class CompanyDetailProvider : ICompanyDetailProvider
{
    private readonly IFilePathProvider _filePathProvider;
    private readonly IEntityReader _entityReader;

    public CompanyDetailProvider(IFilePathProvider filePathProvider, IEntityReader entityReader)
    {
        _filePathProvider = filePathProvider;
        _entityReader = entityReader;
    }

    public IEnumerable<CompanyDetail> GetCompanyDetailsForDate(DateTime date)
    {
        var path = _filePathProvider.GetCompanyDetailsFilePathForDate(date);

        return _entityReader.ReadEntities<CompanyDetail>(path);
    }
}

Now you can sandbox the gory details, which become quite cohesive in isolation:

public sealed class EntityReader : IEntityReader
{
    private readonly IDataReaderFactory _dataReaderFactory;

    public EntityReader(IDataReaderFactory dataReaderFactory)
    {
        _dataReaderFactory = dataReaderFactory;
    }

    public IEnumerable<T> ReadEntities<T>(string path)
    {
        Func<IDataReader> sourceProvider =
            () => _dataReaderFactory.CreateReader(DataFileType.FlatFile, path);

        return new Hydrator<T>(sourceProvider);
    }
}

As shown in this example, I think you should abstract the data reader factory away and directly instantiate the hydrator. The distinction is that EntityReader uses the data reader factory, while it only creates the hydrator. It isn't actually dependent on the instance at all; instead, it serves as a hydrator factory.