Repository/Service Design Pattern

Question

I use the Repository/Service design pattern in my projects and I have found something that might be a bit redundant. Am I writing any unnecessary code?

With that in mind, here is my structure:

public class Competition
{
    public int Id { get; set; }
    [Required] public string UserId { get; set; }
    public string UserName { get; set; }
    public DateTime DateCreated { get; set; }
    public DateTime? DateModified { get; set; }
    [Required] public string Name { get; set; }
}

This is an example POCO class, but as you will see I am using Generics so it can be anything:

internal interface IRepository<TEntity> : IDisposable where TEntity : class
{
    IList<TEntity> GetAll();
    TEntity Get(int id);
    void Save(TEntity model);
    void Delete(int id);
}

My interface implements IDisposable and allows any class object to be set as TEntity:

internal class CompetitionRepository : IRepository<Competition>
{
    private readonly string userId;
    private readonly CompetitionProvider provider;

    public CompetitionRepository(string userId) : this(userId, "DefaultConnection")
    {
    }

    public CompetitionRepository(string userId, string connectionString)
    {
        this.userId = userId;
        this.provider = new CompetitionProvider(connectionString);
    }

    public IList<Competition> GetAll()
    {
        return provider.Get(this.userId);
    }

    public Competition Get(int id)
    {
        return GetAll().Where(model => model.Id == id).SingleOrDefault();
    }

    public void Save(Competition model)
    {
        provider.Save(model);
    }

    public void Delete(int id)
    {
        provider.Delete(id);
    }

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

My repository which implements IRepository, specifying Competition as TEntity. This repository queries my database hence the provider.

In this repository, I only do the minimal of what is required to query my database:

public class CompetitionService : IRepository<Competition>
{

    private readonly string userId;
    private readonly IRepository<Competition> repository;

    public CompetitionService(string userId) : this(userId, "DefaultConnection")
    {
    }

    public CompetitionService(string userId, string connectionString)
    {
        this.userId = userId;
        this.repository = new CompetitionRepository(this.userId, connectionString);
    }

    public IList<Competition> GetAll()
    {
        return this.repository.GetAll();
    }

    public Competition Get(int id)
    {
        return this.repository.Get(id);
    }

    public void Save(Competition model)
    {
        if (model.Id > 0)
        {
            model.UserId = userId;
            model.DateCreated = DateTime.UtcNow;
        } else
        {
            model.DateModified = DateTime.UtcNow;
        }

        this.repository.Save(model);
    }

    public void Delete(int id)
    {
        this.repository.Delete(id);
    }

    public void Dispose()
    {
        this.repository.Dispose();
    }
}

Now this is my service. For example, you can see the Save method assigns values depending on whether we are creating or updating. A Service class can have anything in there, for example performing an uploads or whatever is not related to data source.

The Repository is there to interact with the data source only and no other code is present. This allows for swapping out the repository with another that queries a different data source.

My problem is that I have noticed that, although the service has code that modifies and corrects the values before they are then saved via the repository, the structure is always very similar to the repository, i.e. it has:

GetAll()

Get()

Save()

Delete()

I have implemented IRepository<Competition> as well. The service may have other public methods, but it doesn't matter because, in my controllers, I would call it like this:

//
// POST: /Competitions/Create
[HttpPost]
public ActionResult Create(Competition model)
{
    try
    {
        using (var service = new CompetitionService(User.Identity.GetUserId()))
        {
            service.Save(model);
        }

        return RedirectToAction("Index");
    }
    catch
    {
        return View(model);
    }
}

1. Do you think it is good practice or do you think I am over-killing it a bit?
2. Do you think I should implement IRepository on the service?
3. In the more extreme case, do you think I should even bother with a service?

(NB: I am reluctant to remove the service because sometimes there is a lot of code that needs to go in there that I don't want clogging up my controllers)

Answer 1

You are on the right track. The separation of business logic (services) and data-access logic (repositories) is a good thing, I strongly recommend this. I created a generic repository implementing the IRepository<TEntity> interface. It's slightly different than yours, but you get the point (I have a BaseEntity constraint, you can also use class for this):

public class EfRepository<TEntity> : IRepository<TEntity>
    where TEntity : BaseEntity, new()
{
    private readonly IDbContext _context;
    private IDbSet<TEntity> _entities;

    public EfRepository(IDbContext context)
    {
        _context = context;
    }

    private IDbSet<TEntity> Entities
    {
        get { return _entities ?? (_entities = _context.Set<TEntity>()); }
    }

    public TEntity GetById(object id)
    {
        return Entities.Find(id);
    }

    public void Insert(TEntity entity)
    {
        Entities.Add(entity);
        _context.SaveChanges();
    }

    public void Update(TEntity entity)
    {
        _context.SaveChanges();
    }

    public void Delete(int id)
    {
        // Create a new instance of an entity (BaseEntity) and add the id.
        var entity = new TEntity
                     {
                         ID = id
                     };

        // Attach the entity to the context and call the delete method.
        Entities.Attach(entity);
        Delete(entity);
    }

    public void Delete(TEntity entity)
    {
        Entities.Remove(entity);
        _context.SaveChanges();
    }

    public IList<TEntity> Table
    {
        get { return Entities.ToList(); }
    }
}

This greatly reduces the amount of duplicate code when you would use specific repositories. I use this in my services like:

public class ProductService : IProductService
{
    private readonly IRepository<Product> _productRepository;

    public ProductService(IRepository<Product> productRepository)
    {
        _productRepository = productRepository;
    }

    public bool AddProduct(Product product)
    {
        // Validate etc.
        _productRepository.Insert(product);

        // return validation result.
        return true;
    }
}

As you may have noticed I use an IoC framework to inject the dependencies.

Answer 2

I sometimes dream that I'm a god but my real-life situation couldn't be further from that.

I am however willing to share my thoughts on your situation.

First of all I would never get rid of your service layer, I am totally in favor of having your controllers as lightweight as possible!

I'm not sure what you are using behind that repository layer, but by the looks of it your respository classes are not doing much besides passing the calls through. Maybe make a generic respository that handles most entities and if you need something more specific create a new respository?

You could also consider using your Provider directly from your service and treat this provider as a repository.

As for the IRepository in the service layer, I must say I'm against it. If you set up your service as a repository then your controller is limited to these CRUD-like methods. This might work out early on if you just have CRUD-like screens but I'm not a big fan of looking at services this way.

In my opinion the service should provide a method for every logical action that you want to do with that entity.

Save and Edit can be a part of that, but so would be AddNewAnswerToQuestion, RaiseFlagToQuestion, AddTagToQuestion (for example). I fear that if you limit yourself to CRUD-like methods you will either have to make a lot of if structures in your Save method (if tag has been added, if flag has been set) or your controller will end up doing more and more work (call Save method, then call Mail method because a mail needs to be sent in case of an extra tag).

If you take this a bit more extreme you no longer make a service per entity but a service per action.

At that point you stop calling them services and start calling them commands, and use CQS (Command-Query Separation) but that choice is up to you :)

So in short, I would make a generic repository, or remove them altogether and make my services more explicit by either making more methods or creating commands instead of services.

Answer 3

First of all: Why don't you implement your repo as generic? I mean the implementation of generic interface. In your solution, you have to create an IRepository<Entity> implementation for each new POCO. That will bloat your code, and you will not get any use of generic interface.

Here is the example of generic repo implementation:

public class Repository<T> : IRepository<T> where T : class, new()
{
    private IYourContext _context;

    public Repository(IYourContext context)
    {
        this._context = context;
    }

    public IQueryable<T> All
    {
        get { return _context.Set<T>(); }
    }

    public T Find(int id)
    {
        return _context.Set<T>().Find(id);
    }

    public void Delete(T entity)
    {
        if (_context.Entry(entity).State == EntityState.Detached)
            _context.Set<T>().Attach(entity);
        _context.Set<T>().Remove(entity);
    }

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

    public void Update(T entity)
    {
        _context.Set<T>().Attach(entity);
        _context.Entry(entity).State = EntityState.Modified;
    }
}

You can have different methods in your interface and implementation. If that's not the case, then you can use your repo like so:

IRepository<YourPOCO> repo = new Repository<YourPOCO>();

Second: There is a pattern Unit Of Work that is often implemented with Generic Repository pattern. It is a wrapper around repositories that provides a way of sharing one context between all of them.

Here is the example:

public interface IUnitOfWork : IDisposable
{
    IRepository<Product> ProductRepository { get; }
    // other repositories                

    void SaveChanges();
    void RollbackChanges();
}

And the implementation:

public class UnitOfWork : IUnitOfWork
{
    private IRepository<Product> _productRepository;
    private IYourContext _context;
    private bool _disposed = false;

    public UnitOfWork(IYourContext context)
    {
        this._context = context;
    }

    // Lazy Loading pattern is also often used here
    public IRepository<Product> ProductRepository
    {
        get
        {
            if (this._productRepository == null)
                this._productRepository = new Repository<Product>(_context);
            return _productRepository;
        }
    }

    public void SaveChanges()
    {
        _context.SaveChanges();
    }

    // implement other functionality
}

With unit of work, when you've made changes in different repositories, you just need to call SaveChanges() method of the UnitOfWork class only once. So, it will be a single transaction.

Also, maybe you've noticed, I use interfaces everywhere, even in the db context implementation. That will help you to unit test your logic in the future if needed. And use Dependency Injection!

For the context:

public interface IYourContext : IDisposable
{
    DbSet<T> Set<T>() where T : class;
    DbEntityEntry Entry(object entry);
    int SaveChanges();
}

public class YourContext : DbContext, IYourContext
{
    public NorthwindContext(IConnectionFactory connectionFactory) : base(connectionFactory.ConnectionString) { }

    public DbSet<Product> Products { get; set; }
    // other DbSets ..
}

And a connection factory (without it you will get an exception in the DI container):

public interface IConnectionFactory
{
    string ConnectionString { get; }
}

public class ConnectionFactory : IConnectionFactory
{
    public string ConnectionString
    {
        get { return "YourConStringName"; }
    }
}

To summarize: Implement generic repository and unit of work patterns as a DAL (Data Access Layer) and then you can build any service layers with your BL (Business Logic) on top of them.