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.

I'm working on a website on ASP.NET MVC4 and EF5. I want to ensure that only modified values are updated in the database. I'm using a unit of work pattern and repositories for data work. Here's the code for a generic repository. I've two questions, and need a code review for the following code.

  1. Is the code to update only modified fields good? It works, but I don't know if this is the best way. I especially don't like that I have to do this loop through the properties myself (can't EF do this?). I also don't like the cast to object and the comparison.

  2. How are my Add and Update methods? I'm planning the clients (MVC/API controllers) to use a service (passing in a unit of work) to get the work done. They will not directly use the repository. The repository will not have a Save() method, only the UnitOfWork will have one. I can ensure within the service that I will use the same context to update/add entities. Given this, are there any scenarios where my Add/Update code is going to fail?

public class EFRepository<T> : IRepository<T> where T : class
{
    protected DbContext DbContext { get; set; }
    protected DbSet<T> DbSet { get; set; }

    public EFRepository(DbContext dbContext)
    {
        if (dbContext == null)
            throw new ArgumentNullException("dbContext");
        DbContext = dbContext;
        DbSet = DbContext.Set<T>();
    }

    public virtual void Add(T entity)
    {
        DbContext.Entry(entity).State = EntityState.Added;
    }

    public virtual void Update(T entity)
    {
        //dbEntityEntry.State = EntityState.Modified; --- I cannot do this.

        //Ensure only modified fields are updated.
        var dbEntityEntry = DbContext.Entry(entity);
        foreach (var property in dbEntityEntry.OriginalValues.PropertyNames)
        {
            var original = dbEntityEntry.OriginalValues.GetValue<object>(property);
            var current = dbEntityEntry.CurrentValues.GetValue<object>(property);
            if (original != null && !original.Equals(current))
                dbEntityEntry.Property(property).IsModified = true;
        }
    }

    //... Other methods ...
}
share|improve this question
    
So this is assuming the T entity is not already an object obtained directly from the dbcontext already? –  dreza Dec 13 '13 at 19:14
    
yes, T is not something like dbconext.Persons.First(). T is a domain class auto-generated by Ef. an instance of this type is created, typically from view model. –  Narayana Dec 13 '13 at 19:20
    
Is there any reason why you wouldn't obtain the entity direct and manipulate that only? then you don't have to worry about calling any update method. Perhaps you could use something like AutoMapper to map from your viewmodel to model? Just thoughts –  dreza Dec 13 '13 at 19:22
    
my bad, I do call a FindById() before the update, as I get only the Id from the view. then after I map the VM to the object, I call this Update(). so it does come from the same context after all. Oh, do you mean to say I don't even have to set entitystate?? –  Narayana Dec 13 '13 at 19:34
2  
If you are obtaining the object from your context, then by modifynig those properties and then calling SaveChanges on your context it will perform an update on that model already. EF handles this for you. Is that your understanding? –  dreza Dec 13 '13 at 19:40

2 Answers 2

Why bother checking if the value/property has been changed? You are just adding additional complexity for no reason. Sure, it might be slightly! faster to only update modified values, but in return you are adding an overhead to keep track of the modified values, which will diminish whatever performance boost you got.

Just invoke Update on the entire object... All values that has not been changed will remain the same and the values that has been modified will, well, update.

If it's speed you're looking for you are most likely looking at the wrong place to squeeze ms from.

share|improve this answer
1  
I'm thinking from a DB perspective. If I update all the fields in a row, ALL my indexes in that table are going to get updated. Okay, most likely I wont have this logic for all entities, but in certain heavy tables, I need to have this option. –  Narayana Dec 13 '13 at 14:09

I am just learning EF but I would worry about concurrent users with your update.

When you call SaveChanges it would be best if EF will only update the fields that are changed to minimize the chance for collisions or overwriting the changes of a concurrent user.

share|improve this answer
    
Hi, I ended up doing everything within a context, so EF takes care of it. I do not have disconnected scenarios now, meaning, I don't explicitly set modified or added states. I let EF take care of those. –  Narayana Feb 18 at 11:36
    
So if you make use of the change tracking for the entities I think EF will generate an update for only those columns that are dirty. But I know that it can also be circumvented or overridden (such as attach after setting field values, etc) so just wanted to make sure everyone's code watches out for this. –  sdmcnitt Feb 20 at 20:32
    
@sdmcnitt then this should probably be a comment... –  Vogel612 Feb 27 at 18:05

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.