Interface segregation and single responsibility principle woes

Question

I'm trying to follow the Interface Segregation and Single Responsibility principles however I'm getting confused as to how to bring it all together.

Here I have an example of a few interfaces I have split up into smaller, more directed interfaces:

public interface IDataRead
{
    TModel Get<TModel>(int id);
}

public interface IDataWrite
{
    void Save<TModel>(TModel model);
}

public interface IDataDelete
{        
    void Delete<TModel>(int id);
    void Delete<TModel>(TModel model);
}

I have simplified it slightly (there were some where clauses that hindered readability).

Currently I am using SQLite, however, the beauty of this pattern is that it will hopefully give me the opportunity to be more adaptable to change should I choose a different data storage method, like Azure for example.

Now, I have an implementation for each of these interfaces, here's a simplified example of each one:

public class DataDeleterSQLite : IDataDelete
{
    SQLiteConnection _Connection;

    public DataDeleterSQLite(SQLiteConnection connection) { ... }

    public void Delete<TModel>(TModel model) { ... }
}

... 

public class DataReaderSQLite : IDataRead
{
    SQLiteConnection _Connection;

    public DataReaderSQLite(SQLiteConnection connection) { ... }

    public TModel Get<TModel>(int id) { ... }
}

// You get the idea.

Now, I'm having a problem bringing it all together, I'm certain the general idea is to create a Database class which uses interfaces as opposed to the classes (the real implementation). So, I came up with something like this:

public class Database
{
    IDataDelete _Deleter;
    ...

    //Injecting the interfaces to make use of Dependency Injection.
    public Database(IDataRead reader, IDataWrite writer, IDataDelete deleter) { ... }
}

The question here is how should I expose the IDataRead, IDataWrite, and IDataDelete interfaces to the client? Should I rewrite the methods to redirect to the interface? Like this:

//This feels like I'm just repeating a load of work.
public void Delete<TModel>(TModel model)
{
    _Deleter.Delete<TModel>(model);
}

Highlighting my comment, this looks a bit stupid, I went to a lot of trouble to separate the classes into nice, separated implementations and now I'm bring it all back together in one mega-class.

I could expose the interfaces as properties, like this:

public IDataDelete Deleter { get; private set; }

This feels a little bit better, however, the client shouldn't be expected to have to go through the hassle of deciding which interface they need to use.

Am I completely missing the point here? Help!

Bakki

I think DataReaderSQLite should implement IDataRead not IDataDelete. – Unschooled

@BigDaddy My bad. Good spot. – Bakki

IMHO you are bringing the Single Responsability Principle to an absurde extreme. The responsability of the database class should be to perform CRUD operation, no need to split it down more. – Weathers

I don't see why you feel you need the Database composite. Read, Write and Delete are different actions and few, if any, of your business objects will depend on all 3. – Vulva

I also want to know if the last approach is better than a bulk interface... – Liger

Answer 1

When we talk about interface segregation, (and even for single responsibility) we talk about making entities that do a set of operations which are logically related and fit together to form a meaningful complete entity.

The idea is, a class should be able to read an entity from database, and update it with new values. But, a class should not be able to get weather of Rome and update a stock value in NYSE!

Making separate interfaces for Read, Write, Delete is bit extreme. ISP doesn't literally impose a rule to put just one operation in an interface. Ideally, an interface which can read, write, delete makes a complete (but not bulky with not-related operations) interface. Here, the operations in an interface should be related not dependent on each other.

So, conventionally, you can have an interface like

interface IRepository<T>
{
    IEnumerable<T> Read();
    T Read(int id);
    IEnumerable<T> Query(Func<T, bool> predicate);
    bool Save(T data);
    bool Delete(T data);
    bool Delete(int id);
}

This interface you can pass on to client code, which makes complete sense to them. And it can work with any type of entity which follows a basic set of rules (e.g. each entity should be uniquely identified by an integer id).

Also, if your Business/Application layer classes depend just on this interface, rather than the actual implementating class, like this

class EmployeeService
{
    readonly IRepository<Employee> _employeeRepo;

    Employee GetEmployeeById(int id)
    {
        return _employeeRepo.Read(id);
    }

    //other CRUD operation on employee
}

Then you business/application classes become completely independent of the data store infrastructure. You get the flexibility to choose whichever data store you like, and just plug them into the codebase with an implementation of this interface.

You can have OracleRepository : IRepository and/or MongoRepository : IRepository and inject the correct one through IoC as and when required.

Warrigal

You do understand that IRepository<T> is a ISP violation, unless every consumer of that interface uses all of its members (which is very unlikely)? And do you understand that EmployeeService violates SRP by implementing "other CRUD operation on employee"? I'm missing this information from your answer. – Present

@Present "which is very unlikely" in what circumstances? I'm assuming every repository will have these minimal set of operations as laid out by IRepository<T>, to be fully functional. Now that is something I'll need in my work. Someone might think Query() is not needed for him. In some project Delete() might not be required at all! So it's one "example" of repository, not "the universal standard" for repository. I'm not saying include all these methods in interface even if they do not need it. – Warrigal

On EmployeeService violating SRP, SRP is a essential pattern to be followed to make modular, readable, maintainable classes. I hope you'll agree that making the class with just a single GetEmployee() method will be overdoing on SRP? Again, the expectation here is this class will just work on reading/writing Employee objects without any other operations. I agree that EmployeeService might not be the right name. – Warrigal

So every class in your system that consumes this IRepository<T> interface uses all of its methods (all six in your example)? I've actually never seen this happen in any system, and I bet that this doesn't happen in your systems. Some classes will only use Delete, while others will only call Read. As you said, "Delete() might not be required at all". This is in essence an ISP violation, since it states that "no client should be forced to depend on methods it does not use.". It might not always be a problem to violate ISP, but it is important to understand. – Present

And about Employee violating SRP, I completely disagree with you that this is overdoing SRP. Giving each use case each own class has many very clear examples. The comment section is too short for this, but I urge you to read this article to understand why merging all these operations together is a problem (and you might want to read this article first). – Present

@Present First of all, though the heading only mentioned ISP & SRP, the op actually talked about how he can change the database later. So, the example is actually little un-practical to show a basic repository pattern and using IoC. Agreed, this is not mentioned in the answer. If we use something like EF/NHibernate, we can have a single instance of concrete Repository class, and use it across for any/all types. Now, we can definitely follow ISP more strictly, in that case we'll have something similar to what op had posted originally. – Warrigal

@Steven, is it documented that ISP mandates 100% method usage by every consumer? I agree that ISP seems to imply this; but if the principle can be rephrased so simply, I would almost expect Bob Martin himself to have said so. – Permanency

@Permanency None of the SOLID principles state that you must comply for 100%. If you read Martin's book Agile Principles, Patterns and Practices you'll see that he clearly warns about overdoing it. – Present

Answer 2

Am I completely missing the point here? Help!

I don't think you are completely missing it, you're on the right track, but taking it way too far in this case. All your CRUD functions are totally related to each other so they belong in a single interface that exposes a single responsibility. If your interface exposed CRUD functions AND some other responsibility, then it would be a good candidate to refactor into separate interfaces, in my opinion.

If, as a consumer of your functionality, I had to instantiate different classes for inserts, deletes, etc., I would come looking for you.

Unschooled

Answer 3

Going by this example, the power of breaking up each type of operation would be if you wanted to define capabilities of an object based on combinations of the interfaces.

So you could have something that only Gets, and something else that Gets, Saves, and Deletes, and something else that only Saves. You could then pass them into objects whose methods or constructors only call out ISaves, or whatever. That way they aren't worried about knowing how something saves, just that it saves, which is invoked via a Save() method that is exposed by the interface.

Alternatively, you could have a scenario in which the Database implements all of the interfaces, but then it is passed to objects that only care about triggering a write, or read, or update, etc.--and so when it is passed to that object it is passed as the appropriate interface type and the ability to perform other actions is not exposed to the consumer.

With that in mind, it could be very likely that your application does not have a need for that type of functionality. You may not be drawing on data from disparate sources and have a need to abstract one common method of invoking CRUD operations across them, which is what the first would solve, or a need to decouple the concept of the database as a data source, as opposed to an object that supports CRUD ops, which is what the second would solve. So please do ensure this is being employed to meet a need, and not in an attempt to follow best practices--because this is just one way to employ a certain practice, but whether it is "best" can only be determined within the context of the problem it is solving.

Solidarity

Answer 4

Not really an answer, but I wanted to put more here than a comment would allow. It feels like you are using the repository pattern, so you could wrap it all up with an IRepository.

interface IRepository
{
    T Get<TModel>(int id);
    T Save<TModel>(TModel model);
    void Delete<TModel>(TModel model);
    void Delete<TModel>(int id);
}

Now you can have a concrete Database like you have above:

class Database : IRepository
{
    private readonly IDataReader _reader;
    private readonly IDataWriter _writer;
    private readonly IDataDeleter _deleter;

    public Database(IDataReader reader, IDataWriter writer, IDataDeleter deleter)
    {
        _reader = reader;
        _writer = writer;
        _deleter = deleter;
    }

    public T Get<TModel>(int id) { _reader.Get<TModel>(id); }

    public T Save<TModel>(TModel model) { _writer.Save<TModel>(model); }

    public void Delete<TModel>(TModel model) { _deleter.Delete<TModel>(model); }

    public void Delete<TModel>(int id) { _deleter.Delete<TModel>(id); }
}

Yes, on the surface it looks like an unnecessary abstraction, but there are many benefits. As @moarboilerplate said is his answer, don't let "best" practices get in the way of delivering a product. Your product dictates which principles you need to follow and abstraction level required by the product.

Here is one quick benefit of proceeding with this approach above:

class CompositeWriter : IDataWriter
{
    public List<IDataWriter> Writers { get; set; }

    public void Save<TModel>(model)
    {
        this.Writers.ForEach(writer =>
        {
            writer.Save<TModel>(model);
        });
    }
}

class Database : IRepository
{
    private readonly IDataReader _reader;
    private readonly IDataWriter _writer;
    private readonly IDataDeleter _deleter;
    private readonly ILogger _logger;

    public Database(IDataReader reader, IDataWriter writer, IDataDeleter deleter, ILogger _logger)
    {
        _reader = reader;
        _writer = writer;
        _deleter = deleter;
        _logger = logger;
    }

    public T Get<TModel>(int id)
    {
        var sw = Stopwatch.StartNew();

        _writer.Get<TModel>(id);

        sw.Stop();

        _logger.Info("Get Time: " + sw. ElapsedMilliseconds);
    }

    public T Save<TModel>(TModel model)
    {
         //this will execute the Save method for every writer in the CompositeWriter
         _writer.Save<TModel>(model);
    }

    ... other methods omitted
}

Now you can have places to augment functionality. The above example shows how you could use different IDataReaders and time them without having to add logging and timing into every IDataReader. This also shows how you could have a composite IDataWriter that could actually store data into multiple stores.

So, yes, the abstraction does come with some plumbing and it may feel as though it's not needed, but depending on the life span of your project, this can save you an immense about of technical debt in the future.

Seminal

I like it. CompositeWriter is basically the handler in the Command Handler Pattern. This lets you adhere to Open/Closed Principle and avoid tech debt as you mentioned. – Faradism

Answer 5

The question here is how should I expose the IDataRead, IDataWrite, and IDataDelete interfaces to the client?

If you create these interfaces therefore you already exposed them to the client. Client can use it as dependencies that are injected to consuming classes using Dependency Injection.

I went to a lot of trouble to separate the classes into nice, separated implementations and now I'm bring it all back together in one mega-class.

ISP is about separating interfaces not implementation. In your cause you can even implement these interfaces in one class because therefore you achieve high cohesion in your implementation. Client is even not aware that you make implementation of these interfaces in one class.

public class Database : IDataRead, IDataWrite, IDataDelete
{
}

This might look something similar to following :

public interface IRepository : IDataRead, IDataWrite, IDataDelete
{
}

But, you shouldn't do that because you lose advantages of adhering to ISP. You separated interfaces and created another interface that aggregates other. So, every client that use IRepository interface is still forced to implement all interfaces. This is sometimes called interface soup anti-pattern.

however, the client shouldn't be expected to have to go through the hassle of deciding which interface they need to use.

Actually, I think you miss point here. Client must know what he wants to do , and what ISP tell us it that client should not be forced to use methods that he doesn't need.

---

In example that you showed, when you follow ISP is easy to create assymetric data access. This is familiar concept in CQRS architecture. Imagine that you want separate your reads from writes. And to achieve that actually you doesn't need to modify your existing code (thanks to that you adhere also to OCP). What you need to do is to provide new implementation of IDataRead interface and register this implementation in your Dependency Injection container

Felting

Does it mean exposing via properties is better than implementing a bulk interface? – Liger

Answer 6

When I am designing repositories, I am always thinking in the terms of reading and writing.

This means I am currently using these interfaces:

/// <summary>
/// Inform an underlying data store to return a set of read-only entity instances.
/// </summary>
/// <typeparam name="TEntity">The entity type to return read-only entity instances of.</typeparam>
public interface IEntityReader<out TEntity> where TEntity : Entity
{
    /// <summary>
    /// Inform an underlying data store to return a set of read-only entity instances.
    /// </summary>
    /// <returns>IQueryable for set of read-only TEntity instances from an underlying data store.</returns>
    IQueryable<TEntity> Query();
}

/// <summary>
/// Informs an underlying  data store to accept sets of writeable entity instances.
/// </summary>
/// <typeparam name="TEntity"></typeparam>
public interface IEntityWriter<in TEntity> where TEntity : Entity
{
    /// <summary>
    /// Inform an underlying data store to return a single writable entity instance.
    /// </summary>
    /// <param name="primaryKey">Primary key value of the entity instance that the underlying data store should return.</param>
    /// <returns>A single writable entity instance whose primary key matches the argument value(, if one exists in the underlying data store. Otherwise, null.</returns>
    TEntity Get(object primaryKey);

    /// <summary>
    /// Inform the underlying  data store that a new entity instance should be added to a set of entity instances.
    /// </summary>
    /// <param name="entity">Entity instance that should be added to the TEntity set by the underlying data store.</param>
    void Create(TEntity entity);

    /// <summary>
    /// Inform the underlying data store that an existing entity instance should be permanently removed from its set of entity instances.
    /// </summary>
    /// <param name="entity">Entity instance that should be permanently removed from the TEntity set by the underlying data store.</param>
    void Delete(TEntity entity);

    /// <summary>
    /// Inform the underlying data store that an existing entity instance's data state may have changed.
    /// </summary>
    /// <param name="entity">Entity instance whose data state may be different from that of the underlying data store.</param>
    void Update(TEntity entity);
}

/// <summary>
/// Synchronizes data state changes with an underlying data store.
/// </summary>
public interface IUnitOfWork
{
    /// <summary>
    /// Saves changes tot the underlying data store
    /// </summary>
    void SaveChanges();
}

Some might say that the IEntityWriter is a bit overkill and might violate SRP, since it both can create and delete entities and that the IReadEntities is a leaky abstraction since no one can fully implement IQueryable<TEntity> - but still haven't found the perfect way.

For Entity Framework I then implement all these interfaces:

internal sealed class EntityFrameworkRepository<TEntity> : 
    IEntityReader<TEntity>, 
    IEntityWriter<TEntity>, 
    IUnitOfWork where TEntity : Entity
{
    private readonly Func<DbContext> _contextProvider;

    public EntityFrameworkRepository(Func<DbContext> contextProvider)
    {
        _contextProvider = contextProvider;
    }

    public void Create(TEntity entity)
    {
        var context = _contextProvider();
        if (context.Entry(entity).State == EntityState.Detached)
        {
            context.Set<TEntity>().Add(entity);
        }
    }

    public void Delete(TEntity entity)
    {
        var context = _contextProvider();
        if (context.Entry(entity).State != EntityState.Deleted)
        {
            context.Set<TEntity>().Remove(entity);
        }  
    }

    public void Update(TEntity entity)
    {
        var entry = _contextProvider().Entry(entity);
        entry.State = EntityState.Modified;
    }

    public IQueryable<TEntity> Query()
    {
        return _contextProvider().Set<TEntity>().AsNoTracking();
    }

    public TEntity Get(object primaryKey)
    {
        return _contextProvider().Set<TEntity>().Find(primaryKey);
    }

    public void SaveChanges()
    {
        _contextProvider().SaveChanges();
    }
}

And then I depend in my command handlers on IWriteEntities<MyEntity> and for the query handlers on IReadEntities<MyEntity>. The saving of the entities (using the IUnitOfWork) is done though a decorator pattern using IoC.

Socialize

In your IEntityWriter interface you could instead of separate methods Create and Update make one Save method. – Felting