Database first entity framework, repository, service, UnitOfWork pattern

Question

I have seperated my project into two projects. One is the web forms project and the other is the Model project with contains a generic repository implementation, a service class for each of my entities and a UnitOfWork class. All the code below apart from the last section is in my Model class library project.

Generic repository interface

public interface IRepository<TEntity> : IDisposable where TEntity : class
{
    IQueryable<TEntity> GetAll();
    IQueryable<TEntity> GetWhere(Expression<Func<TEntity,bool>> predicate);
    TEntity GetSingle(Expression<Func<TEntity, bool>> predicate);
    TEntity GetSingleOrDefault(Expression<Func<TEntity, bool>> predicate);

    void Add(TEntity entity);
    void Delete(TEntity entity);
}

I start of with this Interface which my services will interact with.

A few points about this class:

• As i am having a service for each of my entity object, I see no reason why I shouldn't use a generic repository as creating a repository for each entity object as-well, would bloat the amount of classes i have and make it harder to manage, when i can just pass a predicate from my service into the Get methods to retrieve the data i need. So instead of creating multiple methods such as "GetById", "GetByName" in a repository specific to each entity i use a generic one and include those methods in the service class. However i have heard many people say the generic repository is an anti-pattern so i'm not sure if this is the right way.
• I do not include a save changes methods in the repositories as i want to make any interactions with these only come from the UnitOfWork class (which will expose the service classes as properties). So this should force validation to be done via the service classes as these are the only objects which will interact with repository. In every example i see the repository has a save changes method so i'n not sure if this is a reasonable approach.

Generic repository

Below is the implementation of the IRepository for my web app, I think it has a pretty standard structure. I swap this out for another implementation when testing the service classes. I don't think there is much to say about it other than what i mentioned above.

internal class RepositoryBase<TEntity> : IRepository<TEntity> where TEntity : class
{
    private bool isDisposed;

    private DbSet<TEntity> dbSet;
    public DbSet<TEntity> DbSet
    {
        get { return dbSet; }
    }

    private RequestboxEntities requestboxContext;

    public RepositoryBase(RequestboxEntities requestboxContext)
    {
        this.requestboxContext = requestboxContext;
        this.dbSet = requestboxContext.Set<TEntity>();
    }

    public IQueryable<TEntity> GetAll()
    {
        return dbSet;
    }

    public IQueryable<TEntity> GetWhere(System.Linq.Expressions.Expression<Func<TEntity, bool>> predicate)
    {
       return dbSet.Where(predicate);
    }

    public TEntity GetSingle(System.Linq.Expressions.Expression<Func<TEntity, bool>> predicate)
    {
        return dbSet.Single(predicate);
    }

    public TEntity GetSingleOrDefault(System.Linq.Expressions.Expression<Func<TEntity, bool>> predicate)
    {
        return dbSet.SingleOrDefault(predicate);
    }

    public void Add(TEntity entity)
    {
        dbSet.Add(entity);
    }

    public void Delete(TEntity entity)
    {
        dbSet.Remove(entity);
    }

    public virtual void Dispose(bool isManuallyDisposing)
    {
        if (!isDisposed)
        {
            if (isManuallyDisposing)
                requestboxContext.Dispose();
        }

        isDisposed = true;
    }

    public void Dispose()
    {
        Dispose(true);
        GC.SuppressFinalize(this);
    }

    ~RepositoryBase()
    {
        Dispose(false);
    }

} 

Service classes

These are the only classes that will interact with the generic repository and will be used in the UnitOfWork class. Below is part of one of my service classes, i have left out some code to improve readability, all other services are very similar to this one.

public class GroupService
{
    IUnitOfWork unitOfWork;
    IRepository<Group> groupRepository;

    public GroupService(IRepository<Group> groupRepository, IUnitOfWork unitOfWork)
    {
        this.groupRepository = groupRepository;
        this.unitOfWork = unitOfWork;
    }

    public Group GetById(int id)
    {
        return groupRepository.GetSingle(g => g.Id == id); 
    }

    public List<Group> GetProductGroupsUserBelongsTo(int instanceId, Guid userId)
    {
        IQuerable<UserGroup> userGroups = unitOfWork.UserGroupService.GetByUserId(userId);
        IQuerable<Group> groupsInInstance = groupRepository.GetWhere(g => g.InstanceId == instanceId);

        return groupsInInstance.Join(userGroups, g => g.Id, u => u.GroupId, (g, u) => g).ToList();
    }

    public void Add(Group group)
    {
       if(!ExistsInInstance(group.Name,group.InstanceId))
           groupRepository.Add(group);
       else 
           throw new Exception("A group with the same name already exists for the instance");          
    }

    public bool ExistsInInstance(string name, int instanceId)
    {
        return GetAll().SingleOrDefault(g => g.Name == name && g.InstanceId == instanceId) != null;
    }
}

Some points and concerns about these classes

• I have made these classes require a IUnitOfWork class in the constructor. This is for functions which require interacting with a repository other than it's own. In the "GetProductGroupsUserBelongsTo" method i need to read from my UserService class aswell so instead of creating a new UserService object i use the existing one in the UnitOfWork object passed through to the service. I did this as it ensures I always use the same entity context (set in the UnitOfWork class) when using different services/repositories. Doing it this way also makes sure i don't use the context directly in my service classes, which makes them easier to test.
• In my Get Methods i generally use GetSingle() so an error is thrown if an object doesnt exist for the Id. However i am not sure if i should be using GetSingleOrDefault() and then handling the null on the client that consumes the service or if i should expect the client to use the "ExistsInInstance" method to check if it exists before calling the Get Methods.

UnitOfWork interface

I have removed some of the services for readability purposes, but there are many more services defined as properties just like the "GroupService".

public interface IUnitOfWork : IDisposable
{
    GroupService GroupService { get; }
    UserService UserService { get; }

    void Commit();
}

UnitOfWork Class

 public class UnitOfWork : IUnitOfWork
{
    private bool disposed = false;
    private RequestboxEntities requestboxContext = new RequestboxEntities();

    private GroupService groupService;
    private UserService userService;

    public GroupService GroupService
    {
        get
        {
            if (groupService == null)
                groupService = new GroupService(new RepositoryBase<Group>(requestboxContext));

            return groupService;
        }

    }

    public UserService UserService
    {
        get
        {
            if (userService == null)
                userService = new UserService(new RepositoryBase<User>(requestboxContext));

            return userService;
        }

    }

    public UnitOfWork()
    {
        requestboxContext = new RequestboxEntities();
    }

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

    public void Refresh()
    {
        requestboxContext.Dispose();
        requestboxContext = new RequestboxEntities();
    }

    protected virtual void Dispose(bool isManuallyDisposing)
    {
        if (!this.disposed)
        {
            if (isManuallyDisposing)
                 requestboxContext.Dispose();
        }

        this.disposed = true;
    }

    public void Dispose()
    {
        Dispose(true);
        GC.SuppressFinalize(this);
    }

    ~UnitOfWork()
    {
        Dispose(false);
    }
}

A few points about this class:

• Each service is exposed as a public property. This is so when working with it you can easily call the methods like this "unitOfWork.GroupService.GetById(id)"

• This class shares the same entity context between each service, so to ensure when working with the same unitOfWork object everything is kept in sync. This is the only class that can save the context via the "Commit" method.

Working with the Model

So that's the structure of my Model project here is an example if how i consume it in a web page. I know this might fit better in an MVC project rather than webforms but that is not a realistic option at the moment and my main concerns are with the structure and use of the Model project not the web app.

 public class BasePage : System.Web.UI.Page
{
   UnitOfWork uow = new UnitOfWork();

   protected override void OnUnload(EventArgs e)
    {
        uow.Dispose();
        base.OnUnload(e);
    }
}

Each page will inherit this class so a unique UnitOfWork will be made for each request and will always be disposed on the "Unload" event. Then to use it in a page i can do something like this.

 public class ManageGroupsPage : BasePage
{
   protected void Page_Load(object sender, EventArgs e)
    {
        int groupId = (int)dropdown.SelectedItem.Value;
        Group group = uow.GroupService.GetById(groupId);

        NameTextBox.text = group.Name;
    }
}

I can then easily and quickly work with the UnitOfWOrk object by interacting with it's public properties (The Service classes).

Conclusion and Concerns

My main concerns are with the structure of the Model project and how the layers interact with each other. Whether it is a good idea to use the generic repository, if the service classes should be passed through a IUnitOfWork object as this prevents them from working without this class so it's not as flexible. And if it is a good idea to have evrything rely on the UnitOfWork class.

Answer 1

I've used the Generic repository solution myself in the past. At first I found it really great but after a while as the project got a bit bigger it started to feel a bit restrictive and didn't seem to offer much benefits for the amount of code written.

The more I read the more it appeard Entity framework was already a Repository / UnitofWork. Why did I need another layer? Well I'm sure there are plenty of reasons why but in my projects I couldn't see it.

Instead I found myself starting to use the entity-framework directly within a service layer but abstracting the EF away using interfaces. This allowed me to remove the Repository layer whilst maintaining the level of abstraction I was after between the UI layer and any business or data layer (as well as continuing to easily mock for unit tests).

Using your solution an alternative approach I might consider would be something like:

public interface IUnitOfWork
{
   public IDbSet<Group> Groups { get; set; }
   // other entity sets here

   System.Data.Entity.Database Database { get; }
   DbEntityEntry<TEntity> Entry<TEntity>(TEntity entity) where TEntity : class;
   int SaveChanges();
}

My Unit of work was essentially my Entity framework data context

public class MyDbContext : DbContext, IUnitOfWork
{
   public DbSet<Group> Groups { get; set; }

   // etc
}

I would now only use a service layer directly interacting with the IUnitOfWork. However I would also consider abstracting each service behind an interface itself.

public interface IGroupService : ICrudService<Group> // Maybe, maybe not depending
{
    Group GetById(int id);
}

If there were a group of interfaces that shared a common theme i.e. CRUD I might consider making those seperate and implementing them where necessary. Or even a base service class to contain common methods.

public interface ICrudService<TEntity>
{
   TEntity GetById(int id);
   void Update(TEntity entity);
   void Add(TEntity entity);
   void Delete(TEntity entity);
}

public class GroupService : IGroupService
{
   private readonly IUnitOfWork _unitOfWork;

   public GroupService(IUnitOfWork unitOfWork)
   {
      _unitOfWork = unitOfWork;
   } 

   public Group GetById(int id)
   {
      return _unitOfWork.Find(id);
   }
}

If my service required other services I would simply pass those into the constructor. If I found myself passing too many then I would start knowing I had a bit of SRP mis-use and consider creating another service or re-arranging my existing code base.

If you wanted a GOD service class you could still achieve this as you did with your UnitOfWork example by doing something like:

public interface IServiceContext
{
   IGroupService GroupService { get; private set; }
   // And all your other services
}

Hopefully you get the drift....