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);
}
}
- Do you think it is good practice or do you think I am over-killing it a bit?
- Do you think I should implement
IRepository
on the service? - 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)