Dynamic, generic property "assignator"

Question

Point one, I know "Assignator" isn't a real word. I couldn't find a better fit so right now that word is added to my Visual Studio's dictionary!

As a part of my (future) MVVM framework, I needed a class to assign values to a property of an object. As it's a quite generic framework, the class itself needed to be generic. I had the choice to use reflection (*booo*) or to give it a try and Expression compilation at runtime (*cheers*)! Guess what, I went with expression compilation.

public interface IPropertyAssignator<T> where T : class
{
    void Assign(T target, string propertyName, object value);
}

/// <summary>
/// Dynamically assigns values to properties of an object
/// </summary>
/// <typeparam name="T">Assignation's target type</typeparam>
/// <remarks>This class is not thread safe.</remarks>
public class DynamicPropertyAssignator<T> : IPropertyAssignator<T> where T : class
{
    private readonly bool _lazyPopulate;
    private readonly Dictionary<string, Action<T, object>> _expressionMap;

    /// <summary>
    /// Creates an instance of DynamicPropertyAssignator.
    /// </summary>
    /// <param name="lazyPopulate">If true, assignation expressions will be generated only when needed.</param>
    public DynamicPropertyAssignator(bool lazyPopulate = true)
    {
        _lazyPopulate = lazyPopulate;
        _expressionMap = new Dictionary<string, Action<T, object>>();

        if (!lazyPopulate)
        {
            var properties = typeof(T).GetProperties(BindingFlags.Instance | BindingFlags.Public);

            foreach (var property in properties)
            {
                CreateAndAddAssignExpression(property.Name, property.PropertyType);
            }
        }
    }

    /// <summary>
    /// Assigns <paramref name="value"/> to <paramref name="propertyName"/> of <paramref name="target"/>
    /// </summary>
    /// <param name="target">Target to be modified</param>
    /// /// <param name="propertyName">Property of the target to modify</param>
    /// <param name="value">Value to assign the target</param>
    /// <exception cref="ArgumentException">If the assignation arguments are of incoherent types.</exception>
    public void Assign(T target, string propertyName, object value)
    {
        if (target == null) throw new ArgumentNullException(nameof(target));
        if (propertyName == null) throw new ArgumentNullException(nameof(propertyName));
        if (value == null) throw new ArgumentNullException(nameof(value));

        Action<T, object> assignExpression = null;

        if(_lazyPopulate && !_expressionMap.TryGetValue(propertyName, out assignExpression))
        {
            if (PropertiesMatch(value.GetType(), propertyName))
            {
                assignExpression = CreateAndAddAssignExpression(propertyName, value.GetType());
            }
            else
            {
                //Add null to show we checked that this property doesn't exist in T, so we skip the checks the next time.
                _expressionMap.Add(propertyName, null);
            }
        }

        try
        {
            ApplyExpression(assignExpression, target, value);
        }
        catch (InvalidCastException ice)
        {
            throw new ArgumentException($"{typeof(T)}.{propertyName} isn't assignable to {value.GetType()}", ice);
        }
    }

    /// <summary>
    /// Call of the <paramref name="expression"/> with given parameters.
    /// </summary>
    /// <param name="expression">Expression to use</param>
    /// <param name="target">Target of modification</param>
    /// <param name="value">Value to apply</param>
    protected virtual void ApplyExpression(Action<T, object> expression, T target, object value)
    {
        expression?.Invoke(target, value);
    }

    /// <summary>
    /// Dynamically creates an Action that will be used assign a value to the target's property and adds it to the map.
    /// </summary>
    /// <param name="propertyName">Name of the property to assign</param>
    /// <param name="propertyType">Type of the property to assign</param>
    /// <returns>An Assign expression compiled to an Action</returns>
    private Action<T, object> CreateAndAddAssignExpression(string propertyName, Type propertyType)
    {
        var targetParameter = Expression.Parameter(typeof(T), "target");
        var targetProperty = Expression.Property(targetParameter, propertyName);

        var valueParameter = Expression.Parameter(typeof(object),"value");
        //This is necessary because we hold a Action<T,object>.
        var convertedValue = Expression.Convert(valueParameter, propertyType);

        var assignation = Expression.Assign(targetProperty, convertedValue);

        //Final result looks like : (target, value) => target.Property = (propertyType)value
        var action = Expression.Lambda<Action<T, object>>(assignation, targetParameter, valueParameter).Compile();

        _expressionMap.Add(propertyName, action);

        return action;
    }

    /// <summary>
    /// Asserts that the property exists on <typeparamref name="T"/> and that the type fits
    /// </summary>
    /// <param name="valueType">Type of the value that would be assigned</param>
    /// <param name="propertyName">Property name of <typeparamref name="T"/> to check</param>
    /// <returns></returns>
    private static bool PropertiesMatch(Type valueType, string propertyName)
    {
        var property = typeof(T).GetProperty(propertyName);

        return (property != null && property.PropertyType.IsAssignableFrom(valueType));
    }
}

There are some goals to my review (Obviously, anything else is welcomed). I'd want to know if :

1. my code and comments are self-explanatory enough that I don't need to explain my code.
2. the expression generation is good
3. the caching method that I use for the expression seems good.

I ran benchmarks of this class versus an "assignator" that uses reflection and mine quickly becomes much faster, which is good. But for sure, if something could be made faster, that'd be great.

Answer 1

There are a few functional issues with the code as far as I can see:

First of all, the library does not work when lazyPopulate is passed false. The expressions are created in the constructor, however the following condition always evaluates to false by the first test _lazyPopulate == true and TryGetValue is never executed and the assignExpression is always null, causing failure of all assignments SILENTLY. (see section below)

if (_lazyPopulate && !_expressionMap.TryGetValue(propertyName, out assignExpression))

Silent failures

Should not you be throwing an exception at this point if the passed property name does not exist in the target type, or the value passed does not match to the property type? The way it works currently is hiding the assignment failures from the caller and the caller thinks he did set the property values.

I changed the logic of that part as follows just to fix the library to run a benchmark: (see section below)

if (_lazyPopulate)
{
    if (!_expressionMap.TryGetValue(propertyName, out assignExpression))
    {
        if (PropertiesMatch(value.GetType(), propertyName))
        {
            assignExpression = CreateAndAddAssignExpression(propertyName, value.GetType());
        }
        else
        {
            // CONSIDER:
            // throw new System.Reflection.TargetException("The property " + propertyName + " property does not exist in the target type " + typeof(T));
            //Add null to show we checked that this property doesn't exist in T, so we skip the checks the next time.
            _expressionMap.Add(propertyName, null);
        }
    }
}
else
{
    if (!_expressionMap.TryGetValue(propertyName, out assignExpression))
    {
        // CONSIDER:
        // throw new System.Reflection.TargetException("The property " + propertyName + " property does not exist in the target type " + typeof(T));
        //Add null to show we checked that this property doesn't exist in T, so we skip the checks the next time.
        _expressionMap.Add(propertyName, null);
    }
}

Read-only properties

The constructor will throw an exception when lazyPopulate is false and when the provided type (the generic argument) contains any read-only properties like

public int ThouShallNotWrite { get { return _thouShallNotWrite; } }

You can consider checking:

typeof(T).GetProperty(propertyName).SetMethod != null

And throwing an exception if it is.

Benchmark

When fixed, I tried to benchmark the class with a CodeDom memory compiled equivalent (though, no lazy population support for now). The difference of the results are astonishing:

I used the following class for the benchmark:

public class TestClass
{
    public string TestString { get; set; }
    public int TestInt32 { get; set; }
    public List<int> TestList1_Int32 { get; set; }
}

Here is the way I executed the benchmark: (Object initializations are not included and only Assign operations are invoked 10 million times)

public static void Main()
{
    Console.WriteLine("Enter to start");
    Console.ReadLine();

    IPropertyAssignator<TestClass> compiledAssignator = AssignatorFactory<TestClass>.Create();
    DynamicPropertyAssignator<TestClass> linqExpressionAssignator = new DynamicPropertyAssignator<TestClass>(false);

    Stopwatch sw = new Stopwatch();
    int iterationCount = 10000000;

    TestClass testClass1 = new TestClass();
    List<int> intList = new List<int>();

    sw.Reset();
    sw.Start();
    for (int i = 0; i < iterationCount; i++)
    {
        linqExpressionAssignator.Assign(testClass1, "TestInt32", 5);
        linqExpressionAssignator.Assign(testClass1, "TestList1_Int32", intList);
        linqExpressionAssignator.Assign(testClass1, "TestString", "This is a test string");
    }
    sw.Stop();
    Console.WriteLine("linqExpressionAssignator " + iterationCount + "iterations ended in {0} ms", sw.Elapsed.TotalMilliseconds);

    sw.Reset();
    sw.Start();
    for (int i = 0; i < iterationCount; i++)
    {
        compiledAssignator.Assign(testClass1, "TestInt32", 5);
        compiledAssignator.Assign(testClass1, "TestList1_Int32", intList);
        compiledAssignator.Assign(testClass1, "TestString", "This is a test string");
    }
    sw.Stop();
    Console.WriteLine("compiledAssignator " + iterationCount + "iterations ended in {0} ms", sw.Elapsed.TotalMilliseconds);
}
}

Benchmark results

Enter to start

linqExpressionAssignator 10000000 iterations ended in 3071,7773 ms
compiledAssignator 10000000 iterations ended in 520,0131 ms

If you want me to share the CodeDom compiled version of the assignator, I will.

Answer 2

Class naming

I needed a class to assign values to a property of an object

this can be reworded to

I needed a class to map values to a property of an object

which results in IPropertyMapper and DynamicPropertyMapper.

---

public void Assign(T target, string propertyName, object value)

You are doing null checks at the top of the method which is very good.

Because the propertyName will be passed to the PropertiesMatch() method which could throw an AmbiguousMatchException you should either enclose this call into a try..catch or the call to GetProperty() inside the PropertiesMatch() method. See the remarks in the docu.

You should also state in the xml documentation that the propertyName parameter is used case-sensitive.

Speaking of xml documentation, you are missing a to after used

/// Dynamically creates an Action that will be used assign a value to the target's property and adds it to the map.  

---

//This is necessary because we hold a Action<T,object>.
  var convertedValue = Expression.Convert(valueParameter, propertyType);

I love this comment because it is clearly telling why you are doing this.

---

In the CreateAndAddAssignExpression() method you are adding the Action<> as a value to the Dictionary<> but you are returning the action as well. Why don't you rename the method to CreateMappingExpression(), skip the adding to the dictionary and add the result to the Dictionary() where you call it ?

For the constructor this would be

        foreach (var property in properties)
        {
            var action = CreateMappingExpression(property.Name, property.PropertyType);
            _expressionMap.Add(property.Name, action);
        }  

and in the Assign() method like so

     if(_lazyPopulate && !_expressionMap.TryGetValue(propertyName, out assignExpression))
     {
         if (PropertiesMatch(value.GetType(), propertyName))
         {
             assignExpression = CreateMappingExpression(propertyName, value.GetType());
         }

         _expressionMap.Add(propertyName, assignExpression);
     }  

You see this would remove the else part and also makes the comment //Add null to show we checked that this property doesn't exist in T, so we skip the checks the next time. redundant.

But as Oguz Ozgul pointed out in his answer you have a problem if _lazyPopulate == false in the way this condition is used. If we revert the condition to check TryGetValue() first it won't work without problems but at least without exceptions like so

     if(!_expressionMap.TryGetValue(propertyName, out assignExpression) && _lazyPopulate)
     {
         if (PropertiesMatch(value.GetType(), propertyName))
         {
             assignExpression = CreateMappingExpression(propertyName, value.GetType());
         }

         _expressionMap.Add(propertyName, assignExpression);
     }  

Answer 3

I'll say it at the risk of sounding harsh--I believe the pattern of using strings and objects as generic "value-holders" but then pushing them into "generic" methods with string/object type parameters should be avoided. Either the implementation has gotten too meta, or generics are ultimately not needed.

If the implementation relies on the inherently generic "bag" nature of types like strings and objects, then a reflection-only-based solution would be optimal simply for the reason that the desired functionality can be achieved using only strings and objects in reflection. A solution that uses strings and objects in addition to generics/type parameters is going to be more difficult to consume and support, and frankly, it's just more code.

Plus, your solution uses reflection anyway, so you've already given into the dark side.

Compilation of Expressions is expensive. IIRC, MSDN explicitly calls out that you should not compile expressions in production code.

It's possible your performance is better over reflection because your generic solution is caching the expressions. But does it perform better than a reflection-based scenario that caches PropertyInfos? I haven't tested it, but I'd assume that even if it's more expensive to call SetValue instead of invoking a compiled expression on an object, the difference is still negligible. Plus there's still the hit of compiling the expressions up front--more efficient than getting a PropertyInfo? If it's more expensive to use expressions, the lazy scenario is going to take a hit.

A method can have generic parameters, too, and multiple ones. I think this is the reason for all the object/string stuff--you need a static type, but since you don't have the type in type-parameter format, you have to rely on a "catch-all" type like string or object. At first glance, I thought the best course of action would be to consider a generic-only solution with no reliance on strings or objects. But here's what happens when you create a generics-only solution:

void Main()
{
    var testClass = new TestClass();
    testClass.Id = 2;
    PropertySetter.AssignValue(testClass, t => t.Id = 1);    
}

public class TestClass
{
    public int Id { get; set; }
}

public class PropertySetter
{
    public static void AssignValue<TTarget>(TTarget target, Action<TTarget> setValue)
    {
        setValue(target);
    }
}

It's completely redundant code; you could just replace it with an assignment statement (FWIW, I also went down the path of using a second TValue type param but that implementation was more complex and suffered from the same redundancy). So I realized that there's probably more to the story and that for some reason you need to provide your properties as strings.

I'd like to see more about why the properties are represented as strings. It's possible the problem can be solved differently, though it may very well be that the string solution is the most appropriate. If that is the case, however, I would recommend cutting down on the complexity by refactoring to a reflection-only approach.