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.

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)

share|improve this question
    
Why ou not use the UnitOfWork pattern as described in the Martin blog martinfowler.com/eaaCatalog/unitOfWork.html just remove the save and the delete methods from the Repository interface and do this job in the commit of the unit of work. –  Bassam Alugili Oct 10 '13 at 13:03
1  
Interfaces shouldn't "implement" IDisposable, only the concrete implementation should, otherwise it's a leaky abstraction. Mock repos are disposable? –  Mat's Mug Oct 23 '13 at 13:52
add comment

migrated from stackoverflow.com Oct 23 '13 at 4:10

This question came from our site for professional and enthusiast programmers.

3 Answers

up vote 2 down vote accepted

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.

share|improve this answer
    
Yeah, it seems all the answers use dependency injection. I hadn't thought of it properly, I shall adapt this also. Cheers –  r3plica Oct 10 '13 at 17:12
    
What is the IoC framework you are using? Also, how do you go about configuring the EfRepository<TEntity> injection? Using AutoFac, I've done something like: builder.RegisterAssemblyTypes(typeof(ProjectRepository).Assembly).Where(t => to.Name.EndsWith("Repository")).AsImplementedInterfaces()... Who would that transfer? –  mickyjtwin Mar 11 at 2:47
    
@mickyjtwin I'm using Simple Injector which allows me to do: container.RegisterOpenGeneric(typeof (IRepository<>), typeof (EfRepository<>));. –  Henk Mollema Mar 11 at 8:23
    
@HenkMollema, all you explained seems to make sense, but I've got little problem. My repository implements IDisposable. Advantage of my solution is that repositories operate on simple db operations and I decide when to close db connection. Problem is when I want to use repository in service. I would like to dispose IRepository in the end of each Service Method. I need some way to save Type of IRepository, not concrete instance. I considered using code reflections, but I am afraid of performance and it will look ugly I concerned. Do you know some design pattern or tool to achieve my aim? –  pt12lol Apr 13 at 22:43
    
pastebin.com/Quhp8Mz7 - here is example of solution I thought of. –  pt12lol Apr 14 at 6:20
show 4 more comments

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.

share|improve this answer
    
Nice comment, you said everything I was thinking :) Cheers –  r3plica Oct 10 '13 at 17:12
add comment

First of all: Why don't You implement You repo as generic? I mean the implementation of generic interface. In Your case You will have to create IRepository implementation for each new POCO. That's will blow up Your code and You will not get any use of generic interface.

Here is the example of generic repo implemetation:

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, that's not the case. So You can use Your repo then 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 UnitOfWork class only once. So, it will be a single transaction.

Also, mabby You 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 exception in 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 the top of them.

Hope this helps!

share|improve this answer
    
Great answer, thanks I have marked this as the answer along with the others. I shall have a look at the UnitOfWork pattern as I have not used that before. Cheers –  r3plica Oct 10 '13 at 17:11
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.