Return to Revisions

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);
     }