2

Here's the basic idea of what we're doing:

  • The website collects data from a user in a form and saves it all to a row in a table, which we'll call FooObject.
  • FooObject is then passed into a business logic class
  • Based on a key in the app.config, a factory class will instantiate one of three concrete classes:
    • XMLStringA : IFooInterface
    • XMLStringB : IFooInterface
    • XMLStringC : IFooInterface
  • Each class has two methods:
    • GenerateXML(FooObject fooObject)
    • ParseResponse(string serviceCallResponse)
  • After the class in instantiated, we call GenerateXML(), pass in FooObject, get back an XML string, and then submit it to any one of three separate, external, third party web services. (The address of the service is also in the app.config.)
  • We get the response back, and send it into ParseResponse()

All's well. But, C has some extra requirements. When submitting the XML to this service, one of the XML elements requires an extensive lookup. We decided to add the data needed for that lookup to a table in our database. So, there is a private method in XMLStringC that uses EF to make a DB call and get the needed data to add to the XML string.

I was kind of aware that doing this violated the Single Responsibility principle, as these classes should really be doing nothing but building an XML string. The A and B classes don't make a call to the DB.

The possible folly of what I was doing was driven home when I tried to make a unit test to test A, B, and C. As we are not in context when running the unit test, C fails when trying to call the DB.

I'm not sure where to do this custom logic for C. On one hand, it only happens when we are going to submit to the C service, so it makes sense to do it inside the C class. On the other hand, I don't like making a database call from inside that class. Ultimately it might not matter, if I can figure out how to just unit test it and make it work.

What's the best practice way to do this?

Casey Crookston
  • 13,016
  • 24
  • 107
  • 193
  • Why not pass whatever C requires as a parameter to C instead of making it go to the database? That way, when unit testing you can just pass a mock object. – dmeglio Dec 16 '15 at 21:58
  • If I did that, then A,B and C would all need it. But A and B don't care about it. They all implement the same interface. – Casey Crookston Dec 16 '15 at 22:00
  • 1
    Not if you use an object to resolve it. Pass a class, `ExternalDependencyProvider` which has a method, `GetExternalData` which does the db lookup. In your app you build a subclass of this that goes to the db, in your unit test you pass a version of the class that returns a static object. If GetExternalData is never called (A and B) no harm is done, if it is (C), then you get your data. – dmeglio Dec 16 '15 at 22:06
  • That all makes sense but for one thing. When instantiating A B or C, I don't want logic to say "If C then pass in ExternalDependencyProvider". – Casey Crookston Dec 17 '15 at 14:37

1 Answers1

2

If I did that, then A,B and C would all need it. But A and B don't care about it. They all implement the same interface.

If you follow dependency injection best practices, your dependencies are not part of the interface, they are part of the object's constructor.

You are correct in your assessment that this violates the SRP. What you need is a service to do the lookup that is passed into C as a dependency. Then your services don't violate the SRP and you can still unit test your XMLStringC class.

public class XMLStringB : IFooInterface
{
    // No constructor defined here - we have no dependencies

    public string GenerateXML(FooObject fooObject)
    {
        // implementation here
    }

    public void ParseResponse(string serviceCallResponse)
    {
        // implementation here
    }
}

public class XMLStringC : IFooInterface
{
    private readonly IDatabaseLookupService databaseLookupService;

    public XMLStringC(IDatabaseLookupService databaseLookupService)
    {
        if (databaseLookupService == null)
            throw new ArgumentNullException("databaseLookupService");
        this.databaseLookupService = databaseLookupService;
    }

    public string GenerateXML(FooObject fooObject)
    {
        // Use this.databaseLookupService as needed.
        var data = this.databaseLookupService.Lookup(fooObject.ID);

        // implementation here
    }

    public void ParseResponse(string serviceCallResponse)
    {
        // Use this.databaseLookupService as needed.
        var data = this.databaseLookupService.Lookup(someID);

        // implementation here
    }
}

Your dependency on the database would then shift to the IDatabaseLookupService instead of being tied to your business logic.

NightOwl888
  • 55,572
  • 24
  • 139
  • 212
  • Thank you sir! This is helpful. – Casey Crookston Dec 17 '15 at 14:19
  • Ok, in looking at this further, I'm unclear on how to fully implement this. In the factory pattern where either A,B or C are instantiated, would I need to pass in the service IF I am instantiating C? The entire point of that factory is that it's doesn't care or need to know which it's going to implement until runtime. I don't want extra logic there to have to say "If C then pass in databaseLookupService." – Casey Crookston Dec 17 '15 at 14:28
  • A common way to implement is to pass the dependency injection container instance into the constructor of the factory, or to [pass in a factory method](http://stackoverflow.com/questions/31950362/factory-method-with-di-and-ioc#31956803), which would solve the problem elegantly. The first option assumes that the factory is part of your composition root. Remember, that the constructor is not part of the object interface, so the values (dependencies) you pass to different implementations can vary as needed - you only need to pass them through the interface if they vary at runtime. – NightOwl888 Dec 17 '15 at 15:09
  • Another thing you could do is combine the strategy pattern with the abstract factory pattern [as shown here](http://stackoverflow.com/questions/31950362/factory-method-with-di-and-ioc#31971691). (Related to my previous comment - see the definition of [composition root](http://blog.ploeh.dk/2011/07/28/CompositionRoot/)). – NightOwl888 Dec 17 '15 at 15:12