Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Thought 1

public interface IRepository<T> : IDisposable where T : class
        {
            IQueryable<T> Fetch();        
            IEnumerable<T> GetAll();        
            IEnumerable<T> Find(Func<T, bool> predicate);        
            T Single(Func<T, bool> predicate);        
            T First(Func<T, bool> predicate);        
            void Add(T entity);        
            void Delete(T entity);        
            void SaveChanges();        
        }

with the above approach I will go like Repository and then have a Repository with the implementation of methods written above.

I think this frees my tests from DbContext dependency.

Thought 2

 public class RepositoryBase<TContext> : IDisposable where TContext : DbContext, new()
    {
            private TContext _DataContext;

            protected virtual TContext DataContext
            {
                get
                {
                    if (_DataContext == null)
                    {
                        _DataContext = new TContext();
                    }
                    return _DataContext;
                }
            }

            public virtual IQueryable<T> GetAll<T>() where T : class
            {                   
                using (DataContext)
                    {
                        return DataContext.Set<T>();
                    }                   
            }

            public virtual T FindSingleBy<T>(Expression<Func<T, bool>> predicate) where T : class
            {
                if (predicate != null)
                {
                    using (DataContext)
                    {
                        return DataContext.Set<T>().Where(predicate).SingleOrDefault();
                    }
                }
                else
                {
                    throw new ArgumentNullException("Predicate value must be passed to FindSingleBy<T>.");
                }
            }

            public virtual IQueryable<T> FindAllBy<T>(Expression<Func<T, bool>> predicate) where T : class 
            {
                if (predicate != null)
                {
                    using (DataContext)
                    {
                        return DataContext.Set<T>().Where(predicate);
                    }
                }
                else
                {
                    throw new ArgumentNullException("Predicate value must be passed to FindAllBy<T>.");
                }
            }

            public virtual IQueryable<T> FindBy<T, TKey>(Expression<Func<T, bool>> predicate,Expression<Func<T, TKey>> orderBy) where T : class 
            {
                if (predicate != null)
                    {
                        if (orderBy != null)
                        {
                            using (DataContext)
                            {
                                return FindAllBy<T>(predicate).OrderBy(orderBy).AsQueryable<T>(); ;
                            }
                        }
                        else
                        {
                            throw new ArgumentNullException("OrderBy value must be passed to FindBy<T,TKey>.");
                        }
                    }
                else
                    {
                            throw new ArgumentNullException("Predicate value must be passed to FindBy<T,TKey>.");
                    }
            }

            public virtual int Save<T>(T Entity) where T : class
            {
                return DataContext.SaveChanges();
            }    
            public virtual int Update<T>(T Entity) where T : class
            {
                return DataContext.SaveChanges();
            }    
            public virtual int Delete<T>(T entity) where T : class
            {
                DataContext.Set<T>().Remove(entity);
                return DataContext.SaveChanges();
            }        
            public void Dispose()
            {
                if (DataContext != null) DataContext.Dispose();
            }
    }

I could now have CustomerRepository : RepositoryBase<Mycontext>,ICustomerRepository ICustomerRepository gives me the option of defining stuff like GetCustomerWithOrders etc...

I am confused as to which approach to take. Would need help through a review on the implementation first then comments on which one will be the easier approach when it comes to Testing and extensibility.

Edit Revised after feedback.

public interface IRepositoryBase
    {
        int Delete<T>(T entity) where T : class;
        void Dispose();
        IQueryable<T> FindAllBy<T>(Expression<Func<T, bool>> predicate) where T : class;
        T FindSingleBy<T>(Expression<Func<T, bool>> predicate) where T : class;
        IQueryable<T> GetAll<T>() where T : class;
        IQueryable<T> GetIncluding<T>(params Expression<Func<T, object>>[] includeProperties) where T:class;
        int Save<T>(T Entity) where T : class;
        int Update<T>(T Entity) where T : class;
    }

public class RepositoryBase<TContext> : IDisposable, IRepositoryBase where TContext : DbContext,new()
{
        private TContext _DataContext;
        protected virtual TContext DataContext
        {
            get
            {
                if (_DataContext == null)
                {
                    _DataContext = new TContext();
                }
                return _DataContext;
            }
        }

        public virtual IQueryable<T> GetAll<T>() where T : class
        {                   
            using (DataContext)
                {
                    return DataContext.Set<T>();
                }                   
        }

        public virtual T FindSingleBy<T>(Expression<Func<T, bool>> predicate) where T : class
        {
            if (predicate != null)
            {
                using (DataContext)
                {
                    return DataContext.Set<T>().Where(predicate).SingleOrDefault();
                }
            }
            throw new ArgumentNullException("Predicate value must be passed to FindSingleBy<T>.");
        }

        public virtual IQueryable<T> FindAllBy<T>(Expression<Func<T, bool>> predicate) where T : class 
        {
            if (predicate != null)
            {
                using (DataContext)
                {
                    return DataContext.Set<T>().Where(predicate).AsQueryable<T>(); ;
                }
            }
           throw new ArgumentNullException("Predicate value must be passed to FindBy<T,TKey>.");

        }

        public IQueryable<T> GetIncluding<T>(params Expression<Func<T, object>>[] includeProperties) where T: class
        {
            IQueryable<T> query = _DataContext.Set<T>();
            foreach (var includeProperty in includeProperties)
            {
                query = query.Include(includeProperty);
            }
            return query;
        }

        public virtual int Save<T>(T Entity) where T : class
        {
            return DataContext.SaveChanges();
        }

        public virtual int Update<T>(T Entity) where T : class
        {
            return DataContext.SaveChanges();
        }

        public virtual int Delete<T>(T entity) where T : class
        {
            DataContext.Set<T>().Remove(entity);
            return DataContext.SaveChanges();
        }

        public void Dispose()
        {
            if (DataContext != null) DataContext.Dispose();
        }
    }

Edit 2 :

I was writing tests against this using MOQ and NUnit. It fast turned into a nightmare of sorts. I went through the pain of my tests becoming dependent on EF. I do not think that is a good idea.Anyone have any ideas how to unit test RepositoryBase or as I see it I need to refactor and have a UnitOFWork. This has been a serious examination of my skills.

Edit 3 :

https://gist.github.com/4175430 I have put my new code there just the outline not the details inside . Now I am struggling with the ability to test this. I know the integration tests with a live db will give me the idea about Linq2Entities since testing this using Linq2Objects doesn't really make any sense.

With the answers mentioned below it seems like I need to have a service layer and my tests will have to be against this layer and not the repository since I should trust Microsoft with testing EF in the first place.

share|improve this question
2  
Why not have both and have your RepositoryBase implement the IRepository interface? –  dreza Nov 26 '12 at 21:32
    
@dreza then, should the IRepository have methods like add , save , delete or should it have the find , get , fetch methods ? I guess both are addressing different concerns one set of methods is query and other is transactional logic, and the subsequent question is what are the benefits/drawbacks of making the RepositoryBase implement the IRepository interface ? –  ashutosh raina Nov 27 '12 at 5:42
    
I would make the repository have methods like SingleOrDefault(), GetAll(), FirstOrDefault() rather than having to do .Fetch().SingleOrDefault() etc –  dreza Nov 28 '12 at 1:02
    
About edit 2: Tests of the RepositoryBase has to be "integrated" since DbContext isn't abstract or have any good interface, but a nice trick is to use Database.SetInitializer(new DropCreateDatabaseAlways<YourContext>()) in those tests. Which means you always have a fresh DB for your integrated tests. It's also useful to move the concrete Repository and UnitOfWork classes and tests into separate assemblies, so you don't have those dependencies in your business logic tests and classes. Otherwise you could always go buy TypeMock which can mock more or less anything. –  Lars-Erik Nov 28 '12 at 14:21
    
@Lars-Erik So RepositoryBase will be tested through an Integration test with a database. I should be able to however test my business logic through unit tests. My POCO classes are in a separate assembly and the repository is in a separate assembly. My unit tests project just has a reference to the Repository. Is that the correct structure ? –  ashutosh raina Nov 28 '12 at 14:46

2 Answers 2

up vote 16 down vote accepted

You should use Expression<Func<T, bool>> as predicates on your interface and have the RepositoryBase implement that interface. By only using Func, you won't get translation to L2E etc, but will have to enumerate the entire DB table before you can evaluate the Func. The interface can be mocked, hence unit-tested without a physical db and also used with other ORMs.

It's generally prefered to keep SaveChanges and the DbContext in a separate IUnitOfWork implementation. You can pass the UnitOfWork to the repository constructor, which can access an internal property on the UOW exposing the DbContext.

By doing that you can share the same DbContext between repositories, and batch updates to several entity roots of different type. You also keep fewer open connections, which are the most expensive resource you have when it comes to DBs.

Not to forget they can share transactions. :)

Hence you should not dispose the DbContext in the Repository, the Repository really doesn't need to be disposable at all. But the UnitOfWork / DbContext must be disposed by something.

Also, ditch the OrderBy predicate to FindBy. Since you return an IQueryable, and use an Expression for the predicate, you can keep building the Queryable after the call. For instance repo.FindBy(it => it.Something == something).OrderBy(it => it.OrderProperty). It will still be translated to "select [fields] from [table] where [predicate] order by [orderprop]" when enumerated.

Otherwise it looks good.

Here's a couple of good examples:

http://blog.swink.com.au/index.php/c-sharp/generic-repository-for-entity-framework-4-3-dbcontext-with-code-first/

http://www.martinwilley.com/net/code/data/genericrepository.html

http://www.mattdurrant.com/ef-code-first-with-the-repository-and-unit-of-work-patterns/

And here's how I do it:

Model / Common / Business assembly

No references but BCL and other possible models (interfaces/dtos)

public interface IUnitOfWork : IDisposable
{
    void Commit();
}

public interface IRepository<T>
{
    void Add(T item);
    void Remove(T item);
    IQueryable<T> Query();
}

// entity classes

Business tests assembly

Only references Model / Common / Business assembly

[TestFixture]
public class BusinessTests
{
    private IRepository<Entity> repo;
    private ConcreteService service;

    [SetUp]
    public void SetUp()
    {
        repo = MockRepository.GenerateStub<IRepository<Entity>>();
        service = new ConcreteService(repo);
    }

    [Test]
    public void Service_DoSomething_DoesSomething()
    {
        var expectedName = "after";
        var entity = new Entity { Name = "before" };
        var list = new List<Entity> { entity };
        repo.Stub(r => r.Query()).Return(list.AsQueryable());
        service.DoStuff();
        Assert.AreEqual(expectedName, entity.Name);
    }

}

Entity Framework implementation assembly

References Model and the Entity Framework / System.Data.Entity assemblies

public class EFUnitOfWork : IUnitOfWork
{
    private readonly DbContext context;

    public EFUnitOfWork(DbContext context)
    {
        this.context = context;
    }

    internal DbSet<T> GetDbSet<T>()
        where T : class
    {
        return context.Set<T>();
    }

    public void Commit()
    {
        context.SaveChanges();
    }

    public void Dispose()
    {
        context.Dispose();
    }
}

public class EFRepository<T> : IRepository<T>
        where T : class
{
    private readonly DbSet<T> dbSet;

    public EFRepository(IUnitOfWork unitOfWork)
    {
        var efUnitOfWork = unitOfWork as EFUnitOfWork;
        if (efUnitOfWork == null) throw new Exception("Must be EFUnitOfWork"); // TODO: Typed exception
        dbSet = efUnitOfWork.GetDbSet<T>();
    }

    public void Add(T item)
    {
        dbSet.Add(item);
    }

    public void Remove(T item)
    {
        dbSet.Remove(item);
    }

    public IQueryable<T> Query()
    {
        return dbSet;
    }
}

Integrated tests assembly

References everything

[TestFixture]
[Category("Integrated")]
public class IntegratedTest
{
    private EFUnitOfWork uow;
    private EFRepository<Entity> repo;

    [SetUp]
    public void SetUp()
    {
        Database.SetInitializer(new DropCreateDatabaseAlways<YourContext>());
        uow = new EFUnitOfWork(new YourContext());
        repo = new EFRepository<Entity>(uow);
    }

    [TearDown]
    public void TearDown()
    {
        uow.Dispose();
    }

    [Test]
    public void Repository_Add_AddsItem()
    {
        var expected = new Entity { Name = "Test" };
        repo.Add(expected);
        uow.Commit();
        var actual = repo.Query().FirstOrDefault(e => e.Name == "Test");
        Assert.IsNotNull(actual);
    }

    [Test]
    public void Repository_Remove_RemovesItem()
    {
        var expected = new Entity { Name = "Test" };
        repo.Add(expected);
        uow.Commit();
        repo.Remove(expected);
        uow.Commit();
        Assert.AreEqual(0, repo.Query().Count());
    }
}
share|improve this answer
    
Expression v/s Func ..nice catch.. the RepositoryBase has the correct predicate. OrderBy also looked useless to me. Will remove that. The link you provide have two different implementations in terms of passing context or passing context and entity type as Type parameters. Which one is the better one or are both the same and just a matter of style. I am somehow unable to get my head around this. –  ashutosh raina Nov 27 '12 at 18:06
    
DbContext is a UoW in itself, so what would be the benefits the having a separate UoW ? –  ashutosh raina Nov 27 '12 at 18:09
    
@ashutoshraina possibly to abstract the UoW itself. People do that thinking they're going to have to change their ORM or data storage eventually. In my experience, it never happens and thus the less you abstract the better. –  maxbeaudoin Nov 27 '12 at 21:24
    
It's generally better to mock an interface which doesn't have dependencies on libraries other than the BCL itself. Even if you won't change your ORM, your tests can run without depending on anything but the BCL. Hence the need to wrap DbContext. Won't be much anyway. To make a point, we did this for EF 1 combined with the EF Poco Adapter project from codeplex. When migrating to EF 4, we had to swap out the old contexts and dbset implementations for the new ones, and benefited from having our own UOW and Repos. –  Lars-Erik Nov 28 '12 at 10:59
    
@Lars-Erik See my Edit 2. –  ashutosh raina Nov 28 '12 at 14:13

Reinventing the wheel

If you need a generic repository class use IObjectSet for base type and implement it if needed (in production code it is not needed, in tests you can use an IList<> as backend).

Leek on the abstraction

Repositories aren't supposed to have Save method becouse if someone else is in a transactional moment in shared context then you (or the other guy) can trick each other. Please create an independent IUnitOfWork for this.

interface IUnitOfWork : IDisposable {
    void Commit();
    void Rollback();
}

interface IDataStore {
    IUnitOfWork InUnitOfWork();
    IObjectSet<T> Set<T>() where T : class;
}

using (var uow = _data.InUnitOfWork()) {
    _data.Set<Order>.AddObject(order);
    _data.Set<SomethingElse>.Remove(otherObject);

   uow.Comit();
}

The tricky part is how you handles the transaction. I have created an ISession interface with some methods (BeginTransaction(), InTransaction, Commit(), Rollback()) and created a default implementation. In my default IUnitOfWork implementation the constructor recieves an ISession instance and starting a transaction with it. The ISession implementation has an inner counter to count how many other actor started it's own transaction. When an IUnitOfWork from the shared storage Commited the counter is decremented by 1 if someone says RollBack then everything is rolled back.

EDIT (Dec. 03)

Fixed the IUnitOfWork Dispose behavior: the default is Rollback() not Commit()

In this way you can test your code easily.

share|improve this answer
1  
IObjectSet is a MS api specific interface, isn't it? You can't reuse it if you swap to say NHibernate or RavenDB if you don't abstract the repository too. –  Lars-Erik Dec 3 '12 at 1:19
    
+1 for the Rollback @Lars-Erik shoulddn't rollback be there if we have the Save in the UnitOfWork ? –  ashutosh raina Dec 3 '12 at 3:43
    
@Lars-Erik: the System.IDisposable is a Microsoft specific thing slong with all the power of the .NET has. The IObjectSet<TEntity> has no specific constraint on TEntity just a simple "be a class". So yes, you can and should reuse it. –  Peter Kiss Dec 3 '12 at 6:39
    
AFAIK, the DbContext doesn't have rollback unless you create a transactionscope instance and roll back on that. I use UOW Dispose to rollback, since I probably don't want to continue the trans anyway. @Peter I guess I should look into IObjectSet, but I still really like to call it IRepository since it's a known and proven pattern name. IObjectSet isn't.. And it's 20 lines of code, so who really cares? –  Lars-Erik Dec 3 '12 at 18:50
    
20 lines of unnecessary code can make a big mess in the code base and why you would write plus code if it's already exists? Another thing that is where is .NET there is an IObjectSet<T>, in any .NET app. –  Peter Kiss Dec 3 '12 at 20:24

protected by syb0rg Mar 1 at 23:48

Thank you for your interest in this question. Because it has attracted low-quality answers, posting an answer now requires 10 reputation on this site.

Would you like to answer one of these unanswered questions instead?

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