I've written a method in c# that allows me to do create a dictionary from a passed in object and N lambda expressions that reference that objects properties and methods. It's working the way I want it and seems to perform well, but I'm looking for criticism and seeking to improve the code.

Here's an example usage:

class Person
{
    public string Name { get; set; }
    public int Age { get; set; }        

    public Dictionary<string, object> MakeDictionary()
    {
        return DictionaryThing.Data(this, x => x.Name, x => x.Age);             

        /* example return dictionary:

          Name: "Ronnie"
          Age: 29

        */
    }
}

And here's the code for DictionaryThing:

static class DictionaryThing 
{

    static ConcurrentDictionary<string, Delegate> expCache = new ConcurrentDictionary<string, Delegate>();

    public static Dictionary<string, object> Data<T>(T obj, params Expression<Func<T, dynamic>>[] expressions)
    {
        var dict = new Dictionary<string, object>();

        foreach (var exp in expressions)
        {
            string name = null;
            var body = exp.Body;    

            var unaryExp = body as UnaryExpression;
            if (unaryExp != null)
                body = unaryExp.Operand;

            var memberExp = body as MemberExpression;
            if (memberExp != null)
                name = memberExp.Member.Name;

            var methodCallExp = body as MethodCallExpression;
            if (methodCallExp != null)
                name = methodCallExp.Method.Name;

            if (name == null)
                throw new InvalidExpressionException(
                    string.Format("The expression '{0}' is invalid. You must supply an expression that references a property or a function of the type '{1}'.",
                        exp.Body, typeof(T)));

            var key = typeof(T).FullName + "." + name;          
            var func = (Func<T, dynamic>)expCache.GetOrAdd(key, k => ((LambdaExpression)exp).Compile());

            dict[name] = func(obj);
        }

        return dict;
    }

}

My concerns are:

  • Is there a better way to work with the expressions that are passed in.
  • Can the expressions parameter be better constrained by a more appropriate type?
  • Is there a better way to derive the cache key?

Ronnie,

You have a good starting point for the problem you are trying to solve.

To answer your concerns:

  1. Is there a better way to work with the expressions that are passed in.

    I think what you have done is fine. You are using Expression to let user specify methods/properties in type safe way. The only thing I would change is to use Expression<Func<T, object>>[] instead of Expression<Func<T, dynamic>>[] as dynamic will invoke the compiler at run time.

  2. Can the expressions parameter be better constrained by a more appropriate type?

    I don't think you can specify additional constraints at compile time but you can definitely add code to validate the expression at runtime.

  3. Is there a better way to derive the cache key?

    I think your cache key is fine. If you want to be more conservative, you can include Type.AssemblyQualifiedName. I would worry about it only if I was releasing to general public and not if I am using this class as part of a team.

With that said, here's the full re-factored code along with my notes. I hope it helps.

void Main()
{
    var ronnie = new Person();
    ronnie.Name = "Ronnie";
    ronnie.Address = "Greensboro, NC";

    var dictionary = ObjectToDictionaryConverter.GetDictionary(ronnie, x => x.Name, x => x.Address);

    // In following example, we are trying to pass in an expression which is not valid for our object.
    // Running this code will throw an appropriate exception like
    // Expression value(UserQuery+<>c__DisplayClass0).anotherDictionary.Keys is invalid. 
    // Expression Property/Member Type System.Collections.Generic.Dictionary`2[[System.String, mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089],[System.String, mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089]]
    //, expecting Type: UserQuery+Person

    //var anotherDictionary = new Dictionary<string, string>();
    //var dictionary = ObjectToDictionaryConverter.GetDictionary(ronnie, x => anotherDictionary.Keys);  

    //I don't think you can enforce this constraint at compile time (I am not sure about it, I would love to be proved wrong.)

    Console.WriteLine(dictionary["Name"]); //Prints Ronnie
    Console.WriteLine(dictionary["Address"]); //Prints Greensboro, NC

}

public class Person
{
    public string Name { get; set; }

    public string Address { get; set; }
}

// Why would you name a class like 'DictionaryThing'? It doesn't explain anything about what the class does. 
// Let's give it a more descriptive 'ObjectToDictionaryConverter' name. You may prefer some other name but 
// make sure that you put same thought in to naming your class as the code which goes inside the class.

// Next, the original code was doing two things
// 1. Inspecting the expression to derive a key
// 2. Create the dictionary from the input object (while also catching the expression details)

// Following SRP, let's separate it out in two classes so that each class is responsible for doing one thing.
// This also means that you can use the Expression inspection code somewhere else if you choose to.
public static class ObjectToDictionaryConverter 
{
    //Always spell out the variable name in its full. (expCache -> expressionCache)
    static ConcurrentDictionary<string, Delegate> expressionCache = new ConcurrentDictionary<string, Delegate>();

    public static Dictionary<string, object> GetDictionary<T>(T obj, params Expression<Func<T, object>>[] expressions)
    {
        //Always validate your arguments. 
        if(ReferenceEquals(obj, null))
            throw new ArgumentNullException("obj");

        if(expressions.Length == 0)
            throw new ArgumentException("You must specify at least one expression.");

        foreach (var expression in expressions)
        {
            if(expression == null)
                throw new ArgumentException("You can not specify NULL expression.");
        }

        var result = new Dictionary<string, object>();

        foreach (var expression in expressions)
        {
            //The purpose of ExpressionDetail is to inspect our expression 
            var expressionDetail = ExpressionDetail.Create(expression);

            //A lambda expression can be a valid expression referring to a property or function.
            //But for our need, we will need to compile this expression to a delegate and run on object of Type T, let's make
            //sure that expression refers to the correct type.

            //IMPORTANT: We should not put this check in ExpressionDetail class. It is not
            //the responsibility of ExpressionDetail to enforce this type constraint. 
            //This type constraint is only needed for ObjectToDictionaryConverter class. 
            if(expressionDetail.DeclaringType != typeof(T))
                throw new InvalidExpressionException("Expression " + expression.Body + " is invalid. Expression Property/Member Type " + expressionDetail.DeclaringType.FullName + ", expecting Type: " + typeof(T).FullName);      

            //expressionDetail has properties like 'Name', 'FullName', 'Delegate' 
            var func = (Func<T, object>) expressionCache.GetOrAdd(expressionDetail.FullName, expressionDetail.Delegate);        

            result[expressionDetail.Name] = func(obj);
        }

        return result;
    }
}

public class ExpressionDetail
{
    private ExpressionDetail()
    {

    }

    public MemberInfo MemberInfo { get; private set; }

    public LambdaExpression Expression { get; private set; }

    //By figuring out MemberInfo from the Expression, 
    //we can now have all these read-only properties to get expression detail.
    public string Name { get { return MemberInfo.Name; } }

    public Type DeclaringType { get { return MemberInfo.DeclaringType; } }

    public string FullName { get { return DeclaringType.FullName + "." + Name; } }

    //Depending on performance requirement, you may want to use Lazy<T> to calculate this value
    //only once. 
    public Delegate Delegate { get { return Expression.Compile(); } }

    //We are expecting a lambda expression which should either point to a method or a property access.
    //To get body, we have to handle the case of expression being UnaryExpression
    //To learn more: http://stackoverflow.com/questions/3567857/why-are-some-object-properties-unaryexpression-and-others-memberexpression
    private static Expression GetBody(LambdaExpression expression)
    {
        //We don't validate arguments here only because it's a private method.

        var unaryExpression = expression.Body as UnaryExpression;
        return unaryExpression != null ? unaryExpression.Operand : expression.Body;
    }

    //In your original method, you returned the name.
    //However, it could be even more useful to get the MemberInfo and store it.
    //Now we will have access to Name as well as the Type in which the property/method is declared.

    // There are lots of edge cases here. 
    // Refer to: http://stackoverflow.com/questions/671968/retrieving-property-name-from-lambda-expression and update this method to handle the edge cases that you care about.
    private static MemberInfo GetMemberInfo(Expression expression)
    {
        //We don't validate arguments here only because it's a private method.

        var memberExpression = expression as MemberExpression;

        if(memberExpression != null)
            return memberExpression.Member;

        var methodCallExpression = expression as MethodCallExpression;

        if(methodCallExpression != null)
            return methodCallExpression.Method;

        return null;
    }

    public static ExpressionDetail Create(LambdaExpression expression)
    {
        if(expression == null)
            throw new ArgumentNullException("expression");

        var body = GetBody(expression);

        var memberInfo = GetMemberInfo(body);

        if (memberInfo == null)
                throw new InvalidExpressionException(
                    string.Format("The expression '{0}' is invalid. You must supply an expression that references a property or a function.",
                        expression.Body));

        return new ExpressionDetail { MemberInfo = memberInfo, Expression = expression};
    }

}
  • Even AssemblyQualifiedName is not 100 % safe. You can create two distinct dynamic assemblies with the same name. (But this will be even rarer than FullName collision and I agree with you that that one should be already pretty rare, especially if you control the code that uses this method.) – svick Oct 24 '13 at 16:30
  • @svick Agreed. It looks like you are a regular here, any comments on my code? Any other improvement ideas? – SolutionYogi Oct 24 '13 at 17:07
  • Your code seems reasonable to me. One note: it's annoying when I have to scroll horizontally, especially when reading comments. (This wouldn't be a problem for your code normally, but it is when you post it to SE, which shows code in a narrow window.) – svick Oct 24 '13 at 17:16
  1. I think DictionaryThing is a really bad name for a class, you should think about a better name. And Data() is not a great name either. DictionaryThing.Data doesn't really tell you what it does.

  2. I don't see any reason to use dynamic in your code. Using object instead would work just as well and it would be less confusing, because writing dynamic implies that you're actually doing something dynamically.

  3. You could create a separate cache for each T. With that, you wouldn't need to compute key from the type name and you also wouldn't need the cast.

    To do this, you could create a nested static generic type, that would hold the cache. Something like:

    static class DictionaryThing 
    {
        private static CacheHolder<T>
        {
            public static ConcurrentDictionary<string, Func<T, object>> expressionCache = …;
        }
    
        …
    }
    
  4. You don't check that the UnaryException is actually Convert. I can't think of a sensible situation where it would be anything else, but I would check it anyway, just to be on the safe side.

  • +1 Great idea of using separate Cache for each T. It avoids possible collision. – SolutionYogi Oct 24 '13 at 16:19

Your Answer

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

Not the answer you're looking for? Browse other questions tagged or ask your own question.