Using a lambda expression to avoid using a "magic string" to specify a property

Question

I am writing a service to take a collection of objects of a particular type and output its primitive, string, and DateTime types to a string in CSV Format. I have both of the below statements working. I find the the lambda based version to be much cleaner.

Magic String Version

string csv = new ToCsvService<DateTime>(objs)
    .Exclude("Minute")
    .ChangeName("Millisecond", "Milli")
    .Format("Date", "d")
    .ToCsv();

vs. Lambda Version

string csv = new ToCsvService<DateTime>(objs)
    .Exclude(p => p.Minute)
    .ChangeName(p => p.Millisecond, "Milli")
    .Format(p => p.Date, "d")
    .ToCsv();

Per Jon Skeet's recommendation all of the lambda methods share a similar method signature

public IToCsvService<T> Exclude<TResult>(
        Expression<Func<T, TResult>> expression)

I then pass the expression.Body to FindMemberExpression. I've adapted code from the FindMemberExpression method of ExpressionProcessor.cs from the nhlambdaextensions project. My very similar version of FindMemberExpression is below:

private string FindMemberExpression(Expression expression)
{
    if (expression is MemberExpression)
    {
        MemberExpression memberExpression = (MemberExpression)expression;

        if (memberExpression.Expression.NodeType == ExpressionType.MemberAccess
            || memberExpression.Expression.NodeType == ExpressionType.Call)
        {
            if (memberExpression.Member.DeclaringType.IsGenericType
                && memberExpression.Member.DeclaringType
                .GetGenericTypeDefinition().Equals(typeof(Nullable<>)))
            {
                if ("Value".Equals(memberExpression.Member.Name))
                {
                    return FindMemberExpression(memberExpression.Expression);
                }

                return String.Format("{0}.{1}",
                    FindMemberExpression(memberExpression.Expression),
                    memberExpression.Member.Name);
            }
        }
        else
        {
            return memberExpression.Member.Name;
        }
    }

    throw new Exception("Could not determine member from "
        + expression.ToString());
}

I am testing for enough cases in FindMemberExpression? Is what I am doing overkill given my use case?

Anton

Answer 1

EDIT: The core to making this simpler is to change the signature of your methods to be generic in the result type too:

public IToCsvService<TSource> Exclude<TResult>(
    Expression<Func<TSource, TResult>> expression)

That way you won't end up with a conversion expression because no conversion will be necessary. For example, p => p.Minute will end up as an Expression<Func<DateTime, int>> automatically due to type inference.

---

It looks like overkill to me, given that at the moment all you need is a property - at least, that's all that your sample shows.

Why not start off just recognising a property, and expand it later if you need to?

EDIT: Here's a short but complete example which doesn't show any conversions:

using System;
using System.Linq.Expressions;

class Test
{
    static void Main()
    {
        Expression<Func<DateTime, int>> dt = p => p.Minute;
        Console.WriteLine(dt);
    }
}

If you change the expression type to Expression<Func<DateTime, long>> however, it does show the Convert(...) bit. I suspect you need to change the signatures of your Exclude (etc) methods.

Papeete

OT: yowser; about 4 days from 200k? – Gallicize

To just recognize a property I was just using the UnaryExpression path and a simplified version of the MemberExpression path. Is there an even cleaner mechanism than what I have outlined? – Anton

@Marc: Indeed - 3 if I'm really lucky :) – Papeete

@ahsteele: Why do you need the UnaryExpression path if you're only recognizing a property? Isn't that just MemberAccess? Do you actually need to cope with casts? – Papeete

I was wondering which would happen first; 100k C# questions or 200k Jon Skeet points ;p – Gallicize

@Jon I am probably a bit out of my depth at this point but isn't p => p.Minute a UnaryExpression? Is there a mechanism by which I can get the same utility without checking for Unary? – Anton

@Anton - Nope, that's just a PropertyExpression in .NET 4 or a MemberExpression in .NET 3.5 (wrapped up in a lambda expression in both cases, of course). Maybe it's the wrapping which is confusing things - I was unconditionally just taking the Body of the given expression arbitrarily, assuming it to be a lambda expression :) – Papeete

@Jon - The expression I end up with in my methods is {p => Convert(p.Minute)} with an expression.Body of {Convert(p.Minute)} which is a UnaryExpression. So I pull the Operand out which is p.Minute, a MemberExpression from there I can grab the Member.Name. Ultimately, from my view what I am doing is taking p => p.Minute and running it down this way ((expression.Body as UnaryExpression).Operand as MemberExpression).Member.Name. Are you saying there's a "cleaner" way of doing that or am I totally missing what you are driving at? – Anton

@ahsteele: What are the signatures of your Exclude (etc) methods? I can't see why it would use Convert unless it was trying to convert the value to a different type. – Papeete

@Jon I placed the signature in the body of the question earlier today but here it is: public IToCsvService<TSource> Exclude(Expression<Func<TSource, object>> expression). The goal here is to not always use a DateTime but have this work for any collection of objects. – Anton

@ahsteele: You're already generic in the source of the expression - you should be generic in the result too. I'll edit my answer to show this. – Papeete

@Jon thank you for taking the time to help me grok what was going on. – Anton

Answer 2

Do you have any plans on make it more flexible, or is this all that it needs to do?

Ideally you should have the simplest code that will do what you need done, so that you can decrease the number of things that can go wrong.

If you are doing this as a proof of concept, and know that later you will need lambda expressions, then it makes sense to keep them there, but, if this is the final product then the former one is easier to read, and less likely to be a cause of confusion if someone else needs to make changes to the code.

Finzer

I am all for readability but isn't the type safety worth the "cost" of the utilizing a lambda expression? – Anton

@Anton - You are outputting this to a string anyway, so, unless you are doing something with the values, besides writing to a file, type-safety doesn't by you anything. Besides, if you are concerned about type safety there are other ways besides adding the complexity of lambda expressions to get that. Keep it as simple as it needs to be is the important part here. – Finzer

agreed, but I was talking about the Type Safety for the classes that I am working against. For purposes of this example I used DateTime but if I was using a value type that I controlled and changed a property name the lambda expression would assist with that. Whereas the magic string version would leave me in the dark until I later discovered the issue. I tend towards do the simplest thing that could possibly work but am wondering if that's the "right" answer here. Am I missing something? – Anton

@ahstelle - Do you foresee any need to not use DateTime? That is the real issue. Don't design for what might be needed, someday, that leads to problems. I asked this question in the first sentence of my answer. – Finzer

I see what you are saying. This will be used for a wide variety of Value types. Assuming I get it "right" I'd like to even make the final product OSS on CodePlex or Google Code. – Anton