2
\$\begingroup\$

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>
{
    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<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();
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ This would be a better CR post if you included a few implementations for that interface, IMO. \$\endgroup\$ – Mathieu Guindon Jan 16 '15 at 3:14
  • \$\begingroup\$ @Mat'sMug Added simple example to show Insert method. Let me know if you would like to see more. \$\endgroup\$ – Issa Fram Jan 16 '15 at 3:31
  • 1
    \$\begingroup\$ 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? \$\endgroup\$ – Mathieu Guindon Jan 16 '15 at 3:43
  • \$\begingroup\$ @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. \$\endgroup\$ – Issa Fram Jan 16 '15 at 4:01
  • \$\begingroup\$ 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. \$\endgroup\$ – Steve Michael Jan 16 '15 at 12:22
2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ 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? \$\endgroup\$ – Issa Fram Jan 16 '15 at 4:57
  • \$\begingroup\$ 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. \$\endgroup\$ – Issa Fram Jan 16 '15 at 5:03
  • \$\begingroup\$ 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 :) \$\endgroup\$ – Mathieu Guindon Jan 16 '15 at 5:04

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.