30

A common problem in any language is to assert that parameters sent in to a method meet your requirements, and if they don't, to send nice, informative error messages. This kind of code gets repeated over and over, and we often try to create helpers for it. However, in C#, it seems those helpers are forced to deal with some duplication forced upon us by the language and compiler. To show what I mean, let me present some some raw code with no helpers, followed by one possible helper. Then, I'll point out the duplication in the helper and phrase my question precisely.

First, the code without any helpers:

public void SomeMethod(string firstName, string lastName, int age)
{
     if(firstName == null)
     {
          throw new WhateverException("The value for firstName cannot be null.");
     }

     if(lastName == null)
     {
          throw new WhateverException("The value for lastName cannot be null.");
     }

     // Same kind of code for age, making sure it is a reasonable range (< 150, for example).
     // You get the idea
}

}

Now, the code with a reasonable attempt at a helper:

public void SomeMethod(string firstName, string lastName, int age)
{
      Helper.Validate( x=> x !=null, "firstName", firstName);
      Helper.Validate( x=> x!= null, "lastName", lastName);
}

The main question is this: Notice how the code has to pass the value of the parameter and the name of the parameter ("firstName" and firstName). This is so the error message can say, "Blah blah blah the value for the firstName parameter." Have you found any way to get around this using reflection or anything else? Or a way to make it less painful?

And more generally, have you found any other ways to streamline this task of validating parameters while reducing code duplication?

EDIT: I've read people talking about making use of the Parameters property, but never quite found a way around the duplication. Anyone have luck with that?

Thanks!

Jon Skeet
  • 1,421,763
  • 867
  • 9,128
  • 9,194
Charlie Flowers
  • 17,338
  • 10
  • 71
  • 88
  • Use of `ApplicationException` class is not recommended. – Mehrdad Afshari Mar 21 '09 at 18:08
  • OK, I changed it to Exception, just to avoid any distractions. As an aside, if the recommendation is based on comments from the guys who wrote the "Framework Guidelines" book, then I'm not really convinced (though those guys have written some good stuff). But that's a discussion for another day. – Charlie Flowers Mar 21 '09 at 18:12
  • Henk: Wrong. *Any* use of `ApplicationException` is not recommended. See the Remarks section here, especially the second paragraph: http://msdn.microsoft.com/en-us/library/system.applicationexception.aspx – Ryan Lundy Dec 14 '09 at 19:09
  • 1
    @Kyralessa if that's the only citation for this those are rather superflouous arguments to make. There is not 1 substantive reason made on why you shouldn't use application exception in that remarks section. It even directly says "It was originally thought that custom exceptions should derive from the ApplicationException class; however in practice this has not been found to add significant value." so basically MS admitted that the ApplicationException class is unneeded, not that it was bad to use. – Chris Marisic Mar 16 '11 at 15:43

14 Answers14

15

You should check out Code Contracts; they do pretty much exactly what you're asking. Example:

[Pure]
public static double GetDistance(Point p1, Point p2)
{
    CodeContract.RequiresAlways(p1 != null);
    CodeContract.RequiresAlways(p2 != null); 
    // ...
}
John Feminella
  • 303,634
  • 46
  • 339
  • 357
  • Hey, that's cool ... especially that it will be built into the language in c# 4.0. But still ... what can we do in the meantime? – Charlie Flowers Mar 21 '09 at 18:18
  • Well, they'll be baked in .NET 4, but you can still use them right now. They don't depend on anything specific to .NET 4, so you can go all the way back to 2.0 if you want. The project is here: http://research.microsoft.com/en-us/projects/contracts/ – John Feminella Mar 21 '09 at 18:21
  • You can work on your own classes which look like that, but throw exceptions for libraries. Pretty simple to write one up. – user7116 Mar 22 '09 at 03:36
  • 1
    But have You ever tried using code contracts on some bigger projects? Unfortunately it is not usable, compile time increases too much. – Mic Aug 10 '12 at 10:56
11

Wow, I found something really interesting here. Chris above gave a link to another Stack Overflow question. One of the answers there pointed to a blog post which describes how to get code like this:

public static void Copy<T>(T[] dst, long dstOffset, T[] src, long srcOffset, long length)
{
    Validate.Begin()
            .IsNotNull(dst, “dst”)
            .IsNotNull(src, “src”)
            .Check()
            .IsPositive(length)
            .IsIndexInRange(dst, dstOffset, “dstOffset”)
            .IsIndexInRange(dst, dstOffset + length, “dstOffset + length”)
            .IsIndexInRange(src, srcOffset, “srcOffset”)
            .IsIndexInRange(src, srcOffset + length, “srcOffset + length”)
            .Check();

    for (int di = dstOffset; di < dstOffset + length; ++di)
        dst[di] = src[di - dstOffset + srcOffset];
}

I'm not convinced it is the best answer yet, but it certainly is interesting. Here's the blog post, from Rick Brewster.

Charlie Flowers
  • 17,338
  • 10
  • 71
  • 88
7

I tackled this exact problem a few weeks ago, after thinking that it is strange how testing libraries seem to need a million different versions of Assert to make their messages descriptive.

Here's my solution.

Brief summary - given this bit of code:

int x = 3;
string t = "hi";
Assert(() => 5*x + (2 / t.Length) < 99);

My Assert function can print out the following summary of what is passed to it:

(((5 * x) + (2 / t.Length)) < 99) == True where
{
  ((5 * x) + (2 / t.Length)) == 16 where
  {
    (5 * x) == 15 where
    {
      x == 3
    }
    (2 / t.Length) == 1 where
    {
      t.Length == 2 where
      {
        t == "hi"
      }
    }
  }
}

So all the identifier names and values, and the structure of the expression, could be included in the exception message, without you having to restate them in quoted strings.

Daniel Earwicker
  • 114,894
  • 38
  • 205
  • 284
  • Wow, that's excellent. I don't know why I didn't think of that (good ideas often provoke that feeling). I'm writing a calculation app for which this could be very nice in terms of explaining the answers arrived at. – Charlie Flowers Mar 21 '09 at 19:33
  • The combination of reflection and expression trees enables a lot of stuff that takes you by surprise - especially if (like me) you come from a C/C++ background where the runtime is non-existent. We have to re-invent a bunch of stuff that Lispers have been doing for half a century! – Daniel Earwicker Mar 21 '09 at 19:38
  • Nice idea, and also to use for a calc app! – flq Mar 21 '09 at 19:41
7

This may be somewhat helpful:

Design by contract/C# 4.0/avoiding ArgumentNullException

Community
  • 1
  • 1
core
  • 32,451
  • 45
  • 138
  • 193
  • Thanks! This led to 2 approaches that have both blown me away! And I can see using them both. – Charlie Flowers Mar 22 '09 at 03:11
  • That implementation of fluent validation is pretty slick huh. – core Mar 22 '09 at 03:55
  • Yes, it is very slick. I had not realized that you can call an extension method on a variable that is null. I don't know if Anders meant for that to happen or if it is a side-effect. Either way, it's very helpful. – Charlie Flowers Mar 22 '09 at 15:50
  • Charlie, yeah, I kept thinking, "Okay, now that can't be right. What about the NullReferenceException?" until I scrolled down to see the extension methods. Cool deal. – core Mar 23 '09 at 01:17
  • This response taught me the most. Gotta give credit where credit is due. Thanks to everyone for excellent responses! – Charlie Flowers Mar 24 '09 at 20:48
6

Alright guys, it's me again, and I found something else that is astonishing and delightful. It is yet another blog post referred to from the other SO question that Chris, above, mentioned.

This guy's approach lets you write this:

public class WebServer
    {
        public void BootstrapServer( int port, string rootDirectory, string serverName )
        {
            Guard.IsNotNull( () => rootDirectory );
            Guard.IsNotNull( () => serverName );

            // Bootstrap the server
        }
    }

Note that there is no string containing "rootDirectory" and no string containing "serverName"!! And yet his error messages can say something like "The rootDirectory parameter must not be null."

This is exactly what I wanted and more than I hoped for. Here's the link to the guy's blog post.

And the implementation is pretty simple, as follows:

public static class Guard
    {
        public static void IsNotNull<T>(Expression<Func<T>> expr)
        {
            // expression value != default of T
            if (!expr.Compile()().Equals(default(T)))
                return;

            var param = (MemberExpression) expr.Body;
            throw new ArgumentNullException(param.Member.Name);
        }
    }

Note that this makes use of "static reflection", so in a tight loop or something, you might want to use Rick Brewster's approach above.

As soon as I post this I'm gonna vote up Chris, and the response to the other SO question. This is some good stuff!!!

Charlie Flowers
  • 17,338
  • 10
  • 71
  • 88
  • 1
    Hi, bit late in noticing this, but I think it's missing the general power of expressions a bit. Look at my answer again - you don't need to write a separate function for each kind of assertion you might want to make. Just write an expression in normal C# syntax: `X.Assert(() => serverName != null);` There's no good reason to accumulate a set of special methods (IsNull, IsNotNull, AreEqual, etc.) one for each kind of assertion, because we can get a complete description of any expression and all the values in it, whatever the expression happens to be. – Daniel Earwicker Aug 06 '09 at 10:55
  • Earwicker, that's an interesting observation. I like it, though it is not a slam dunk and there might be cases where I'd go with the many-methods approach (IsNull, IsNotNull, etc.) Or at least I'd debate with myself about it. The error message can be prettier saying "FirstName can't be null" than "The assertion (FirstName != null) failed". Plus if you have a hundred places where you assert (something != null), that feels like code duplication. I'm not sure if it is harmful duplication or not. Depends probably. But cool observation, thanks. – Charlie Flowers Aug 17 '09 at 06:33
5

Using my library The Helper Trinity:

public void SomeMethod(string firstName, string lastName, int age)
{
    firstName.AssertNotNull("firstName");
    lastName.AssertNotNull("lastName");

    ...
}

Also supports asserting that enumeration parameters are correct, collections and their contents are non-null, string parameters are non-empty etcetera. See the user documentation here for detailed examples.

Kent Boogaart
  • 175,602
  • 35
  • 392
  • 393
  • +1. I like the use of extension methods to minimize the amount of words. I'd call the method MustNotBeNull or something instead, but that's just personal preference – Orion Edwards Mar 22 '09 at 03:16
  • What happens in that code if firstName is null? Wouldn't a NullRef ensue? Never tried to see if Extension Methods can still attach to a null pointer. – Jarrod Dixon Mar 22 '09 at 03:35
  • @Jarrod, extension methods are static, so in the extension method you can test if the 'this' target is null. – ProfK Mar 22 '09 at 08:04
  • @ProfK - awesome! +1 to this answer, as well, then. – Jarrod Dixon Mar 22 '09 at 23:45
  • I also did not know you could call an extension method on null until I started seeing some of the answers here. This is fantastic, because it opens up a lot of things that result in more "fluent" syntax. – Charlie Flowers Mar 23 '09 at 00:50
4

Here's my answer to the problem. I call it "Guard Claws". It uses the IL parser from the Lokad Shared Libs but has a more straightforward approach to stating the actual guard clauses:

string test = null;
Claws.NotNull(() => test);

You can see more examples of it's usage in the specs.

Since it uses real lambdas as input and uses the IL Parser only to generate the exception in the case of a violation it should perform better on the "happy path" than the Expression based designs elsewhere in these answers.

The links are not working, here is the URL: http://github.com/littlebits/guard_claws/

brendanjerwin
  • 1,381
  • 2
  • 11
  • 25
2

The Lokad Shared Libraries also have an IL parsing based implementation of this which avoids having to duplicate the parameter name in a string.

For example:

Enforce.Arguments(() => controller, () => viewManager,() => workspace);

Will throw an exception with the appropriate parameter name if any of the listed arguments is null. It also has a really neat policy based rules implementation.

e.g.

Enforce.Argument(() => username,    StringIs.Limited(3, 64), StringIs.ValidEmail);
Nick Cox
  • 6,164
  • 2
  • 24
  • 30
1

My preference would be to just evaluate the condition and pass the result rather than passing an expression to be evaluated and the parameter on which to evaluate it. Also, I prefer to have the ability to customize the entire message. Note that these are simply preferences -- I'm not saying that your sample is wrong -- but there are some cases where this is very useful.

Helper.Validate( firstName != null || !string.IsNullOrEmpty(directoryID),
                 "The value for firstName cannot be null if a directory ID is not supplied." );
tvanfosson
  • 524,688
  • 99
  • 697
  • 795
0

In this case, rather than use your own exception type, or really general types like ApplicationException.. I think it is best to use the built in exception types that are specifically intended for this use:

Among those.. System.ArgumentException, System.ArgumentNullException...

markt
  • 5,126
  • 31
  • 25
  • OK, but please ... let's don't focus on the exception type. That is ancillary to the core issue here and therefore I just typed the first thing that popped into my head. – Charlie Flowers Mar 21 '09 at 18:14
  • Fine.. but I believe that any time you are throwing exceptions, throwing a useful exception type is absolutely essential. – markt Mar 21 '09 at 18:30
0

Don't know if this technique transfers from C/C++ to C#, but I've done this with macros:


    #define CHECK_NULL(x)  { (x) != NULL || \
           fprintf(stderr, "The value of %s in %s, line %d is null.\n", \
           #x, __FILENAME__, __LINE__); }
 
Adam Liss
  • 47,594
  • 12
  • 108
  • 150
0

Postsharp or some other AOP framework.

Kris Erickson
  • 33,454
  • 26
  • 120
  • 175
0

It does not apply everywhere, but it might help in many cases:

I suppose that "SomeMethod" is carrying out some behavioral operation on the data "last name", "first name" and "age". Evaluate your current code design. If the three pieces of data are crying for a class, put them into a class. In that class you can also put your checks. This would free "SomeMethod" from input checking.

The end result would be something like this:

public void SomeMethod(Person person)
{
    person.CheckInvariants();
    // code here ...
}

The call would be something like this (if you use .NET 3.5):

SomeMethod(new Person { FirstName = "Joe", LastName = "White", Age = 12 });

under the assumption that the class would look like this:

public class Person
{
    public string FirstName { get; set; }
    public string LastName { get; set; }
    public int Age { get; set; }

    public void CheckInvariants()
    {
        assertNotNull(FirstName, "first name");
        assertNotNull(LastName, "last name");
    }

    // here are your checks ...
    private void assertNotNull(string input, string hint)
    {
        if (input == null)
        {
            string message = string.Format("The given {0} is null.", hint);
            throw new ApplicationException(message);
        }
    }

Instead of the syntactic sugar of .NET 3.5 you can also use constructor arguments to create a Person object.

Theo Lenndorff
  • 4,556
  • 3
  • 28
  • 43
0

Just as a contrast, this post by Miško Hevery on the Google Testing Blog argues that this kind of parameter checking might not always be a good thing. The resulting debate in the comments also raises some interesting points.

Henrik Gustafsson
  • 51,180
  • 9
  • 47
  • 60
  • There is truth to this point of view. I'm not necessarily talking about religiously enforcing contracts (I think that's good in some cases but not all). But still, some methods need to perform certain checks, and often you end up wanting helpers for those cases. – Charlie Flowers Mar 22 '09 at 23:55
  • Certainly these kinds of checks are useful in many situations, I just wanted the contrasting opinion around for future generations :) – Henrik Gustafsson Mar 23 '09 at 00:56