31
\$\begingroup\$

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 ...
}
\$\endgroup\$
  • \$\begingroup\$ So this is assuming the T entity is not already an object obtained directly from the dbcontext already? \$\endgroup\$ – dreza Dec 13 '13 at 19:14
  • \$\begingroup\$ 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. \$\endgroup\$ – Narayana Dec 13 '13 at 19:20
  • \$\begingroup\$ 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 \$\endgroup\$ – dreza Dec 13 '13 at 19:22
  • 4
    \$\begingroup\$ 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? \$\endgroup\$ – dreza Dec 13 '13 at 19:40
  • 1
    \$\begingroup\$ Just an update, I've totally moved away from EF and am completely on Dapper now. \$\endgroup\$ – Narayana Dec 25 '16 at 5:32
25
\$\begingroup\$

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.

\$\endgroup\$
  • 13
    \$\begingroup\$ 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. \$\endgroup\$ – Narayana Dec 13 '13 at 14:09
  • 1
    \$\begingroup\$ @Narayana because a large part of the C# community (note: NOT C# itself) penalizes anyone who dares to think in terms of performance. That's a biiiig no no. Yeah, you're right, I'm a bit jaded about this. \$\endgroup\$ – Nicholas Petersen Apr 4 '16 at 18:29
  • 1
    \$\begingroup\$ And what if after all the checks it turns out that no updates are necessary at all and we can save the whole round-trip to the database. Not to mention potential caches invalidation and as @Narayana mentions rebuilding the indices. \$\endgroup\$ – Steves Apr 16 '17 at 17:39
  • 1
    \$\begingroup\$ To extend on the idea that updating fields in the DB without changes, consider (albeit poorly written) triggers that check IF UPDATE(FieldName) and then execute logic. If EF includes the unchanged field in the update statement, it will check true in the IF UPDATE(FieldName) check and likely perform unnecessary logic. \$\endgroup\$ – Chris Porter Apr 19 '17 at 20:46
  • \$\begingroup\$ This is such a poor answer.. what if you are developing a mobile application and you have a wide table. Just send the entire DTO back, all 45 columns and update the indexes? As mentioned above, what if the table is heavily indexed? Now we should just cause fragmentation in 10 indexes as well? Can't believe this got 24 up votes \$\endgroup\$ – Tom DeMille Aug 6 at 13:48
6
\$\begingroup\$

If you are worried about the update statement that it might slow the query down, I would suggest a slightly different approach. You can modify your code to pass in the modified property list. So that in scenarios when you know which property is updated you have an option to make it even quicker and also when no property is supplied, then you can update all -

public virtual void Update(T entity, params Expression<Func<T, object>>[] updatedProperties)
{
    //dbEntityEntry.State = EntityState.Modified; --- I cannot do this.

    //Ensure only modified fields are updated.
    var dbEntityEntry = DbContext.Entry(entity);
    if (updatedProperties.Any())
    {
        //update explicitly mentioned properties
        foreach (var property in updatedProperties)
        {
            dbEntityEntry.Property(property).IsModified = true;
        }
    }else{
        //no items mentioned, so find out the updated entries
        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;
        }
    }
}

This approach gives you the freedom of doing the both of the below -

_repository.Update(obj);

and also

_repository.Update(obj, o => o.Name, o => o.Description);

Also notice, I am taking params. So you can chain as much properties as you like.

\$\endgroup\$
  • \$\begingroup\$ I appreciate this idea because it's very helpful, but I'm struggling to understand how to implement the "else" clause that finds the updated entries. Every time my OriginalValues match my CurrentValues for every property. How does it know the original values? \$\endgroup\$ – Josh Feb 6 '17 at 20:25
2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – Narayana Feb 18 '14 at 11:36
  • 1
    \$\begingroup\$ 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. \$\endgroup\$ – sdmcnitt Feb 20 '14 at 20:32
  • \$\begingroup\$ @sdmcnitt then this should probably be a comment... \$\endgroup\$ – Vogel612 Feb 27 '14 at 18:05

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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