current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 1 down vote favorite
1

Would like to ask for a code review for my DAL. I use Entity framework 6 and Autofac for dependency injection. All comments will be appreciated.

    public interface IBaseEntityObject 
    {
        public int Id {get; set;}
    }

    public abstract class BaseEntityObject : IBaseEntityObject
    {
        [Key]
        [DatabaseGenerated(DatabaseGeneratedOption.Identity)]
        public int Id {get; set;}
    }

    public class Folder : BaseEntityObject
    {   
        [DataMember]
        public string Name {get; set;}

        [DataMember]
        public List<Letter> Letters {get; set;} 
    }

    public abstract class Letter : BaseEntityObject
    {   
        [DataMember]
        public string Title {get; set;}

        [DataMember]
        public string Content {get; set;}

        public virtual Folder Folder {get; set;}

        [DataMember]
        public int FolderId {get; set;}

        [DataMember]
        public DateTime CreationDate {get; set;}
    }

    public class OutgoingLetter : Letter
    {
        // .. OutgoingLetter properties
    }

    public class ReceviedLetter : Letter
    {
        // .. ReceviedLetter properties
    }


    public class MyDbContext : DbContext
    {
        public DbSet<Folder> Folders {get; set;}

        public DbSet<Letter> Letters {get; set;}


        protected override void OnModelCreating(DbModelBuilder modelBuilder)
        {
            base.OnModelCreating(modelBuilder);

            // Folder <-> Letters       
            modelBuilder.Entity<Letter>()
            .HasRequired(t => t.Folder)
            .WithMany(f => f.Letters)
            .HasForeignKey(t => t.FolderId)
            .WillCascadeOnDelete(true);
        }
    }

public IRepository<T> where T: BaseEntityObject
{
    void Add(T entity);

    void Remove(T entity);

    void Attach(T entity);

    void MarkAsModified(T entity);

    List<T> Get();

    T FindById(int id);

    int SaveChanges();

    void Dispose(); 
}


public abstract class EFRepository<T> : IRepository<T>
{
    protected readonly DbContext Context;

    public EFRepository(DbContext context)
    {
        Context = context;
    }

    public abstract List<T> Get();

    public void Add(T item)
    {
        Context.Set<T>().Add(item);
    }

    public void Remove(T item)
    {
        Context.Set<T>().Remove(item);
    }

    public void Attach(T item)
    {
        Context.Set<T>().Attach(item);
        MarkAsModified(item);
    }

    public void MarkAsModified(T item)
    {
        Context.Entry(item).EntityState = EntityState.Modified;
    }

    public void Update(T item)
    {
        Context.Entry(item).State = EntityState.Modified;
    }

    public void Dispose()
    {
        Context.Dispose();
    }

    public int SaveChanges()
    {
        return Context.SaveChanges();
    }

    public T FindById(int id)
    {
        return Context.Set<T>().Find(id); 
    }
}


public LettersRepository : EFRepository<Letter>
{
    public LettersRepository(DbContext context) : base(context) {}

    // Override for case includes will be needed in future
    public override List<T> Get()
    {
        return Context.Set<T>().ToList();
    }
}

public FoldersRepository : EFRepository<Folder>
{
    public FoldersRepository(DbContext context) : base(context) {}

    public override List<T> Get()
    {
            return Context.Set<T>().Include("Letters").ToList();
    }
}


public interface IUnitOfWork
{
        void Commit();
}

public class EFUnitOfWork : IUnitOfWork 
{
    private readonly DbContext _context;

    public IRepository<Folder> foldersRepository;
    public IRepository<Letters> lettersRepository;

    public class EFUnitOfWork(DbContext context, IRepository<Folder> folders, IRepository<Letter> letters)
    {
        _context = context;
        _lettersRepository = letters;
        _foldersRepository = folders;       
    }

    public void Commit()
    {
        _context.SaveChanges();
    }

    public void Dispose()
    {
        _context.Dispose();
    }
}

public class DataService : IDataService
{
    private Func<Owned<Folder>> _unitOfWorkFactory;

    public DataService(Func<Owned<IUnitOfWork>> unitOfWorkFactory)
    {
        _unitOfWorkFactory= unitOfWorkFactory;
    } 

    public void SaveLetterInFolder (Letter letter)
    {
       using (var unitOfWork = unitOfWorkFactory()
       {
           unitOfWork.lettersRepository.Add(letter);
           unitOfWork.Commit();
       }
    }   

    public void RemoveLetterFromFolder (int letterId)
    {
       using (var unitOfWork = unitOfWorkFactory()
       {
           var removed = new OutgoingLetter {Id = letterId}
           unitOfWork.lettersRepository.Attach(removed);
           unitOfWork.lettersRepository.Remove(removed);
           unitOfWork.Commit();
       }
    } 

    public List<Letter> GetAllLetters (int folderId)
    {
       using (var unitOfWork = unitOfWorkFactory()
       {
           return unitOfWork.lettersRepository.Find(folderId).Letters;
       }
    }
}

Registrations via dependency injection (Autofac):

UnitOfWork is registered as perDependency

DbContext is registered as perLifetimeScope

Repositories are registered as perLifetimeScope

share|improve this question
    
I have rolled back the last edit. Please see what you may and may not do after receiving answers. – Mat's Mug♦ Feb 20 at 17:00
up vote 1 down vote accepted

I am not familiar with Autofac (I am a Ninject fan), but I expect similar concepts and implementations, so I have a few ideas:

1) data models serialization

I see [DataMember] attributes in your data models, which leads to serialization (WCF?). As far as I know, directly serializing EF data models is not a good idea because it might lead to circular references caused by navigation properties (which crashes the serialization).

One way is to define some service models that are obtained from data models. Mapping can be done quite easy using a auto mapping library such as AutoMapper.

2) Repository implementation can be extended with some useful functions

    // gets all entities as a queryable allowing further selection filtering
    public IQueryable<T> All
    {
        get { return _context.Set<T>().AsQueryable(); }
    }

    // get all entities, but non-tracked. Particularly useful for large collection when entities are not changed
    public IQueryable<T> AllNoTracking
    {
        get { return _context.Set<T>().AsNoTracking(); }
    }

Having the methods above renders Get() almost useless. Even if it kept, it can be safely implemented in the Repository<T> class, as it is expected to look almost the same for all repositories.

Also, I would not force Includes in the repository itself. Maybe the service only wants some ids and not the whole data, so you should leave it the change to include or not data as needed.

I would remove SaveChanges from the repository and allow it only in the UnitOfWork. While it might be useful there, in most of the times you must be certain that persistence is transactional, so repositories cannot persist data in their own transactions.

3) DBContet injection in the repository

Since you are using DI, even the context can be injected (i.e. MyDbContext : IMyDbContext).

4) Unit of work can be extended with some useful functions, as well. Also, database context can be injected.

public IRepository<T> GetRepository<T>()
    where T: class
{
    Type thisType = this.GetType();
    foreach (var prop in thisType.GetProperties())
    {
        var propType = prop.PropertyType;

        if (!typeof(IRepository).IsAssignableFrom(propType))
            continue;

        var repoType = propType.GenericTypeArguments[0];
        if (repoType == typeof(T))
            return (IRepository<T>) prop.GetValue(this);
    }

    throw new ArgumentException(String.Format("No repository of type {0} found", typeof(T).FullName));
}

// or CommitEx to have your name
public bool SaveChangesEx()
{
    return _context.SaveChangesEx();
}

where SaveChangesEx might look like:

public bool SaveChangesEx()
{
    try
    {
        SaveChanges();
        return true;
    }
    catch (DbEntityValidationException exc)
    {
        // just to ease debugging
        foreach (var error in exc.EntityValidationErrors)
        {
            foreach (var errorMsg in error.ValidationErrors)
            {
                logger.LogEx(LogLevel.Error, "Error trying to save EF changes - " + errorMsg.ErrorMessage);
            }
        }

        return false;
        throw;
    }
}

This last function is particularly useful to debug some EF persistence errors which are quite hidden.

5) Base service (optional)

Since almost all your services use unit of work, it would make sense to spare unit of work factory and assignment, by having them in a BaseService.

public class BaseService 
{
    protected Func<Owned<Folder>> _unitOfWorkFactory;

    public DataService(Func<Owned<IUnitOfWork>> unitOfWorkFactory)
    {
        _unitOfWorkFactory= unitOfWorkFactory;
    } 
}

public class DataService : BaseService, IDataService
{
    public DataService(Func<Owned<IUnitOfWork>> unitOfWorkFactory) : base(unitOfWorkFactory)
    {
    }

    // other code comes here 
}

Not the greatest benefit in terms of typing, but other cross-service functionality might get in (I usually have a ISecurityService also in the BaseService).

share|improve this answer
    
Very useful! Understood your save changes implementation and I will take care of it. Next, creating a BaseService, will do. Repositories no longer contain SaveChanges or Dispose. NoTracking will be added. Points I didn't understand: - DbContext injection - what did you mean? I do inject it into all repositories and into the unit of work to manage the disposal and save changes. - The method you added in Irepository / UnitOfWork - what's the purpose of it? Lastly, do you happen to know a way to have methods with Include in the generic repository? I want to add that (First/All) with includes – S. Peter Feb 20 at 16:20
    
Plus, if I get a single entity that I know I won't change, is there anything simillar to AsNoTracking for single entity? Should I detach it before returning? – S. Peter Feb 20 at 16:25
    
The GetRepository method ia not working for me.The IsAssignableFrom always return false. Any idea why? – S. Peter Feb 23 at 11:53
    
Yes, there is something that is missing from my code - each generic repository inherits an empty interface: public interface IRepository<TEntity> : IRepository where TEntity : class This is done to simplify this check, but I think in can be improved to not require IRepository. – Alexei Feb 23 at 19:47
    
I think you should get rid of 'throw exc;' as the code is unreachable? Additionally it is bad practice to rethrow an exception using the exception variable, as it hides the original stack trace. – Wiebe Tijsma Aug 7 at 15:44

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.