Is this is good way to check for null?

I ran into the code below today, and my gut instinct tells me this is an expensive way to do a null check. The point the author was making was that if you changed the name of the object then you don't need to change the value of the string being thrown in the exception.

The proposed use would be:

string cantBeNull=...;
Guard.IsNotNull(cantBeNull);
//instead of
if(cantBeNull == null) throw new ArgumentNullException("cantBeNull");

So Is this an acceptable way of checking for null? Is this overly expensive just to save you from not having the change the value passed to the exception?

Code in question:

public static void IsNotNull(Expression> expression) where T : class
{
    if (expression == null)
        throw new ArgumentNullException("expression");

    var value = expression.Compile()();
    var param = (MemberExpression)expression.Body;
    var paramName = param.Member.Name;

    if (value != null)
        return;

    throw new ArgumentNullException(paramName);
}

How often do you change parameter names? How often does the code run? Under any normal distribution of those, you should go with a minor code efficiency gain over a programmer efficiency gain.

On the other hand, if you (for some bizarre reason) change the parameter name almost every time the code runs, the tradeoff is probably worth it.

But just remember, that whether or not the string matches the actual parameter name, it should still let you find exactly which parameter was null so long as there isn't anywhere else in that function which throws an ArugmentNullException with the same string.

StackExchange Code Review Q#19032, answer score: 4