How to simplify repeating if-then-assign construction?

Question

I have the following method:

protected override bool ModifyExistingEntity(Product entity, ProductModel item)
{
    bool isModified = false;

    if (entity.Title != item.Title)
    {
        isModified = true;
        entity.Title = item.Title;
    }

    if (entity.ServerId != item.Id)
    {
        isModified = true;
        entity.ServerId = item.Id;
    }

    return isModified;
}

I wonder if you could suggest a better way to implement the method.

The problem is obvious: 5 lines of almost copy-pasted code per property is too much. May be there's a solution using Func-s / Expression-s out of my vision.

Answer 1

You have a case of temporal coupling there, i.e. you're mixing the check whether the entity has changed with the assignments. If you separate the two, your code becomes much cleaner:

protected override bool ModifyExistingEntity(Product entity, ProductModel item)
{
    bool isModified = this.IsEntityModified(entity, item);

    if (isModified)
    {
        this.UpdateEntity(entity, item);
    }

    return isModified;
}

private bool IsEntityModified(Product entity, ProductModel item)
{
    return entity.Title != item.Title || entity.ServerId != item.ServerId;
}

private void UpdateEntity(Product entity, ProductModel item)
{
    entity.Title = item.Title;
    entity.ServerId = item.Id;
}

Doing any smart and funky stuff (TM) with Func<> or anything like that, doesn't seem helpful in this case as it wouldn't transport your intention as clearly.

Answer 2

Something like this should work

protected bool ModifyExistingEntity(Person entity, ProductModel item)
{
    bool isModified = CompareAndModify(() => entity.Title = item.Title, () => entity.Title != item.Title);
    isModified |= CompareAndModify(() => entity.ServerId = item.Id, () => entity.ServerId != item.Id);

    return isModified;
}

private bool CompareAndModify(Action setter, Func<bool> comparator)
{
    if (comparator())
    {
        setter();
        return true;
    }
    return false;
}

Not sure if this is readable. It is subjective.

Answer 3

I think an extension of this answer might work for you:

public static bool SetIfModified<CLeft, T>(Expression<Func<CLeft, T>> exprLeft, CLeft leftType, T rightValue)
{
    var getterLeft = exprLeft.Compile();

    if (EqualityComparer<T>.Default.Equals(getterLeft(leftType), rightValue))
    {
        var newValueLeft = Expression.Parameter(exprLeft.Body.Type);
        var assignLeft = Expression.Lambda<Action<CLeft, T>>(Expression.Assign(exprLeft.Body, newValueLeft), exprLeft.Parameters[0], newValueLeft);

        var setterLeft = assignLeft.Compile();

        setterLeft(leftType, rightValue);
        return true;
    }
    else
    {
        return false;
    }
}

It takes an expression to check the value. It compiles and executes it dynamically.

Use it like this:

public class Product { public string Title { get; set; } }
public class ProductModel { public string Title { get; set; } }

static void Main(string[] args)
{
    Product lc = new Product();
    ProductModel rc = new ProductModel();
    rc.Title = "abc";
    bool modified = SetIfModified(l => l.Title, lc, r.Title);

    // modified is true
    // lc.Title is "abc"

}

Answer 4

Use T4 for Metaprogramming

Another approach - very often when we have duplicated code that is actually simple and probably very quick. In this case, each duplicated if block is not the same - it holds a little knowledge - the mapping from one property to another.

It is annoying to write and maintain the duplicated blocks.
One way to avoid writing useful repetitive code is to automatically generate it.

With my solution, the mapping is straightforward:

var mappings = new []{
    new Mapper("ProductModel", "Product")
    { 
        "Title",               // ProductModel.Title goes to Product.Title
        {"Id", "ServiceId"},   // ProductModel.Id goes to Product.ServiceId
    },
};

Here is a t4 Text Template (build-in feature to Visual Studio):

<#@ template debug="false" hostspecific="false" language="C#" #>
<#@ assembly name="System.Core" #>
<#@ import namespace="System.Collections.Generic" #>
<#@ output extension=".cs" #>
<#
    // Consider including the namespace in the class names.
    // You only need to change the mappings.
    var product = new Mapper("Product", "ProductEntity") { "Name", {"Id", "ServiceId"} };
    var person = new Mapper("Person", "DbPerson") { "Employee", {"Name", "FullName"}, {"Addredd", "HomeAddress"} };

    var mappings = new [] {product, person};
#>
// !!!
// !!!  Do not modify this file, it is automatically generated. Change the .tt file instead.     !!!
// !!!
namespace Your.Mapper
{
    partial class Mapper
    {
        <# foreach(var mapping in mappings) { 
        #>/// <summary>
        /// Set <paramref name="target"/> properties by copying them from <paramref name="source"/>.
        /// </summary>
        /// <remarks>Mapping:<br/>
        <#foreach(var property in mapping){
        #>/// <see cref="<#=mapping.SourceType#>.<#=property.SourceProperty#>"/> → <see cref="<#=mapping.TargetType#>.<#=property.TargetProperty#>"/> <br/>
        <#}
        #>/// </remarks>
        /// <returns><c>true</c> if any property was changed, <c>false</c> if all properties were the same.</returns>
        public bool ModifyExistingEntity(<#=mapping.SourceType#> source, <#=mapping.TargetType#> target)
        {
            bool dirty = false;
            <# foreach(var property in mapping) {
            #>if (target.<#=property.TargetProperty#> != source.<#=property.SourceProperty#>)
            {
                dirty = true;
                target.<#=property.TargetProperty#> = source.<#=property.SourceProperty#>;
            }           
            <#}
            #>return dirty;
        }
        <#
         } 
        #>

    }
}

<#+
class Mapper : IEnumerable<PropertyMapper>
{
    private readonly List<PropertyMapper> _properties;

    public Mapper(string sourceType, string targetType)
    {
        SourceType = sourceType;
        TargetType = targetType;
        _properties = new List<PropertyMapper>();
    }

    public string SourceType { get; set; }
    public string TargetType { get; set; }

    public void Add(string fieldName)
    {
        _properties.Add(new PropertyMapper {SourceProperty = fieldName, TargetProperty = fieldName});
    }

    public void Add(string sourceProperty, string targetProperty)
    {
        _properties.Add(new PropertyMapper { SourceProperty = sourceProperty, TargetProperty = targetProperty });
    }

    IEnumerator<PropertyMapper> IEnumerable<PropertyMapper>.GetEnumerator() { return _properties.GetEnumerator(); }
    System.Collections.IEnumerator System.Collections.IEnumerable.GetEnumerator() { return _properties.GetEnumerator(); }
}

class PropertyMapper
{
    public string SourceProperty { get; set; }
    public string TargetProperty { get; set; }
}
#>

This template generates the following code: https://gist.github.com/kobi/d52dd1ff27541acaae10

Advantages:

• Heavy lifting is done at compile time (actually once before compile time) - the generated code is quick.
• Generated code is documented.
• Easy to maintain - you can change all mappers in a single point.
• Generated methods are documented.
• No copy-paste bugs.
• This is pretty fun.

Disadvantages:

• Use of strings to get property names. Keep in mind - this isn't production code, it is just used to generate code. It is possible to use the real types and Expression trees (an example below).
• Static analysis will probably miss the usage in the template (even if we use Expressions, not all tools look into tt files).
• Many people don't know what is going on.
• If you are using Expressions, it is tricky to reference your types.

Notes:

• I've named the arguments source and target, and changed their order so source is always first.

---

There has been some concern that I'm using strings instead of the real properties. Although this is a minor problem in this case (the output is compiled), here is an addition that works with your real objects.

At the top, add this (3rd one should be your namespace):

<#@ assembly name="$(TargetPath)" #>
<#@ import namespace="System.Linq.Expressions" #>
<#@ import namespace="ConsoleApplicationT4So29913514" #>  

At the bottom, add:

class Mapper<TSource, TTarget> : Mapper
{
    public Mapper()
        : base(typeof(TSource).FullName, typeof(TTarget).FullName)
    {

    }

    private static string GetExpressionMemberAccess(LambdaExpression getProperty)
    {
        var member = (MemberExpression)getProperty.Body;
        //var lambdaParameterName = (ParameterExpression)member.Expression;
        var lambdaParameterName = getProperty.Parameters[0]; // `x` in `x => x.PropertyName`
        var labmdaBody = member.ToString();
        //will not work with indexer.
        return labmdaBody.Substring(lambdaParameterName.Name.Length + 1); //+1 to remove the `.`, get "PropertyName"
    }

    public void Add<TProperty>(Expression<Func<TSource, TProperty>> getSourceProperty, Expression<Func<TTarget, TProperty>> getTargetProperty)
    {
        Add(GetExpressionMemberAccess(getSourceProperty), GetExpressionMemberAccess(getTargetProperty));
    }

    /// <summary>
    /// The doesn't really make sense, but we assume we have <c>source=>source.Property</c>, <c>target=>target.Property</c>
    /// </summary>
    public void Add<TProperty>(Expression<Func<TSource, TProperty>> getProperty)
    {
        Add(GetExpressionMemberAccess(getProperty));
    }
}

Usage:

var mappings = new Mapper[] {
    new Mapper<Student,StudentRecord>
    {
        {s=>s.Title, t=>t.EntityTitle},
        {s=>s.StudentId, t=>t.Id},
        s=>s.Name,
        s=>s.LuckyNumber,
    },
    new Mapper<Car,RaceCar>
    {
        c=>c.Color,
        c=>c.Driver,
        {c=>c.Driver.Length, r=>r.DriverNameDisplayWidth},
    },
};

The whole file should look like this: https://gist.github.com/kobi/6423eaa13cca238447a8
Output still looks the same: https://gist.github.com/kobi/3508e9f5522a13e1b66b

Notes:

• Expressions are only used to get the property name as a string, we are not compiling them or running them.
• In C# 6 we will have the nameof() operator, which is a nice compromise between Expressions and no-magic-strings.

Answer 5

There is no magic wand to simplify this.

You could have the entity itself provide an IsModified property, which is then set by the property setters, such as:

public string Title {
   get { return _title; }
   set {
         if (value != _title)
         {
             _title = value;
             IsModified = true;
         }
    }
}

If that is too much work, your solution is fine.

Answer 6

If you want to make it readable, you can create a class for this purpose with really simple usage avoiding repetitive code :

protected override bool ModifyExistingEntity(Product entity, ProductModel item)
{
    return new Modifier<Product>(entity)
               .SetIfNeeded(e => e.Title, item.Title);
               .SetIfNeeded(e => e.ServerId, item.Id);
               .EntityWasModified;
}

Implementation:

I took some code from Patrick Hofman to generate a setter from the getter expression.

public class Modifier<TEntity>
{    
    public Modifier(TEntity entity)
    {
        Entity = entity;
    }

    public TEntity Entity { get; private set; }

    public bool EntityWasModified { get; private set; }

    public Modifier<TEntity> SetIfNeeded<TProperty>(Expression<Func<TEntity, TProperty>> entityPropertyGetter, TProperty modelValue)
    {
        var getter = entityPropertyGetter.Compile();
        var setter = GetSetterExpression(entityPropertyGetter).Compile();

        if (!object.Equals(getter(Entity), modelValue))
        {
            setter(Entity, modelValue);
            EntityWasModified = true;
        }
        return this;
    }

    private static Expression<Action<TEntity, TProperty>> GetSetterExpression(Expression<Func<TEntity, TProperty>> getterExpression)
    {
        var newValue = Expression.Parameter(getterExpression.Body.Type);

        return Expression.Lambda<Action<TEntity, TProperty>>(
            Expression.Assign(getterExpression.Body, newValue),
            getterExpression.Parameters[0], newValue);
    }
}

You may want to cache the result of .Compile to improve performance.

Answer 7

Check out my own (almost as weird as the others) solution

[TestMethod]
public void DifferentTitleAndId_ExpectModified()
{
    var entity = new Product
        {
            Id = 0,
            ServerId = 0,
            Title = "entity title"
        };

    var model = new ProductModel
        {
            Id = 1,
            Title = "model title"
        };

    bool isModified = ModifyExistingEntity(entity, model);

    Assert.IsTrue(isModified);
}

protected bool ModifyExistingEntity(Product entity, ProductModel model)
{
    return
        IsModified(entity.Title, model.Title, x => entity.Title = x) |
        IsModified(entity.ServerId, model.Id, x => entity.ServerId = x);
}

protected bool IsModified<T>(T value1, T value2, Action<T> setter)
{
    return IsModified(() => value1, () => value2, () => setter(value2));
}

protected bool IsModified<T>(Func<T> valueGetter1, Func<T> valueGetter2, Action setter)
{
    if (!Equals(valueGetter1(), valueGetter2()))
    {
        setter();
        return true;
    }

    return false;
}

Answer 8

I have seen the most intricate answers to this question, but I think you would be best suited with a fairly straightforward, no-nonsense simple solution.

I would assume you are using some kind of Data Mapper pattern in your codebase and Product is your DAL/Domain entity and ProductModel is your app-level object. In that case I would simply have a method that compares the two (which can later be moved to a separate layer), and if they aren't equal, map.

But this raises the question, why are you worried about only updating if it's changed? It is probably acceptable to simply update every time.

Also, you should probably not be passing in an entity to a method with the expectation that it gets updated.

I would change the logic as follows:

protected bool UpdateIfChanged(Product entity, ProductModel item)
{
    var areEqual = CompareProductAndProductModel(entity, item);

    if(!areEqual)
        UpdateProduct(MapProductModelToProduct(item));

    return !areEqual;
}

internal bool CompareProductAndProductModel(Product product, ProductModel productModel)
{
    return product.Title == productModel.Title && product.ServerId == productModel.Id; //could be abstracted to an equality comparer if you were inclined
}

The biggest departure that this answer makes from other answers is that it does not modify the Product entity. Instead, it compares the Product and ProductModel, but if changes are detected, it then uses the ProductModel to create a new Product, which is then passed to another DAL method that actually does the updating work. I believe this is probably the most maintainable approach as you do not have to deal with methods that change the state of objects passed in to them (implicit coupling even if the method and caller exist in different places), which means you do not have to mentally track state changes to the entity as you step through code during debugging.

Answer 9

"Better" is subjective in this context. Since you were complaining about line count, I have a more concise way:

protected override bool ModifyExistingEntity(Product entity, ProductModel item)
{
    bool isModified = false;

    isModified |= (entity.Title!= item.Title) ? (entity.Title = item.Title) == item.Title : false;
    isModified |= (entity.ServerId != item.Id) ? (entity.ServerId = item.Id) == item.Id : false;

    return isModified;
}

Answer 10

Continueing on @bstenzel answer, shouldn't this also do the trick?

protected override bool ModifyExistingEntity(Product entity, ProductModel item)
{
    bool isEntityModified = entity.Title != item.Title || entity.ServerId != item.ServerId;
    entity.Title = item.Title;
    entity.ServerId = item.Id;
    return isEntityModified;
}

Clean and simple.

Answer 11

I think you are looking for a way to compare the content of properties of two objects. Your sample contains two properties but I expect your real code contains a lot more (as a Product entity does probably have many properties).

You should start by writing a method that compares your two objects. For your references here are some SO questions regarding the matter:

1. Properly implement comparison of two objects with different type but semantically equivalent
2. Comparing object properties in c#

You method would look like:

public static bool IsEqualTo<TSource>(this TSource sourceObj, TDestination destinationObj)
    where TSource : class
    where TDestination : class
{
    // Your comparison code goes here
}

Then you will have to write a second method to copy data between your objects. This questions can guide you through (check Marc answer):

3. How to deep copy between objects of different types in C#.NET

You method would look like:

public static bool CopyDataTo<TSource>(this TSource sourceObj, TDestination destinationObj)
    where TSource : class
    where TDestination : class
{
    // Your data copy code goes here
}

You final code will look as simple as

protected override bool ModifyExistingEntity(Product entity, ProductModel item)
{
    if (!entity.IsEqualTo(item))
    {
        item.CopyDataTo(entity);
        return true;
    }

    return false;
}

Answer 12

I'm not sure if the Product class is extensible for you, nor am I certain if you're looking for a "fancy" answer or just a simpler one...but if so, you can things around a bit and put the logic in the Product class itself; imho you end up with a fairly readable method:

protected override bool ModifyExistingEntity(Product entity, ProductModel item)
{
    entity.SetTitle(item.Title);
    entity.SetServerId(item.Id);
    return entity.WasModified();
}

The added bonus is that you neatly encapsulate the behavior into Product (as well as validation, etc.)

public partial class Product
{
    public void SetTitle(string title)
    {
       if(this.Title!=title) //and other validation, etc
       {
         this.Title = title;
         Modified();
       }
    }

    public void SetServerId(int serverId)
    {
       if(this.ServerId!=serverId)
       {
          this.ServerId=serverID;
          Modified();
        }
    }

    private bool _wasModified;

    private void Modified()
    {
        //Or implement INotifyPropertyChanged if you like
        _wasModified=true;
    }

    public bool WasModified()
    {
        return _wasModified;
    }
}

Of course if you don't need any "business logic" and this really is just an unchecked mapping operation, any one of the very clever answers here will do :)

Answer 13

Since you most likely do not want to write the code for every entity and model pair, you should rely on reflection to map the properties which are the same between the entity and model and then just compare those values with the property Infos.

Edit: Added copying of the modified value and locking for multiple threads.

class Program
{
    static void Main(string[] args)
    {
        if (ModifyExistingEntity(new Product { Name = "bar" }, new ProductModel { Name = "test" }))
            Console.WriteLine("Product modified");

        if (ModifyExistingEntity(new Customer { Number = 1001 }, new CustomerModel { Number = 1002 }))
            Console.WriteLine("Customer was modified");

        if (!ModifyExistingEntity(new Customer { Number = 1001 }, new CustomerModel { Number = 1001 }))
            Console.WriteLine("Customer was not modified");

        Console.ReadKey();
    }

    protected static bool ModifyExistingEntity<TEntity, TModel>(TEntity entity, TModel model)
    {
        var isModified = false;

        GetProperties(entity, model).ForEach(
                propertyInfo =>
                {
                    var item2Value = propertyInfo.Item2.GetValue(model, null);
                    if (Equals(propertyInfo.Item1.GetValue(entity, null), item2Value)) 
                        return;

                    propertyInfo.Item1.SetValue(entity, item2Value);
                    isModified = true;
                });

        return isModified;
    }

    private static readonly object MappingLock = new object();
    private static readonly Dictionary<Tuple<Type, Type>, List<Tuple<PropertyInfo, PropertyInfo>>> Mapping =
            new Dictionary<Tuple<Type, Type>, List<Tuple<PropertyInfo, PropertyInfo>>>();

    protected static List<Tuple<PropertyInfo, PropertyInfo>> GetProperties<TEntity, TModel>(TEntity entity, TModel model)
    {
        lock (MappingLock)
        {
            var key = new Tuple<Type, Type>(typeof (TEntity), typeof (TModel));
            if (Mapping.ContainsKey(key))
            {
                return Mapping[key];
            }

            var modelProperties = typeof (TModel).GetProperties();

            var newMapping = (from propertyInfo in typeof (TEntity).GetProperties()
                let modelPropertyInfo = modelProperties.SingleOrDefault(mp => mp.Name == propertyInfo.Name)
                select new Tuple<PropertyInfo, PropertyInfo>(propertyInfo, modelPropertyInfo))
                .ToList();

            Mapping.Add(key, newMapping);

            return newMapping;
        }
    }
}

Answer 14

If you just want shorter code-lines you can compact it to this C-Style assignments:

ModifyExistingEntity( Product entity, ProductModel item )
{
    bool isModified = false;

    isModified |= ( entity.Title != item.Title )
        && retTrue( entity.Title =  item.Title );

    isModified |= ( entity.ServerId != item.Id )
        && retTrue( entity.ServerId =  item.Id );

    return isModified;
}

static bool RetTrue<T>(T dummy) { return true; } // Helper method.

Answer 15

This is a situation where macro use is appropriate:

#define CheckAndAssign(dst,src) (dst != src && (dst = src, true))

return (  CheckAndAssign (entity.Title, item.Title)
        | CheckAndAssign (entity.ServerId, item.Id));

#undef CheckAndAssign

Well, as long as the language is C, C++, Objective-C or anything else with macros. I'd hope that in C++ you can turn it into a template somehow.