Generic UnitOfWork

Question

I am trying to create a Repository & UnitOfWork for Data Access Layer. In my current implementation I have to modify my UnitOfWork everytime I create a new repository. I would like to avoid that and also keep the functionality to extend my repository abstract class.

Following is my generic Repository & UnitOfWork interface & classes

 public interface IRepositoryBase<T> where T : class
 {
 IList<T> FindAll();
 T FindByCondition(Expression<Func<T, bool>> expression);
 void Create(T entity);
 void Update(T entity);
 void Delete(T entity);
 }
 public abstract class RepositoryBase<T> : IRepositoryBase<T> where T : class
 {
 protected DBContext _dbContext { get; set; }
 public RepositoryBase(DBContext dbContext)
 {
 _dbContext = dbContext;
 }
 //other methods removed
 public void Create(T entity)
 {
 _dbContext.Set<T>().Add(entity);
 }
 }
 public interface IUnitOfWork 
 {
 IReminderRepository Reminder { get; }
 void Save();
 }
 public class UnitOfWork : IUnitOfWork, IDisposable
 {
 private IReminderRepository _reminderRepository;
 public UnitOfWork(DBContext dbContext)
 {
 _dbContext = dbContext;
 }
 public IReminderRepository Reminder
 {
 get
 {
 return _reminderRepository = _reminderRepository ?? new ReminderRepository(_dbContext);
 }
 }
 public void Save()
 {
 _dbContext.SaveChanges();
 }
 public void Dispose()
 {
 _dbContext.Dispose();
 }
 }

Here I can extend my Repository as per my specific needs by implementing the specific Repository as

public interface IReminderRepository : IRepositoryBase<Reminder>
 {
 IList<Reminder> GetAllReminders();
 Reminder GetReminderById(Guid id); 
 Reminder GetReminderByName(string name); 
 void CreateReminder(Reminder reminder); 
 void UpdateReminder(Reminder reminder); 
 void DeleteReminder(Reminder reminder); 
 }
public class ReminderRepository : RepositoryBase<Reminder>, IReminderRepository
{
 public ReminderRepository(DBContext dbContext)
 : base(dbContext)
 {
 _dbContext = dbContext;
 }
 //other methods removed
 public Reminder GetReminderByName(string name)
 {
 return FindAll()
 .OrderByDescending(r => r.Name)
 .FirstOrDefault(r => r.Name == name);
 //return FindByCondition(r => r.Name == name);
 }
}

This is ok but when ever I will create a new Specific Repository I will have to modify the UnitOfWork class as well by adding a new property for the new Repository.

While searching online I found following but it does not work in my case as my RepositoryBase is an abstract class.

 public interface IUnitOfWork : IDisposable
 {
 Dictionary<Type, object> _repositories;
 void Save();
 }
 public class UnitOfWork : IUnitOfWork
 {
 private readonly DBContext _dbContext { get; set; }
 private readonly Dictionary<Type, object> _repositories = new Dictionary<Type, object>();
 public Dictionary<Type, object> Repositories
 {
 get { return _repositories; }
 set { Repositories = value; }
 }
 public UnitOfWork(DBContext dbContext)
 {
 _dbContext = dbContext;
 }
 public IRepositoryBase<T> Repository<T>() where T : class
 {
 if (Repositories.Keys.Contains(typeof(T)))
 {
 return Repositories[typeof(T)] as IRepositoryBase<T>;
 }
 IRepositoryBase<T> repo = new RepositoryBase<T>(_dbContext);//This does not work
 Repositories.Add(typeof(T), repo);
 return repo;
 }
 public void Save()
 {
 _dbContext.SaveChanges();
 }
 }

• Graipher: Please don't edit your code after having received an answer. This violates the question-and-answer style of the site. If you think that your code needs more review, just post a follow-up question, linking back to this one. Have a look at our help center for more information about what you can and cannot do after having received an answer.

Answer 1

Unneeded complexity

There is unnecessary complexity because of two reasons: the unit of work wants to be a repository locator and because the repositories are generic on the wrong side.

Remove the repositories references from the UoW and you will be fine. Let's see now a way to have a better generic way for your CRUD.

public interface IRepositoryBase
{
 IList<T> FindAll<T>();
 T FindByCondition<T>(Expression<Func<T, bool>> expression);
 void Create<T>(T entity);
 void Update<T>(T entity);
 void Delete<T>(T entity);
}

So what did I just do here was to make the interface non-generic and move the generic type to the methods. Imagine now how easy is to do following

 public class RemindersService
 {
 private readonly IRepositoryBase _repository;
 private readonly IUnitOfWork _unitOfWork;
 public RemindersService(IRepositoryBase repository, IUnitOfWork uow) 
 { 
 _repository = repository; 
 _uow = uow
 }
 public Reminder AddNewReminder(DateTime moment, string note, Guid receiver, string name)
 {
 var user = _repository.FindById(receiver);
 var reminder = new Reminder {ReceiverId = receiver, Moment = momoent, Note = note };
 _repository.Create(reminder);
 _uow.Save();
 }
 }

The main gain here is for very simplistic CRUD operations you don't need to create an interface and an implementation for each of entity in the DbContext.

The downside of this approach is that now suddenly all the entities will have this generic way for the CRUD operations. Maybe you don't what that, but I consider this a non-issue in most of the applications. One potential issue is when you want to implement here and there let's say a soft-delete and the generic method does a hard delete. There are ways to overcome that.

Use the valuable API from DbContext

I used in the RemindersService.AddNewReminder a new method, FindById, that was not in the original IRepositoryBase definition. This can be easily added and dispatched to dbContext.Find(id). This method returns so you'll probably have to check for that in your Application Services.

Simplify abstractions, remove duplication

Let's move next to the more specific repository. With the non-generic IRepositoryBase it will look like this:

public interface IReminderRepository : IRepositoryBase
 {
 IList<Reminder> GetAllReminders();
 Reminder GetReminderById(Guid id); 
 Reminder GetReminderByName(string name); 
 void CreateReminder(Reminder reminder); 
 void UpdateReminder(Reminder reminder); 
 void DeleteReminder(Reminder reminder); 
 }

The first issue with this interface is the abstraction duplication: you have two ways to create a Reminder: CreateReminder and Create<Reminder> (or Create in the original). Same for Update, Delete and for FindAll. As an API consumer I would have a hard time to pick one because I will feel insecure about the fact I might have picked the wrong one. As an Implementation developer I will have to implement the same functionality twice or to just delegate to the base class.

You have two options: either remove the interface inheritance or remove the the CreateReminder. If you remove the inheritance and keep the CreateXXX methods then you transmit the idea that the those XXXs are not handled as everything else by the repository, but the have a custom logic. I doubt that, since the repositories need to take care only for the store access. So remove these methods and let only the GetByName in the IReminderRepository. You might also decide to remove the interface inheritance and IRepositoryBase<Reminder> when you need those generic methods and IReminderRepository when you need the GetByName method.

Like I said you don't need to tie the UoW with the repositories interfaces. Follow this convention with the Repository suffix and you'll be fine to discover the repositories you need. Once you have that, there is no need to transform the UoW into a ServiceLocator - Repositories Factory.

Avoid newing dependencies

Which brings the next: use an IoC for you dependencies. .Net Core? - built in. .Net Framework: a bunch of them. Still WebForms? Use Property Injection.

Avoid disposing what you didn't create

public class UnitOfWork : IUnitOfWork, IDisposable

Did UnitOfWork create the DbContext? Then don't dispose. Let the creator of the DbContext to decide when and if wants to dispose it.

Naming:

void Create(T entity);
void Update(T entity);
void Delete(T entity);

It's a repository, by defintion: Mediates between the domain and data mapping layers using a collection-like interface for accessing domain objects. Take a look at the IList API and you'll find better naming:

void Add(T entity);
void Remove(T entity);

A list doesn't have an Update method so frankly you don't need this method because of this:

{
 var reminder = _repository.FindById(reminderId);
 reminder.AddNote(note); // probably AddNote will do something like this.Notes.Add(note);
 uow.Save();
}

See? No Update method. The DbContext takes care of the changes made to the returned reminder and will commit them when uow.Save is called.

Async - Await

Use this API as EntityFramework provides the asynchronous way to handle the databases. You'll be scalable, your threads you will be free to do something else (like handling new requests) and won't be blocked until the SQL Server decides to returns its results.

Big performance issue

public class ReminderRepository : RepositoryBase<Reminder>, IReminderRepository
{
 public ReminderRepository(DBContext dbContext)
 : base(dbContext)
 {
 _dbContext = dbContext;
 }
 //other methods removed
 public Reminder GetReminderByName(string name)
 {
 return FindAll()
 .OrderByDescending(r => r.Name)
 .FirstOrDefault(r => r.Name == name);
 //return FindByCondition(r => r.Name == name);
 }
}

The FindAll returns an IList which means the DbContext will send the query to the SQL server and then will map the results to the Reminder objects. After that the ReminderRepository object will search this in memory collection for the first reminder. What if you have 100.000 reminders? Probably you don't want to do that, but to generate the SQL that will have in it the WHERE clause. So avoid ToList().FirstOrDefault() or any indirection to that if you know you will need just a subset of these records/objects.

More about API

IList<T> FindAll();

IList has methods like Remove, Clear etc. Most likely the repository clients won't need that and even if they use them, the outcome won't be the expected one. The FindAll should return something that with they can only iterate/read. So use IReadonlyCollection.

How generic should be?

The FindByCondition has the drawback that you'll lose some encapsulation and abstraction and you open the gate for code duplication. I personally like it, but if I see that I tend to copy the predicate in a new place, then I'll add a new method in the repository, like you just did with FindByName. People are usually lazy (including myself) and sooner or later you'll get in trouble with it.

• niklodeon: 1) When u made IRepositoryBase interface non-generic then how will this interface compile as T is not defined anywhere?
• Adrian Iftode: Welcome, see that I move the T into the methods
• niklodeon: 2) In RemindersService if I have IRepositoryBase repository then I can't access GetReminderByName so I am assuming that I need to pass IReminderRepository repository
• niklodeon: As this is just a sample ASP.NET core project so using the built in IoC. Also I have in my task list to make repository also support async but first I wanted the implementation to be in a good shape. I have made few changes based on ur suggestions, can u plz take a look.
• niklodeon: 3) Regarding naming I understood that I can use better names but I don't understood why not to have Update. What if I need to update the Description of a Reminder. I know in certain cases I can do Delete & Add instead of Update.