BaseRepository for EF6

Question

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)
    {
        _dbContext.Entry(entity).State = EntityState.Added;
    }

    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();
    }
}

Answer 1

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.

Answer 2

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!

Answer 3

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.

Answer 4

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);
}

Answer 5

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.

Answer 6

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.