# BaseRepository for EF6

Is this okay?

public class BaseRepository<T> where T : class
{
private readonly DbContext _dbContext;

public BaseRepository(DbContext dbContext)
{
_dbContext = dbContext;
}

public T Get(Func<T, bool> predicate)
{
return GetAll(predicate).FirstOrDefault();
}

public IEnumerable<T> GetAll(Func<T, bool> predicate = null)
{
IEnumerable<T> result = _dbContext.Set<T>().AsEnumerable();
return (predicate == null) ? result : result.Where<T>(predicate);
}

public void Add(T entity)
{
}

public void Delete(Func<T, bool> predicate)
{
IEnumerable<T> entities = GetAll(predicate);
foreach (T entity in entities)
_dbContext.Entry(entity).State = EntityState.Deleted;
}

public void Delete(T entity)
{
_dbContext.Entry(entity).State = EntityState.Deleted;
}

public void Update(T entity)
{
_dbContext.Entry(entity).State = EntityState.Modified;
}

public async void Save()
{
await _dbContext.SaveChangesAsync();
}

public void Dispose()
{
if (_dbContext != null)
_dbContext.Dispose();
}
}

-
I have used this layer/pattern for two years in my projects ... most because of all tutorials I have seen when I started on asp.NET MVC. IMHO, abstract repository exists just to abstract an existing abstraction. I am coding much happier now that I removed it from my code :) –  Felipe Miosso May 19 '14 at 14:28
My code review. This is going to bring down your entire application. Fix Func<T, bool> for Expression<Func<T, bool>>. Stop using IEnumerable if you don't know what it is vs IQueryable. Just use var. –  Aron May 19 '14 at 14:47

It seems strange to me for Get to be a specific way of getting all items, and GetAll should always return all items. Introduce a GetSome helper for clarity.

public T Get(Func<T, bool> predicate) {
return GetWhere(predicate).FirstOrDefault();
}

public IEnumerable<T> GetAll() {
return GetWhere(null);
}

public IEnumerable<T> GetWhere(Func<T, bool> predicate) {
IEnumerable<T> result = _dbContext.Set<T>().AsEnumerable();
return (predicate == null) ? result : result.Where<T>(predicate);
}


Caveat: I don't write C# so perhaps this doesn't match the standard pattern there.

-
+1 thanks for the tip in my answer! I would use the names or Find and GetAll –  Bassam Alugili May 18 '14 at 22:45
Yes, in my Java DAOs I prefer findBy... and reserve get... for single-item finding methods that fail if no item is found. –  David Harkness May 18 '14 at 22:59
I would probably consider passing in () => true rather than null for the GetSome() method. Also maybe GetWhere instead of GetSome?? –  dreza May 19 '14 at 9:31
@dreza Great name! Regarding null versus a no-op predicate, can Where detect this and return the original result without scanning it needlessly? –  David Harkness May 19 '14 at 15:08
hmmm, a good question! My gut is it will still enumerate, but honestly I don't know. The compiler might be clever enough to optimize it out. –  dreza May 19 '14 at 20:25
public async void Save()
{
await _dbContext.SaveChangesAsync();
}


The name shall be SaveAsync code convention!

I would make the BaseRepository as abstract --> this will prevent the repository user from creating the Base Repository!

-
@DavidHarkness Yes I know it would be better if it do that!? –  Bassam Alugili May 18 '14 at 22:35

Your Get() function firstly, should be named something appropriate, such as First() or GetSingle() (etc). Secondly, I would use Expressions, instead of Func<>, for the simple reason that an IEnumerable<T> will return the full contents of a table, THEN perform the Func<T,bool> (ie, an SQL type WHERE) on the data. This will become very inefficient, VERY fast.

 internal IDbSet<T> dbSet = _dbContext.Set<T>();

public T Single(Expression<Func<T, bool>> predicate)
{
return dbSet.FirstOrDefault(predicate);
}


In general, use IQueryable over IEnumerable in your repository. You want the major filtering of data at the database level, not in your code.

-

All in all, not bad and pretty standard from what I can see. A couple of small comments.

1. I would assume a Get() method would return a single item. Hence to return a First would seem counter intuitive. If I expect a method to return multiple I would call the GetAll(). Consider renaming to SingleOrDefault() Or altering it's implementation to using SingleOrDefault().

2. Rather than using predicate as default I would consider creating an overload. It gets rid of the little inline iif and makes it easier to read, in my opinion.

public IEnumerable<T> GetAll()
{
return GetAll(() => true);
}

public IEnumerable<T> GetAll(Func<T, bool> condition)
{
return _dbContext.Set<T>().Where(predicate);
}

-

This method and sig will pull down the entire database table each and every time

public IEnumerable<T> GetAll(Func<T, bool> predicate = null)
{
IEnumerable<T> result = _dbContext.Set<T>().AsEnumerable();
return (predicate == null) ? result : result.Where<T>(predicate);
}


Don't use Queryable.AsEnumerable() if you don't know what it does.

You need

public IEnumerable<T> GetAll(Expression<Func<T, bool>> predicate = null)
{
IQueryable<T> result = _dbContext.Set<T>();
var ret = (predicate == null) ? result : Queryable.Where(result, predicate);
return ret.AsEnumerable();
}


Your delete also suffers from the same problem.

-
Most important answer here. –  Chris Trombley May 20 '14 at 19:40

I'd change:

public void Delete(Func<T, bool> predicate)
{
IEnumerable<T> entities = GetAll(predicate);
foreach (T entity in entities)
_dbContext.Entry(entity).State = EntityState.Deleted;
}


to:

public void Delete(Func<T, bool> predicate)
{
IEnumerable<T> entities = GetAll(predicate);
foreach (T entity in entities)
Delete(entity);
}


This way, you reuse your Delete(T Entity) function so you only need to change Delete(T Entity) if your method of deletion should ever change.

-