# IRepository Pattern - Interface Segregation Principle

I have an IRepository class that I use a lot. But I noticed that for many of my repositories I do not implement most of the methods. Also, I usually don't have a TKey that I want to specify if I'm not using the GetById method but the current design forces me to specify it. I was thinking of changing the interface or splitting up the interface into multiple smaller interfaces in order to make it cleaner and easier to use. Any ideas?

public interface IRepository<TEntity, in TKey>
{
TEntity Insert(TEntity entity);

TEntity Find(Expression<Func<TEntity, bool>> predicate);

IQueryable<TEntity> FindAll(Expression<Func<TEntity, bool>> predicate);

TEntity GetById(TKey key);

TEntity Update(TEntity entity);

IQueryable<TEntity> GetAll();

void Delete(TEntity entity);
}


Here is an example implementation. Let me know if you need to see more than one method. It is somewhat self explanatory.

/// <summary>
/// The invoices repository.
/// </summary>
public class InvoicesRepository : IRepository<Invoice, int>
{

public InvoicesRepository(string connectionString)
{
this.connectionString = connectionString;
}

public Invoice Insert(Invoice entity)
{
using (SqlConnection connection = new SqlConnection(this.connectionString))
using (SqlCommand command = new SqlCommand("INSERT INTO INVOICES VALUES (entity.SomeProp)", connection))
{
connection.Open();
entity.Id = (int)command.ExecuteScalar();
return entity;
}
}

public Invoice Find(Expression<Func<Invoice, bool>> predicate)
{
throw new NotImplementedException();
}

public IQueryable<Invoice> FindAll(Expression<Func<Invoice, bool>> predicate)
{
throw new NotImplementedException();
}

public Invoice GetById(int key)
{
throw new NotImplementedException();
}

public Invoice Update(Invoice entity)
{
throw new NotImplementedException();
}

public IQueryable<Invoice> GetAll()
{
throw new NotImplementedException();
}

public void Delete(Invoice entity)
{
throw new NotImplementedException();
}

public Invoice GetById(int key)
{
throw new NotImplementedException();
}
}

• This would be a better CR post if you included a few implementations for that interface, IMO. – Mathieu Guindon Jan 16 '15 at 3:14
• @Mat'sMug Added simple example to show Insert method. Let me know if you would like to see more. – Issa Fram Jan 16 '15 at 3:31
• This InvoiceRepository is real? Or made-up? The point I'm trying to make is that hypothetical/example code is frowned upon, we like having as much context as possible - don't be afraid to include "too much code", this isn't Stack Overflow where you have to keep it short & sweet - on Code Review if you narrow it down too much, it gets hard to... review ;) For instance, who is the calling code for such a repository? Does that IQueryable ends up in the UI layer? – Mathieu Guindon Jan 16 '15 at 3:43
• @Mat'sMug It is made up. The original question was about interface design. You asked for example code to maybe get a clearer understanding of what the interface was doing and I tried to give that without giving any of my internal repos away (DTOs, tables, etc). But to answer your questions, anything can use the repos. Could be the service/business layer. I have sometimes used these IRepositorys directly in an ApiController constructor and use them as CRUD methods mapping to Get, Put, Post, Delete. – Issa Fram Jan 16 '15 at 4:01
• Personally I am not a fan of this style. I would prefer to have an IInvoicesRepository that you can then use for injection where necessary. That allows you to specify only the methods you need. I don't see a huge benefit in the generic interface over a specific interface for the class. – Steve Michael Jan 16 '15 at 12:22

The ISP is indeed taking a beating with all these NotImplementedException. If we boil this interface a little (ok a lot), we get this:

public interface IRepository<TEntity>
{
TEntity Find(Expression<Func<TEntity, bool>> predicate);
IQueryable<TEntity> FindAll(Expression<Func<TEntity, bool>> predicate);
}


These two should always be implemented, assuming all entity types can at least be selected. I haven't included GetById because it makes things more complicated, and it's really Find all over again (especially when you have an entity with a composite PK).

That said, TEntity Find(...) feels a bit awkward: one could expect a "Find" method to return more than a single result, like FindAll does. I think it's the naming that's off: Find should be Single, or perhaps Select (consistent with the LINQ method that does the same thing), and then FindAll could become Find.

IQueryable<TEntity> shouldn't be permitted to exist beyond the data access layer - there's no reason FindAll can't materialize the query itself and return an IEnumerable<TEntity> instead, making sure that any further LINQ code on the client side doesn't get to the database.

I'd make Find return an IEnumerable<TEntity>.

Now to keep the ISP happy, we'd have an IInsertable<TEntity>, an IUpdatable<TEntity> and an IDeletable<TEntity>, respectively with Insert, Update and Delete methods; a repository that supports updates will implement IUpdatable<TEntity>, and a repository that doesn't support deletions won't implement IDeletable<TEntity>.

I don't think it's ideal, but it strips the repository interface down quite a bit - in fact it could even be renamed ISelectable<TEntity> at this point. An ApiController would use an IInsertable<Invoice> to insert an invoice in a method, and an IDeletable<Invoice> to delete that invoice in another method - and both methods would actually be dealing with an InvoiceRepository.

• The part about not permitting use of methods was unclear to me though. I can't control who uses the interface. But if you mean to not use IQueryable then I think I understand. I have always been on the fence about that. For lazy loading it is great. For anything else I would use IEnumerable. But maybe I should rethink it. Any guidance on that? – Issa Fram Jan 16 '15 at 4:57
• I only ever included TKey because of the possibility of strange / composite keys. Your answer is incredibly interesting. I think it should work with all use cases. I will play around with it. Maybe post some gists. – Issa Fram Jan 16 '15 at 5:03
• The client code has no way of knowing whether the IQueryable will operate against LINQ-to-Objects, or if it needs to bear in mind that it's LINQ-to-SQL or LINQ-to-Entities and that the supplied Expression parameters must account for that. If you expose an IEnumerable instead, the client code can rightfully expect and assume LINQ-to-Objects and a materialized query. It's all just for clarity :) – Mathieu Guindon Jan 16 '15 at 5:04