How to simplify repeating if-then-assign construction?

Solution 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.

Solution 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.

Solution 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"

}

Solution 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.

How to simplify repeating if-then-assign construction?

Solution 1:

Solution 2:

Solution 3:

Solution 4:

Use T4 for Metaprogramming

Advantages:

Disadvantages:

Notes:

Related

Recent Posts