11
\$\begingroup\$

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.

\$\endgroup\$
  • 2
    \$\begingroup\$ What a name :) Reminds me of Doofenschmirts' "inator"s ( Phineas & Ferb ) to do something evil to the three states :) \$\endgroup\$ – Oguz Ozgul Dec 9 '15 at 19:33
5
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Why did you choose LINQ expressions to implement this? I am just curious, because it seems to me like LINQ is a trending topic for a while, and actually we always have better (in terms of performance) solutions which require a little bit more effort and experience. \$\endgroup\$ – Oguz Ozgul Dec 10 '15 at 11:44
  • \$\begingroup\$ @OguzOzgul Thanks for that great answer. I used LINQ expressions because well... I knew it to be faster than using reflection. But I didn't really know other viable alternatives. I'd really appreciate if you could share links that explains your solution! :) \$\endgroup\$ – IEatBagels Dec 10 '15 at 14:12
  • \$\begingroup\$ Yesi of course. It is CodeDom and you actually generate and compile source code with it on runtime: \$\endgroup\$ – Oguz Ozgul Dec 10 '15 at 15:22
  • \$\begingroup\$ Just a suggestion, you may want to look at Rosalyn, which allows you to use strings which are converted to code, which can be run at run-time. I personally never liked Expressions as I found them too tedious. \$\endgroup\$ – John Peters Dec 11 '15 at 4:42
  • \$\begingroup\$ @johnpeters Roslyn \$\endgroup\$ – Oguz Ozgul Dec 11 '15 at 5:29
6
\$\begingroup\$

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);
     }  
\$\endgroup\$
  • \$\begingroup\$ Good catch about CreateAndAdd. I so wanted to return the created expression that I forgot I didn't really need to add it. And thanks for the name, I guess mapper makes much more sense. :p \$\endgroup\$ – IEatBagels Dec 9 '15 at 16:02
1
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ I am going to benchmark a reflection only solution as well, but no other solutions can ever beat the CodeDom approach since it is pure compiled code, in the CodeDom case, you are invoking a method directly, not through delegates nor you are iterating dictionaries to find the cached compiled expression \$\endgroup\$ – Oguz Ozgul Dec 11 '15 at 23:01
  • \$\begingroup\$ Linq compiled expressions will also beat reflection I think, reflection cannot be as fast as to execute 30 million (3 property assignment x 10 million iterations) even if you cache the property info (The benchmark metrics will say the last word of course) \$\endgroup\$ – Oguz Ozgul Dec 11 '15 at 23:03
  • \$\begingroup\$ @OguzOzgul you might be right about the expression being faster than reflection once it's compiled. That said 1) there's still the overhead of compiling up front and 2) the expression approach is way more complicated. The same could be said for the CodeDom approach--is the extra complexity really necessary? \$\endgroup\$ – moarboilerplate Dec 11 '15 at 23:15
  • \$\begingroup\$ It depends on what kind of project that is, and what kind of load will it face. Apart from that, in my opinion, everyone should keep track of the performance of their code. This is something you are going to need when you start selling your software to enterprises with millions of customers. Consider the difference of the benchmark in my answer. The original code, proportionally, requires 6 cpu for a task which is accomplished by CodeDom by only one. \$\endgroup\$ – Oguz Ozgul Dec 12 '15 at 0:39

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.