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 am using this code in a lot of places:

if (ParameterName == null)
{
    throw new ArgumentNullException("ParameterName");
}

But I think this is not DRY. The only difference here is ParameterName. Is it ok to use these lines everywhere, or is there a way to make it DRY?

share|improve this question
    
This is where you wish you had C macros –  Demetri Mar 24 at 22:34
    
@Demetri no, it really isn't! Even if you did, there's nothing to stop you using the C preprocessor on C# files. –  MattDavey Mar 27 at 11:21
add comment

5 Answers 5

up vote 8 down vote accepted

Looks pretty DRY to me. :) You can use expressions to somewhat reduce the amount of code you need to write:

    public static void ThrowIfNull(Expression<Func<object>> expression)
    {
        var res = expression.Compile();
        if (res.Invoke() == null)
        {
            var lambda = (LambdaExpression) expression;
            var member = (MemberExpression) lambda.Body;
            throw new ArgumentNullException(member.Member.Name);
        }
    }

    public void SomeMethod(object someParameter)
    {
        ThrowIfNull(() => someParameter);
    }

but frankly i'd go with Re-sharper templates (or VS code snippets) instead.

Edit: A quick test on my machine shows, that on average it takes about 0.22 milliseconds per call to execute ThrowIfNull 1000000 times with different (not null) arguments. With first call being ~20 times slower (perhaps repeated compilation is optimized and/or cached in some way, tho i am not competent enough on that subject to make any claims). Is it slower then one equality check? Well, obviously. But it is still unlikely to become any kind of a bottleneck in real life scenario, unless overused. As i said, however, i would not use it. The original code looks clear and short enough to me, even shorter, when templated. So i always go with simple explicit null check.

share|improve this answer
    
You mean the condition checking is DRY itself. Yes, I have found these lines a lot in ASP.NET MVC 5 source which led me to think this is not bed. –  user960567 Mar 24 at 11:02
8  
Won't that be quite slow, compiling an expression each time? A null check should be cheap so you can do lots of them - compiling code for each one seems like it might slow things down a lot. –  MrKWatkins Mar 24 at 12:58
    
Why are you using an expression for this? Seems a little overkill to me, unless of course I'm missing something blindingly obvious. –  Joe Mar 24 at 17:57
    
@MrKWatkins, slow compared to what? To the original solution? Yes, of course. See my edit. –  Nikita Brizhak Mar 25 at 5:03
    
@Joe, to get a parameter name without manually typing a string :) –  Nikita Brizhak Mar 25 at 5:06
show 5 more comments

Google Guava's Precondition class contains good patterns for this problem. For example, the checkNotNull method is the following:

T checkNotNull(T reference, String errorMessageTemplate, Object... errorMessageArgs)

Usage:

checkNotNull(parameter, "parameter cannot be null");

Note its return value. If the value is not null it returns the referencce, so you can use it in the following way:

public MyClass(MyObject dependency) {
    this.dependency = checkNotNull(dependency, "dependency cannot be null");
}

It also supports error message templates with %s.

Although it's a Java library, you can implement the same methods in C# too, they are really simple.

See also: Preconditions Explained

share|improve this answer
    
+1 for mentioning preconditions. This seems like a good time to introduce code contracts (and the DbC principle in general). –  MattDavey Mar 24 at 11:17
    
I am using C#.. –  user960567 Mar 24 at 11:29
    
@user960567: "Although it's a Java library, you can implement the same methods in C# too, they are really simple." –  palacsint Mar 24 at 11:35
2  
As of Java 7, you could use the built-in Objects.requireNotNull method. But then again... this is a C# question :) –  Simon André Forsberg Mar 24 at 12:49
add comment

You could try to extract it to a method, but you'd still have to call it for every parameter and you will lose the information, what the parameter's name is....

CheckNull(object ParameterName){
   if(parameterName == null){
      throw new ArgumentNullException("some param was null, no clue which...");
   }
}
share|improve this answer
add comment

I would go for a generic extension method for this:

public static void ThrowIfNull<T>(this T data, string name) where T : class
{
    if (data == null)
        throw new ArgumentNullException(name);
}

This way, you can call

Parameter.ThrowIfNull("Name");

If you're calling this on a property, you could use CallerMemberNameAttribute, this won't work from a method though.

You might be able to replace the name paramter of the method with CallerMemberNameAttribute though I haven't verified this works.

share|improve this answer
add comment

If you just want a short syntax, you could use Code Contracts. Either

Contract.Requires( x != null );

or

Contract.Requires<ArgumentNullException>( x != null, "x" )

depending on whether the exact exception type is important to you. There is also som configuration to be done, depending on if you want these checks only for debug, or also for release.

I should point out however that Code Contracts are really about much more than simple argument checks, and it has its own pros and cons. More information
http://msdn.microsoft.com/en-us/library/dd264808(v=vs.110).aspx

share|improve this answer
1  
+1 I was waiting to see how long it would be before someone mentioned Code Contracts :) –  MattDavey Mar 26 at 10:58
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.