Problem
I would like you to review the solution I came up with to the following scenario:
-
In attempting to model a generic Repository object (that receives a generic type parameter) to interface with my data store, I created an abstract RepositoryBase class, that receives a DbContext parameter in the constructor, and implements a CRUD interface I called ICrud (providing create, read, update and delete methods).
-
Any repository in this application would then inherit from RepositoryBase and it would begin with all the CRUD methods ready for use.
-
I was asked not to do this, however, because not every entity the repository is intended to deal with should expose all CRUD methods. For example, the repository object that will handle DAL operations with object Message should expose only Read and Update methods (requirement imposition), whereas the BusinessCard repository is supposed to expose Create, Delete and Read.
-
What I ended up doing in the end was to create a helper class, named DALHelper, that implements ICrud. I then added a static class I called Infra, which has a method that returns an instance of a DALHelper.
Is this the optimal solution? Is there a better way to approach this?
RepositoryBase:
public abstract class RepositoryBase<T> where T : class
{
protected DbContext _dbContext;
private DbSet<T> _Entity { get { return _dbContext.Set<T>(); } }
public RepositoryBase()
{
_dbContext = new MilgamFormsContext();
}
public RepositoryBase(MilgamFormsContext dbContext)
{
_dbContext = dbContext;
}
}
ICrud:
public interface ICrud<T> where T : class, new()
{
List<T> Read();
List<T> Read(Predicate<T> predicate);
Int32 Create(T newEntity);
//Int32 Create(Predicate<T> predicate);
Int32 Update(T newEntityValues);
Int32 Update(Predicate<T> predicate, T newEntityValues);
Int32 Delete(T entity);
Int32 Delete(Predicate<T> predicate);
}
DALHelper:
public class DALHelper<T> : ICrud<T> where T : new()
{
DbContext _dbContext;
DbSet<T> _Entity { get { return _dbContext.Set<T>(); } }
public DALHelper(DbContext dbContext)
{
_dbContext = dbContext;
}
public List<T> Read()
{
List<T> messages = new List<T>();
try
{
messages = _Entity.ToList();
}
catch (Exception ex)
{
throw ex;
}
return messages;
}
public List<T> Read(Predicate<T> predicate)
{
List<T> filteredMessages = new List<T>();
try
{
filteredMessages = _Entity.Where((entity) => predicate(entity)).ToList();
}
catch (Exception ex)
{
throw ex;
}
return filteredMessages;
}
public int Create(T newEntity)
{
Int32 changedRows = 0;
try
{
if (newEntity != null)
{
_Entity.Add(newEntity);
changedRows = _dbContext.SaveChanges();
}
}
catch (Exception ex)
{
throw ex;
}
return changedRows;
}
public int Update(T rowToBeChanged)
{
Int32 changedRow = 0;
try
{
T rowCurrentState = _Entity.Find(rowToBeChanged.Id);
rowCurrentState = rowToBeChanged;
changedRow = _dbContext.SaveChanges();
}
catch (Exception ex)
{
throw;
}
return changedRow;
}
public Int32 Update(Predicate<T> predicate, T newEntityValues)
{
Int32 changedRows = 0;
List<T> changingRows = new List<T>();
try
{
changingRows = _Entity.Where((entity) => predicate(entity)).ToList();
for (Int32 i = 0; i < changingRows.Count; i++)
{
changingRows[i] = newEntityValues;
}
changedRows = _dbContext.SaveChanges();
}
catch (Exception ex)
{
throw;
}
return changedRows;
}
public Int32 Delete(T entity)
{
Int32 deletedRows = 0;
try
{
T removedEntity = _Entity.Remove(entity);
deletedRows = _dbContext.SaveChanges();
}
catch (Exception ex)
{
throw;
}
return deletedRows;
}
public Int32 Delete(Predicate<T> predicate)
{
Int32 deletedRows = 0;
List<T> rowsToDelete = new List<T>();
try
{
rowsToDelete = _Entity.Where(entityRow => predicate(entityRow)).ToList();
foreach (T entityRow in rowsToDelete)
{
_Entity.Remove(entityRow);
}
deletedRows = _dbContext.SaveChanges();
}
catch (Exception ex)
{
throw;
}
return deletedRows;
}
}
This is how a given Repository would look like:
public class MessageRepository : RepositoryBase<Message>
{
private DALHelper<Message> _dalHelper;
public MessageRepository()
{
_dalHelper = I.Infra.GetDALHelper<Message>(this._dbContext);
}
public Response<List<Message>> Get()
{
try
{
List<Message> messages = _dalHelper.Read();
return Response<List<Message>>.Success(messages);
}
catch (Exception ex)
{
return Response<List<Message>>.Failed(String.Format("Error in GetMessages. {0}", ex.Message));
}
}
public Response<Boolean> Create(Message message)
{
try
{
return _dalHelper.Create(message) == 1 ? Response<Boolean>.Success(true) : Response<Boolean>.Failed("Could not add message");
}
catch (Exception ex)
{
return Response<Boolean>.Failed(String.Format("Could not add message. {0}"), ex.Message);
}
}
}
Solution
In a nutshell, if you’re using Entity Framework using any kind of repository pattern for marking dirty entities, registering insertions or deletions is a waste of time as EF does this all for you, you may as well reference your EF context inside your controller/viewmodel/service or presenter. But you might be able to benefit by placing your Linq queries inside your repositories for easy code reuse.
Merge DALHelper code into the RepositoryBase as you originally thought
I’m personally against calling any class with a Helper extension – these types of classes tend to spiral out of control during ongoing development projects. Every object has a specific purpose and should be strongly named as such – this is just my opinion.
Inject the base class of MilgramFormsContext (usually DBContext) as the parameter instead of the strongly-typed definition. This means you can reuse this RepositoryBase class in other projects which use a different model.
I’m usually against writing static methods and classes as you can’t inject these into other classes (for example MessageRepository references the static class Infra) – how are you going to unit test your MessageRepository when it contains a direct reference to a static class which you cannot mock??
Extract an interface from the RepositoryBase class called IRepository
This will be used as reference within your Controller/Presenter/ViewModel depending upon the GUI design pattern chosen;
You can use a dependency injection framework to config the strongly-typed repositories which will be instantiated rather than referencing MessageRepository directly in your code – you would reference the abstracted interface (IRepository) instead as suggested above.
I’m not sure if you have abbreviated the code in the try/catch statements but rethrowing an exception without doing anything is more harm than good – you lose the stack trace. In the examples you’ve provided I wouldn’t CATCH any exceptions unless there was specific logic to be ran under this condition. In regards to capturing exceptions I usually let these bubble up to the View and handle them there – i.e. display an appropriate error dialog to the user.
One thing you need to be aware of with Entity Framework is that related entities when connected together automatically get their state updated to that of it’s related entity based upon the state being set. For example IF BusinessCard and Message are an associated dependency and the BusinessCard entity is marked for deletion then the related Message entity is also marked for deletion. However, based upon the logic you want to imply it appears you want to prevent the Message entity from being deleted (I’m assuming this as you’re not exposing the Delete method for this repository). The other thing to think about is whether you want to allow lazy loading on your entities – if you try to access the related entity collection this will automatically call out to the database to get those entities, essentially bypassing your repository.
Using the Repository pattern with an ORM always makes things confusing as Entity Framework implements the UnitOfWork and Repository patterns anyway. DBContext exposes the Insert and Delete operations through the DBSets along with committing changes therefore it is a UOW pattern. Your DBSets are essentially repositories in themselves. What you want todo is write your Linq queries inside the repository classes specific to the entity type you’re querying this keeps them centralised. Traditionally, like you have done also, you would expose Insert/Update/Delete methods for the repository to persist any changes but for EF this really isn’t required.
The below is an attempt to accomplish what you require or what it should essentially look like but please remember the caveats as mentioned above.
class Program
{
static void Main(string[] args)
{
using (MilgamFormsContextUnitOfWork ctx = new MilgamFormsContextUnitOfWork())
{
//get all cards
var cards = ctx.BusinessCardRepository.Read().ToList();
//mark the first entry for deletion
ctx.RegisterDelete(cards.Skip(1).First());
//get a specific phone number
var messages = ctx.MessageRepository.GetMessages(cards.First().ID);
/*
* create a new BusinessCard and register to insert - this will also
* automatically associate Message to be inserted too essentially
* bypassing the call to the Message repository!
*/
BusinessCard bc = new BusinessCard();
p.Messages.Add(new Message());
ctx.RegisterInsert(bc);
/*
* create a new phone number - however the repository
* prevents these types of object from being created
*/
Message m = new Message();
ctx.RegisterInsert(m);
//save changes
ctx.Commit();
}
}
}
public class MilgamFormsContextUnitOfWork : UnitOfWork
{
public IBusinessCardRepository BusinessCardRepository
{
get { return (IBusinessCardRepository)_repositories[BusinessCard.ENTITY_NAME]; }
}
public IMessageRepository MessageRepository
{
get { return (IMessageRepository)_repositories[Message.ENTITY_NAME]; }
}
public MilgamFormsContextUnitOfWork() : base()
{
_ctx = new MilgamFormsContext2008Entities();
/*
* There's no reason why these repositories can't be injected
* as instances to the constructor or dynamically set through
* a DI framework by adding setters on the above repository props
*/
_repositories.Add(BusinessCard.ENTITY_NAME, new BusinessCardRepository(this._ctx));
_repositories.Add(Message.ENTITY_NAME, new MessageRepository(this._ctx));
}
}
public abstract class UnitOfWork : IDisposable
{
protected DbContext _ctx;
protected readonly IDictionary<string, IRepository<object>> _repositories;
public UnitOfWork()
{
_repositories = new Dictionary<string, IRepository<object>>();
}
public void RegisterInsert<TEntity>(TEntity entity)
where TEntity: class, IEntity
{
/* relay to the repository as this will handle as to whether
* we're allowed to insert this type of object or not */
_repositories[entity.EntityName].Insert(entity);
}
public void RegisterDelete<TEntity>(TEntity entity)
where TEntity : class, IEntity
{
/* relay to the repository as this will handle as to whether
* we're allowed to delete this type of object or not */
_repositories[entity.EntityName].Delete(entity);
}
public void Commit()
{
_ctx.SaveChanges();
}
public void Dispose()
{
_ctx.Dispose();
}
}
public interface IEntity
{
string EntityName { get; }
}
public partial class Message : IEntity
{
public static string ENTITY_NAME = "Message";
public string EntityName { get { return ENTITY_NAME; } }
}
public partial class BusinessCard : IEntity
{
public static string ENTITY_NAME = "BusinessCard";
public string EntityName { get { return ENTITY_NAME; } }
}
public interface IMessageRepository
{
IList<Message> GetMessages(int id);
}
public class MessageRepository : EFRepositoryBase<Message>, IMessageRepository
{
public MessageRepository(DbContext ctx) : base(ctx) { }
public override void Insert(object entity)
{
throw new NotSupportedException("Can not insert new Messages");
}
public override void Delete(object entity)
{
throw new NotSupportedException("Can not delete existing Messages");
}
public IList<Message> GetMessages(int id)
{
return this.Read(msg => msg.ID == id).ToList();
}
}
public interface IBusinessCardRepository : IRepository<BusinessCard>
{
BusinessCard GetBusinessCard(int id);
}
public class BusinessCardRepository : EFRepositoryBase<BusinessCard>, IBusinessCardRepository
{
public BusinessCardRepository(DbContext ctx) : base(ctx) { }
public BusinessCard GetBusinessCard(int id)
{
return this.Read(bc => bc.ID == id).SingleOrDefault();
}
}
public interface IRepository<out TEntity>
where TEntity: class
{
void Insert(object entity);
void Delete(object entity);
IEnumerable<TEntity> Read();
}
public abstract class EFRepositoryBase<TEntity> : IRepository<TEntity>
where TEntity : class
{
protected DbContext _ctx;
DbSet<TEntity> _set { get { return _ctx.Set<TEntity>(); } }
public EFRepositoryBase(DbContext dbContext)
{
_ctx = dbContext;
}
public IEnumerable<TEntity> Read()
{
return _set.ToList();
}
protected IQueryable<TEntity> Read(Expression<Func<TEntity, bool>> predicate)
{
return _set.Where(predicate);
}
public virtual void Insert(TEntity entity)
{
_ctx.Set<TEntity>().Add(entity);
}
public virtual void Delete(TEntity entity)
{
_ctx.Set<TEntity>().Remove(entity);
}
public virtual void Insert(object entity)
{
if (entity == null) throw new ArgumentNullException();
_ctx.Set(entity.GetType()).Add(entity);
}
public virtual void Delete(object entity)
{
if (entity == null) throw new ArgumentNullException();
_ctx.Set(entity.GetType()).Remove(entity);
}
}
Why not actually have Repository Base implement ICrud and simply expose the methods I want in any given Repository itself ? No need for a “Helper” static class…