Breaking SOLID Principles in multiple implementation of an Interface

Question

I am facing a problem with dependency inversion in a factory method and it is also breaking Open Closed principle. My code looks like below codes

    public interface IWriter
    {
        void WriteToStorage(string data);
    }

    public class FileWriter : IWriter
    {
        public void WriteToStorage(string data)
        {
            //write to file
        }
    }

    public class DBWriter : IWriter
    {
        public void WriteToStorage(string data)
        {
            //write to DB
        }
    }

Now I an using a factory class to solve the object creation. It look like below code

public interface IFactory
{
    IWriter GetType(string outputType);
}

public class Factory : IFactory
{
    public IWriter GetType(string outputType)
    {
        IWriter writer = null;
        if (outputType.Equels("db"))
        {
            writer = new FileWriter();
        }
        else if (outputType.Equels("db"))
        {
            writer = new DBWriter();
        }
    }
}

Now the problem is the Factory class is breaking Open closed principle so it also breakes Dependency Inversion Principle

And then

public interface ISaveDataFlow
{
    void SaveData(string data, string outputType);
}

public class SaveDataFlow : ISaveDataFlow
{
    private IFactory _writerFactory = null;
    public SaveDataFlow(IFactory writerFactory)
    {
        _writerFactory = writerFactory;
    }
    public void SaveData(string data, string outputType)
    {
        IWriter writer = _writerFactory.GetType(outputType);
        writer.WriteToStorage(data);
    }
}

As the above factory class is breaking the dependency inversion I remove the Factory class and change the SaveDataFlow class like below

public class SaveDataFlow : ISaveDataFlow
{
    private IWriter _dbWriter = null;
    private IWriter _fileWriter = null;
    public SaveDataFlow([Dependency("DB")]IWriter dbWriter,
                        [Dependency("FILE")]IWriter fileWriter)
    {
        _dbWriter = dbWriter;
        _fileWriter = fileWriter;
    }
    public void SaveData(string data, string outputType)
    {
        if (outputType.Equals("DB"))
        {
            _dbWriter.WriteToStorage(data);
        }
        else if (outputType.Equals("FILE"))
        {
            _fileWriter.WriteToStorage(data);
        }
    }
}

And resolved those dependencies using Unity Framework

container.RegisterType<IWriter, DBWriter>("DB");
container.RegisterType<IWriter, FileWriter>("FILE");

Yet eventually I am ending up breaking Open Closed Principle. I need a better design/solution to solve such a problem yet I must follow SOLID Principles.

It seems to me like you don't want a factory. Actually, you want a strategy pattern. However, if you are doing `[Dependency("DB")]` you are coupling yourself to you DI framework which isn't ideal either. — Callum Linington, Jul 20 '16 at 08:24
Why `SaveDataFlow` needs to work off string input types? Can't you simply inject the proper writer? As for the factory, if you wish to make it extensible then you could use reflection to scan assemblies and find all implementors of `IWriter` in order to register them auto-magically. — plalx, Jul 21 '16 at 04:14

Callum Linington · Accepted Answer · 2016-07-20T09:02:56.533

I would simply turn it into a strategy pattern:

namespace UnityMutliTest
{
    using System;
    using System.Collections.Generic;
    using System.Linq;

    using Microsoft.Practices.Unity;

    class Program
    {
        static void Main(string[] args)
        {
            IUnityContainer container = new UnityContainer();

            container.RegisterType<IWriter, FileWriter>("file");
            container.RegisterType<IWriter, DbWriter>("db");

            container.RegisterType<IWriterSelector, WriterSelector>();

            var writerSelector = container.Resolve<IWriterSelector>();

            var writer = writerSelector.SelectWriter("FILE");

            writer.Write("Write me data");

            Console.WriteLine("Success");

            Console.ReadKey();
        }
    }

    interface IWriterSelector
    {
        IWriter SelectWriter(string output);
    }

    class WriterSelector : IWriterSelector
    {
        private readonly IEnumerable<IWriter> writers;

        public WriterSelector(IWriter[] writers)
        {
            this.writers = writers;
        }

        public IWriter SelectWriter(string output)
        {
            var writer = this.writers.FirstOrDefault(x => x.CanWrite(output));

            if (writer == null)
            {
                throw new NotImplementedException($"Couldn't find a writer for {output}");
            }

            return writer;
        }
    }

    interface IWriter
    {
        bool CanWrite(string output);

        void Write(string data);
    }

    class FileWriter : IWriter
    {
        public bool CanWrite(string output)
        {
            return output == "FILE";
        }

        public void Write(string data)
        {
        }
    }

    class DbWriter : IWriter
    {
        public bool CanWrite(string output)
        {
            return output == "DB";
        }

        public void Write(string data)
        {
        }
    }
}

You can have as many IWriters as you want, just register them:

container.RegisterType<IWriter, LogWriter>("log");

You can even implement decorators over the writers if you want as well.

You use the (badly named) IWriterSelector as the implementation on how to select your writer, this should be concerned with only getting a writer! The throw exception here is really useful, it will fail fast if there is no implementation that suits your needs!!

If you ever have Open Closed problems, either use Strategy or Template patterns to overcome.

I use this pattern all the time, to great effect.

I've created a little extension method to prevent you having to name your instances:

static class UnityExtensions
{
    public static void RegisterMultipleType<TInterface, TConcrete>(this IUnityContainer container)
    {
        var typeToBind = typeof(TConcrete);
        container.RegisterType(typeof(TInterface), typeToBind, typeToBind.Name);
    }
}

container.RegisterMultipleType<IWriter, FileWriter>();

Does Unity natively understand what to inject when you use IEnumerable writers ? In previous versions you needed to use an array. — smoksnes, Jul 20 '16 at 08:47
I installed version 4. This code was taken from a running console program — Callum Linington, Jul 20 '16 at 08:52
Cool. I never got it to work. That's why I suggested an array in my alternative 2 above. Great solution! — smoksnes, Jul 20 '16 at 08:53
Personally, I stay away from Unity. This works in Autofac, StructureMap, Ninject without named dependencies and `IEnumerable` — Callum Linington, Jul 20 '16 at 08:56
Why it would work with `Type[]` rather than `IEnumerable` is weird, seeing as array inherits `IEnumerable` — Callum Linington, Jul 20 '16 at 08:56
Yes, I know. It's weird. But for some reason Unity natively understands arrays, but not IEnumerable. Now your example looks very much like my alternative 2, but with a string instead of enum. — smoksnes, Jul 20 '16 at 08:58
I have updated answer with an extension that shows how to get around you explicitly typing the named part! — Callum Linington, Jul 20 '16 at 09:03
@CallumLinington First of all thanks for the solution. I got a small question, the implementations of `IWriter`interface (eq. `DbWriter` and `FileWriter`) holds 2 responsibilities. I was just trying to keep single responsibility. — Rahul Chakrabarty, Jul 20 '16 at 10:58
They don't hold two responsibilities... It only has 1, it will write to the desired place. Just because there is a method that states whether it can handle a particular type isn't really another responsibility. The thing you have to remember is that SOLID is a set of principles. Not set a of strict rules — Callum Linington, Jul 20 '16 at 11:01

score 1 · Answer 2 · edited May 23 '17 at 11:44

I tend to use one of these approaches.

1. Break into different interfaces

public interface IWriter
{
    void WriteToStorage(string data);
}

public interface IFileWriter : IWriter
{
}

public interface IDBWriter: IWriter
{
}

public class FileWriter : IFileWriter 
{
    public void WriteToStorage(string data)
    {
        //write to file
    }
}

public class DBWriter : IDBWriter
{
    public void WriteToStorage(string data)
    {
        //write to DB
    }
}

Pros: You can inject the correct implementation based on the interface, which doesn't break the OCP.

Cons: You have empty interfaces.

2. Use an enum to separate them (strategy pattern)

public interface IWriter
{
    void WriteToStorage(string data);
    StorageType WritesTo { get; }
}

public enum StorageType 
{
    Db = 1,
    File = 2
}

public class Factory : IFactory
{
    public IEnumerable<IWriter> _writers;

    public Factory(IWriter[] writers)
    {
        _writers = writers;
    }

    public IWriter GetType(StorageType outputType)
    {
        IWriter writer = _writers.FirstOrDefault(x => x.WritesTo == outputType);
        return writer;
    }
}

Pros: You can inject them both and then use the one you want by using the enum.

Cons: I guess it kinda breaks the OCP-principle the same way as in your first example.

More about the strategy pattern in this excellent answer from Mark Seemann.

3. Build a factory that creates items based on a func.

In your registration:

container.RegisterType<IWriter, DBWriter>("DB");
container.RegisterType<IWriter, FileWriter>("FILE");
container.RegisterType<IFactory, Factory>(
    new ContainerControlledLifetimeManager(),
    new InjectionConstructor(
        new Func<string, IWriter>(
            writesTo => container.Resolve<IWriter>(writesTo));

And your factory

public class Factory : IFactory
{
    private readonly Func<string, IWriter> _createFunc;

    public Factory(Func<string, IWriter> createFunc)
    {
        _createFunc = createFunc;
    }

    public IWriter CreateScope(string writesTo)
    {
        return _createFunc(writesTo);
    }
}

Pros: Moves the entire dependency to the registration.

Cons: A wrapper for a service-locator pattern. Can be a bit hard to read.

None of the examples above is perfect, as each of them has their pros and cons.

Similiar question here: Inject require object depends on condition in constructor injection

score 1 · Answer 3 · answered Jul 20 '16 at 08:15

Solution 1

Choose before instantiation and use scopes

using(var scope = new Scope(unity))
{
    scope.register<IWriter, ConcreteWriter>();
    var flow = scope.Resolve<ISaveDataFlow>();

}

Solution 2

Inject your strategy at runtime.

ISaveDataFlow flow = ....
IWriter writer = GetWriterBasedOnSomeCondition();
flow.SaveData(data, writer);

I suspect that solution 2 is closer to what you are trying to achieve. Remember, you don't need to pass around a string to describe the strategy you want to use.

You can instead pass around the actual strategy you want to use, in this case, the actual IWriter, you want to use.

Then what you can do instead is have metadata on each IWriter to help the user choose which IWriter to use.

For example

public interface IWriter
{
   void WriteData(data);
   string Name {get;}
}

void GetWriterBasedOnSomeCondition()
{
    Dictionary<string, IWriter> writers = ...ToDictionary(x => x.Name);
    var choice = Console.ReadLine();
    return writers[choice];
}

You wouldn't have a dictionary of stategy patterns, you would simply bind them all in the dependency framework and pass through the constructor `IEnumerable` — Callum Linington, Jul 20 '16 at 08:22
@CallumLinington You would. But then you use a dictionary to map back from the meta-data to the actual strategy. Alternatively you could use position in the `List`... But yes. DI would be involved in the `Chooser` class. — Aron, Jul 20 '16 at 08:25

score 0 · Answer 4 · answered Oct 27 '19 at 17:45

In .NET Core (it's not clear from the question what framework is being used), you can use the built-in DI to achieve the strategy pattern quite easily with very little code.

In Startup.ConfigureServices:

services
    .AddScoped<IWriter, FileWriter>()
    .AddScoped<IWriter, DBWriter>()
    .AddScoped<ISaveDataFlow, SaveDataFlow>();

Add an method to IWriter for the strategy algorithm:

public interface IWriter
{
    bool CanWrite(string outputType);
    void WriteToStorage(string data);
}

public class FileWriter : IWriter
{
    bool CanWrite(string outputType) => outputType == "FILE";
    public void WriteToStorage(string data) {}
}

public class DBWriter : IWriter
{
    bool CanWrite(string outputType) => outputType == "DB";
    public void WriteToStorage(string data) {}
}

Then change the constructor of SaveDataFlow to use a collection type, and change SaveData to call the algorithm method of all resolved IWriter types.

public class SaveDataFlow : ISaveDataFlow
{
    private readonly IWriter _writers;

    public SaveDataFlow(IEnumerable<IWriter> writers)
    {
        _writers= writers;
    }

    public void SaveData(string data, string outputType)
    {
        _writers.Single(w => w.CanWrite(outputType)).WriteToStorage(data);
    }
}

This now complies with the Open/Closed Principle as the concrete selection is only within the concrete classes themselves.

Breaking SOLID Principles in multiple implementation of an Interface

4 Answers4

Linked