The DbContext is a pretty solid generic repository itself, so your implementation over it isn't useful at the moment. The only use such a repository would have (I think), is to create an abstraction of the DbContext. You've almost achieved this at the moment. Let me show you what I mean.
public class AnythingThatUsesTheRepository
{
public IRepository<MyClass> Repository { get; set; }
public void DoSomething()
{
Repository.Insert(new MyClass());
}
}
public interface IRepository<T> where T : class
{
IEnumerable<T> GetAll();
T GetByID(object id);
void Insert(T entity);
void Update(T entity);
T Delete(T entity);
}
public class DbContextRepository<T> where T : class
{
private EFDbContext context = new EFDbContext();
public IEnumerable<T> GetAll()
{
return context.Set<T>();
}
public T GetByID(object id)
{
return context.Set<T>().Find(id);
}
public void Insert(T entity)
{
context.Set<T>().Add(entity);
context.SaveChanges();
}
public void Update(T entity)
{
context.Entry(entity).State = System.Data.Entity.EntityState.Modified;
context.SaveChanges();
}
public T Delete(T entity)
{
if (entity != null)
context.Entry(entity).State = System.Data.Entity.EntityState.Deleted;
context.SaveChanges();
return entity;
}
}
Now, the explanation : Having the IRepository<T> interface, you can do dependency injection. (If you don't know about this, you might want to look at it on Google, it's quite interesting/important I think). Now, the client of the repository AnythingThatUsesTheRepository doesn't know it is using a DbContext, you remove a dependancy. Also, if you wanted to test your AnythingThatUsesTheRepository class or change the implementation for a repository that uses NHibernate for example, the client (AnythingThatUsesTheRepository) wouldn't need to change its code. You would just need to set the Repository parameter to another implementation.
From my experience, too much abstraction with Entity Framework makes you unable to use the DbContext to its full potentiel.
Though, if you want to try and make a generic repository with a little more flexibility, I think you could implement a Query method, like @ba2887 said, though I'll explain why you should use it in more details.
The Query method would look like this :
public IQueryable<T> Query()
{
return context.Set<T>().AsQueryable();
}
Going this way, the clients that uses your repository could create a query by themselves, which means you don't have to worry about the quadrillion possible queries (With Where,Select,Join,Include etc..).
The danger with this method, and with your current GetAll() method, is that I, as a client of your repository, don't know if the query was executed or not. The IQueryable<T> interface, as the IEnumerable<T> interface (since the first implements the second), defers the execution of the query. Meaning that each time I call a foreach on my query, I return to the database to do the query again, which is super expensive. I don't know if that's clear, so here's an example that uses the Query() method (Imagine I added the Query() method to the interface):
public class MyClass
{
public string Prop1 { get; set; }
public int Prop2 { get; set; }
}
public void TerribleStuff()
{
IRepository<MyClass> repo = new DbContextRepository();
//At that point, I, as a client, don't know if the query was executed or not
IQueryable<int> query = repo.Query().Where(m => m.Prop1 == "a").Select(m => m.Prop2);
foreach(int prop2s in query)
{
//Do something.. A first database call is made
}
foreach(int props2 in query)
{
//Do something again.. A second database call is made
}
//THAT is bad. Who knows, client might make 100 database calls.
return query;
}
So, by exposing IQueryable<T> without executing the query beforehand, you loose control over how many calls will be made to the database, which is the main problem with Query(). Then again, maybe I'm paranoid and it isn't that much of a big deal.
For your ReadAll() method, to correct the actual problem (if it is one for you), just call .ToList() at the end of your query in order to execute the said query, so it won't make multiple database calls.
return context.Set<T>().ToList();
In short, I think you should let the client use the DbContext and the IQueryable<T> interface that is super powerful. If the client uses the DbContext.Set<T>().AsQueryable() and doesn't execute the query, that's him problem, not yours! I think that abstracting the ORM is complicated and often leads to other problems that will lead to more abstraction that will lead to other problems etc.. And you'll loose flexibility.
