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.

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();
    }
}
share|improve this question
    
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 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 at 14:47
add comment

6 Answers 6

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.

share|improve this answer
1  
+1 thanks for the tip in my answer! I would use the names or Find and GetAll –  Bassam Alugili May 18 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 at 22:59
1  
I would probably consider passing in () => true rather than null for the GetSome() method. Also maybe GetWhere instead of GetSome?? –  dreza May 19 at 9:31
1  
@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 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 at 20:25
add comment
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!

share|improve this answer
    
@DavidHarkness Yes I know it would be better if it do that!? –  Bassam Alugili May 18 at 22:35
add comment

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.

share|improve this answer
add comment

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);
}
share|improve this answer
add comment

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.

share|improve this answer
1  
Most important answer here. –  Chris Trombley May 20 at 19:40
add comment

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.

share|improve this answer
add comment

Your Answer

 
discard

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

Not the answer you're looking for? Browse other questions tagged or ask your own question.