5
\$\begingroup\$

I was developing a web app using Entity Framework 6 and MVC 5. For the data access layer, I eased the job and wrote a generic repository as following:

public class GenericRepository<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;
    }
}

In controllers, I declare it as a global variable and use it in different action methods.

There is one big problem with this approach:

Flexibility: I'm not able to do anything specific with this repository, like preserving some fields, etc. I'm forced to use some hacks to achieve things like that.

I know that many believe using a repository pattern with context is not useful. The main reason that I tried that was to make my controllers cleaner and a generic one to save time. But I guess it doesn't worse it. I think next time I'll just use the EF context without any additional pattern.

What do you suggest, using this library, using this with Unit of Work, or something else? Is there any other problem with this code?

\$\endgroup\$
3
\$\begingroup\$

Actually your approach for me is a premature generalization. In your case (and the most of the cases, except maybe in enterprise apps) the dbcontext IS the repository. You don't need another one layer that will only add complexity. When the need for a wrapper to the dbcontext arise, you will see it, and you will implement the extra repository that it will fill your exact needs.

Flexibility: I'm not able to do anything specific with this repository, like preserving some fields, etc. I'm forced to use some hacks to achieve things like that.

The Repository SHOULD be specific and Not generic to an application

\$\endgroup\$
3
\$\begingroup\$

One major drawback...

public void Insert(T entity)
{
    context.Set<T>().Add(entity);
    context.SaveChanges();
}

...is that you've just destroyed the usefulness of a unit of work. DbContext wraps a database transaction, and what's great about transactions, is that they allow you to commit a "set" of changes all at once.

But you're calling SaveChanges every time anything happens to any entity, which means if you have 1 request doing 20 calls into that repository, you're effectively committing 20 transactions.

A new unit of work instance should be created, and die with each request: SaveChanges should be called, ideally, exactly once in the lifetime of the DbContext.

\$\endgroup\$
2
\$\begingroup\$

IQueryable

I would suggest returning IQueryable<T> instead of IEnumerable<T>. Why?

IEnumerable execute select query on server side, load data in-memory on client side and then filter data. Hence does more work and becomes slow.

While querying data from database, IQueryable execute select query on server side with all filters. Hence does less work and becomes fast.

Additionally there are more reasons listed here.

Lambdas for Flexibility

Instead of limiting yourself to GetByID(object id) why not GetByID(Func<T, Boolean> predicate)? It may add a tiny extra amount of code when using, but it allows for more flexibility in the case that you ever need to filter on another field other than your ID. Here is an example from my EfBasedRepository<T, TKey> where T : IEntity<TKey> class

    /// <summary>
    /// Reads all objects in which all of the predicates return true.
    /// </summary>
    public virtual IQueryable<T> ReadAll(params Expression<Func<T, Boolean>>[] predicates)
    {
        IQueryable<T> results = db.Set<T>();
        if (predicates == null || predicates.Length <= 0) { return results; }

        foreach (var predicate in predicates)
        {
            results = results.Where(predicate);
        }

        return results;
    }

This allows improved readability, flexibility, and performance. Readability from chaining (not having to do a bunch of && for multiple filters at minimal performance loss), Flexibility, you're still open to extend the query as it is returned as an IQueryable and didn't call .ToList() before returning. (And performance for the same reason mentioned in the IQueryable section).

Summary

  • At the very worst it is equal in performance while gaining some readability and flexibility. At the very best it gains performance, readability, and flexibility.
  • The one major downside is that it is heavily reliant with Lambdas which for EF and LINQ2SQL is great but if you were to (for some reason) want the flexibility to swap out the back-ends (Say for example switch from EF to Raw SQL Queries) it could prove difficult.

Other Notes

  • You might want to consider similar naming style to EF (completely optional). So instead of GetByID you could use Find or even Read.
  • Make use of virtual so that if you have a special-case collection/table that needs some special adjustment you can just override it.
\$\endgroup\$
  • \$\begingroup\$ Thanks for your useful notes, I still have another question: Does it make any difference that I'm using a general variable for the repository, against using it locally that will be terminated after each action? \$\endgroup\$ – Akbari Jul 25 '15 at 2:33
  • 1
    \$\begingroup\$ using a DbContext on a local-scope (i.e. declaring it in each function) will 1) Add a minor amount of extra code and 2) Add a bit of overhead for declaring the context, which I remember reading that the overhead is negligible. That being said I use it as you are, except I'd suggest having your repository class implement IDisposable and add something like if (context != null) { context.Dispose(); } to it to ensure it was properly disposed of. \$\endgroup\$ – Shelby115 Jul 25 '15 at 3:05
1
\$\begingroup\$

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.

\$\endgroup\$

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.