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&lt;TEntity, in TKey&gt;
{
    TEntity Insert(TEntity entity);

    TEntity Find(Expression&lt;Func&lt;TEntity, bool&gt;&gt; predicate);

    IQueryable&lt;TEntity&gt; FindAll(Expression&lt;Func&lt;TEntity, bool&gt;&gt; predicate);

    TEntity GetById(TKey key);

    TEntity Update(TEntity entity);

    IQueryable&lt;TEntity&gt; 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.

/// &lt;summary&gt;
/// The invoices repository.
/// &lt;/summary&gt;
public class InvoicesRepository : IRepository&lt;Invoice, int&gt;
{
    private readonly string connectionString;

    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&lt;Func&lt;Invoice, bool&gt;&gt; predicate)
    {
        throw new NotImplementedException();
    }


    public IQueryable&lt;Invoice&gt; FindAll(Expression&lt;Func&lt;Invoice, bool&gt;&gt; predicate)
    {
        throw new NotImplementedException();
    }

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

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

    public IQueryable&lt;Invoice&gt; GetAll()
    {
        throw new NotImplementedException();
    }

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

    public Invoice GetById(int key)
    {
        throw new NotImplementedException();
    }
}
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&lt;TEntity&gt;
{
    TEntity Find(Expression&lt;Func&lt;TEntity, bool&gt;&gt; predicate);
    IQueryable&lt;TEntity&gt; FindAll(Expression&lt;Func&lt;TEntity, bool&gt;&gt; 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&lt;TEntity&gt; 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&lt;TEntity&gt; 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&lt;TEntity&gt;.



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

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&lt;TEntity&gt; at this point. An ApiController would use an IInsertable&lt;Invoice&gt; to insert an invoice in a method, and an IDeletable&lt;Invoice&gt; to delete that invoice in another method - and both methods would actually be dealing with an InvoiceRepository.