Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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<T>(Expression<Func<T>> 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);
}
share|improve this question
5  
I have a feeling this will be close to an order of magnitude slower than a simple null check. Yes, overly expensive. –  Jesse C. Slicer Nov 26 '12 at 19:45
1  
I feel like the next logical step for this code is to make it an extension method so you can just do a cantBeNull.IsNotNull() :) –  Joe Nov 27 '12 at 16:22
add comment

1 Answer 1

up vote 5 down vote accepted

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.

share|improve this answer
1  
Why would you change the parameter name almost every time the code runs? Apart from that, good answer! –  jpfollenius Nov 27 '12 at 7:13
1  
@Smasher - I really don't know, but that's the only scenario I could think of where dynamically pulling the parameter name would make sense. I suppose there could be some business logic reason to do so? –  Bobson Nov 27 '12 at 14:58
    
How could business logic affect your code? That doesn't make sense. Maybe the content changes each time but not the name of the parameter itself. –  jpfollenius Nov 27 '12 at 15:16
    
@Smasher - Business logic may not have been the right term - I'm thinking of something ridiculous like "Our coding standard says that all parameter names need to have the last date the function was edited appended in _20121127 format". –  Bobson Nov 27 '12 at 16:20
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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