Assuming an IReader interface, an implementation of the IReader interface ReaderImplementation, and a class ReaderConsumer that consumes and processes data from the reader.

public interface IReader
{
     object Read()
}

Implementation

public class ReaderImplementation
{
    ...
    public object Read()
    {
        ...
    }
}

Consumer:

public class ReaderConsumer()
{
    public string location

    // constructor
    public ReaderConsumer()
    {
        ...
    }

    // read some data
    public object ReadData()
    {
        IReader reader = new ReaderImplementation(this.location)
        data = reader.Read()
        ...
        return processedData    
    }
}

For testing ReaderConsumer and the processing I use a mock of IReader. So ReaderConsumer becomes:

public class ReaderConsumer()
{
    private IReader reader = null

    public string location

    // constructor
    public ReaderConsumer()
    {
        ...
    }

    // mock constructor
    public ReaderConsumer(IReader reader)
    {
        this.reader = reader
    }

    // read some data
    public object ReadData()
    {
        try
        {
            if(this.reader == null)
            {
                 this.reader = new ReaderImplementation(this.location)
            }

            data = reader.Read()
            ...
            return processedData    
        }
        finally
        {
            this.reader = null
        }
    }
}

In this solution mocking introduces an if sentence for the production code since only the mocking constructor supplies an instances of the interface.

During writing this I realise that the try-finally block is somewhat unrelated since it is there to handle the user changing the location during application run time.

Overall it feels smelly, how might it be handled better?

  • 16
    Typically, this is not a problem because the constructor with the dependency injected, would be the only constructor. Is it out of the question to make ReaderConsumer independent on ReaderImplementation? – Chris Wohlert Jan 23 '17 at 10:04
  • Currently it would be hard to remove the dependency. By looking at it a bit more I have a deeper problem than just the dependency on ReaderImplemenatation. Since ReaderConsumer is a created during startup from a factory, persist through the lifetime of the application, and accepts changes from the users its requires some extra massaging. Probably the configuration/user input could exist as an object and then ReaderConsumer and ReaderImplementation could be created on the fly instead. Both given answers solves the more generic case pretty well. – kristian mo Jan 23 '17 at 12:32
  • 3
    Yes. This is the point of TDD: having to write tests first implies a more decoupled design (otherwise you are not able to write unit tests...). This helps to make the code more maintainable and also extensible. – Bakuriu Jan 23 '17 at 20:47
  • A good way to detect smells that can be solved with Dependency Injection, is looking for the keyword 'new'. Don't new up your dependencies. Inject them instead. – Eternal21 Jan 25 '17 at 13:25
up vote 67 down vote accepted

Instead of initializing the reader from your method, move this line

{
    this.reader = new ReaderImplementation(this.location)
}

Into the default parameterless constructor.

public ReaderConsumer()
{
    this.reader = new ReaderImplementation(this.location)
}

public ReaderConsumer(IReader reader)
{
    this.reader = reader
}

There is no such thing as a "mock constructor", if your class has a dependency that it requires in order to work, then the constructor should either be provided that thing, or create it.

  • 3
    score++ ...except from a DI standpoint, that default constructor is definitely a code smell. – Mathieu Guindon Jan 23 '17 at 19:04
  • 3
    @Mat'sMug nothing wrong with a default implementation. The lack of ctor chaining is the smell here. =;)- – RubberDuck Jan 23 '17 at 19:08
  • Oh, yes there is - if you don't have the book, this article seems to describe the bastard injection anti-pattern pretty well (just skimmed over though). – Mathieu Guindon Jan 23 '17 at 19:10
  • 2
    @Mat'sMug: You're interpreting DI dogmatically. If you don't need to inject the default constructor, then you don't. – Robert Harvey Jan 23 '17 at 19:27
  • 3
    The book @Mat'sMug linked also talks about "acceptable defaults" for dependencies being just fine (although it's referring to property injection). Let's put it this way: the two-constructor approach costs more in terms of simplicity and maintainability than the one-constructor approach, and with the question being over-simplified for clarity, the cost is not justified, but it may be in some cases. – Carl Leth Jan 24 '17 at 18:17

You only need the single constructor:

public class ReaderConsumer()
{
    private IReader reader = null

    public string location

    // constructor
    public ReaderConsumer(IReader reader)
    {
        this.reader = reader;
    }

in your production code:

var rc = new ReaderConsumer(new ReaderImplementation(0));

in your test:

var rc = new ReaderConsumer(new MockImplementation(0));

Look into Dependency Injection and Inversion of Control

Both Ewan and RubberDuck have excellent answers. But I wanted to mention another area to look into which is Dependency Injection (DI) and Inversion of Control (IoC). Both of these approaches moves the problem you're experiencing into a framework/library so that you don't have to worry about it.

Your example is simple, and is quickly dispensed with, but, inevitably you're going to build upon it and you'll end up with either tons of constructors or initialization routines that look like:

var foo = new Foo(new Bar(new Baz(), new Quz()), new Foo2());

With DI/IoC you use a library which allows you to spell out the rules for matching interfaces to implementations and then you just say "Give me a Foo" and it works out how to wire it all up.

There are lots of very friendly IoC Containers (as they are called) out there, and I'm going to recommend one to look at, but, please explore, as there are very many fine choices.

A simple one to start with is:

http://www.ninject.org/

Here's a list to explore:

http://www.hanselman.com/blog/ListOfNETDependencyInjectionContainersIOC.aspx

  • 7
    Both Ewan's and RubberDuck's answers demonstrate DI. DI/IoC isn't about a tool or a framework, it's about architecting and structuring the code in such a way that control is inverted and dependencies are injected. Mark Seemann's book does a great (awesome!) job at explaining how DI/IoC is completely achievable without an IoC framework, and why and when an IoC framework becomes a good idea (hint: when you have dependencies of dependencies of dependencies of ...) =) – Mathieu Guindon Jan 23 '17 at 19:08
  • Also... +1 because Ninject is awesome, and upon re-reading your answer seems fine. The first paragraph reads a bit like Ewan's and RubberDuck's answers aren't about DI though, which is what prompted my first comment. – Mathieu Guindon Jan 23 '17 at 19:21
  • 1
    @Mat'sMug Definitely get your point. I was trying to encourage the OP to look into DI/IoC which the other posters were, in effect, using (Ewan's is Poor Man's DI), but didn't mention. The advantage, of course, to using a framework is that it will immerse the user in the world of DI with lots of concrete examples which hopefully will guide them to understand what they're doing... though... not always. :-) And, of course, I loved Mark Seemann's book. It's one of my favorites. – Reginald Blue Jan 23 '17 at 19:36
  • Thank you for the tip about a DI framework, it seems to be the way to refactor this mess properly :) – kristian mo Jan 24 '17 at 11:58

Your Answer

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

Not the answer you're looking for? Browse other questions tagged or ask your own question.