0

I've been refactoring guard statements for my project to simply use a custom Ensure class with 2 methods. Some methods require multiple null checks and I implemented a guard method that simply accepts a list. This method will call the singular guard check for each value given. However I don't seem to find any other examples of this done and was wondering if there was a reason for this.

The guard methods:

public static void ArgumentNotNull(object argument, string argumentName)
{
    if (argument == null)
    {
        throw new ArgumentNullException(argumentName);
    }
}

public static void ArgumentsNotNull(params object[] arguments)
{
    foreach(object argument in arguments)
    {
        ArgumentNotNull(argument, nameof(argument));
    }
}

A call would then look like:

Ensure.ArgumentsNotNull(value1, value2, value3);

Is there anything wrong with this approach?

4
  • 5
    My recollection may be wrong, but doesn't nameof return the static name of its argument - that is, it will return the string "argument", since that is the name of the variable that is passed in?
    – Steve
    Dec 14, 2021 at 12:55
  • @Steve True actually. This is probably the reason. Cheers!
    – M. Mayhem
    Dec 14, 2021 at 13:16
  • 2
    Doesn't C# now have nullable reference types, which solves this problem at the type-system level?
    – Alexander
    Dec 14, 2021 at 15:40
  • @Alexander: introducing nullable reference types into a large existing code base can be pretty hard.
    – Doc Brown
    Dec 14, 2021 at 18:46

2 Answers 2

3

Is there anything wrong with this approach?

There's nothing wrong, per se, but nullability is not the only guard that exists. When you start accounting for all possible guards, especially when you get to compound evaluations (x == null || x.IsActive), making prefabs for every check is a bit futile.

In really, all you've really done here is rephrase an evaluation x != null with the English IsNotNull(x), which to any developer (who is not still learning the ropes) reads the same and doesn't really improve the readability.

If you had a non-trivial bit of custom logic that needed to be used all across the codebase (e.g. validating a credit card number), I'd be on board with your idea. But this example is IMHO too trivial to warrant an extra abstraction.


That being said, in testing this is something that is done. Ensure.ArgumentsNotNull is pretty much identical to the existing Assert.NotNull. However, Assert is tailored towards specifically throwing assertion exceptions that test runners capture.

5
  • The codebase is decently large and is cluttered with: if (sampleValue == null) throw new ArgumentNullException(nameof(sampleValue)); Some methods having up to four checks. This was basically only meant for nullchecks. Whenever a specific class related check has to be done (i.e. Active), ofcourse a different approach will be taken.
    – M. Mayhem
    Dec 14, 2021 at 12:51
  • @M.Mayhem: There are different abstraction approaches here. By itself, I do not think a single evaluation warrants abstraction. A set of them? Maybe. But I'd be inclined to abstract that privately in the class itself (some sort of private void Validate(...) method) as opposed to abstracting it. You might also want to look into a dedicated validation library, as opposed to rolling your own. That being said, what you're doing isn't particularly wrong, it's just that it often ends up not being worth the effort of doing so. If you think it's worth the effort, then by all means.
    – Flater
    Dec 14, 2021 at 12:54
  • The problem is related to a project with multiple methods within the same class requiring a validation check. I think in this project, the Validate solution as private methods would not be the right approach, but I'll keep it in mind for future projects. Thank you very much. I was just worried I was overlooking something as I don't see it done. I'm also with you on dont reinvent the wheel, but I prefer not to depend on a library for something as simple as this.
    – M. Mayhem
    Dec 14, 2021 at 12:56
  • @M.Mayhem "I'm also with you on dont reinvent the wheel, but I prefer not to depend on a library for something as simple as this." I get where you're coming from and I do agree with it up to a point. But it is ironically the same line of thinking as to why I don't think it warrants an abstraction to begin with, i.e. because it's too simple.
    – Flater
    Dec 14, 2021 at 13:02
  • I just don't like looking at 4 rows of the line in my first comment and would rather have a more clean and consise looking guard statement without using private methods for each method that requires a guard statement. But maybe you are right.
    – M. Mayhem
    Dec 14, 2021 at 13:05
4

Is there anything wrong with this approach?

Yes... it doesn't work. Just test it.

Ensure.ArgumentsNotNull(value1, value2, value3);

If you call it like this, and for example value2 is null, you will get an Exception telling you that your parameter "argument" is null. Because that is what nameof(argument) does. It returns "argument". It does not magically return the actual name of the parameter that was passed.

In the future, if you want to know if something works, I suggest writing a unit test for it. That is the easiest way to find out.

4
  • 1
    This answer is correct. However, I think it would be a little bit more polite not only to assess the code which does not work, but also the part of the code which works.
    – Doc Brown
    Dec 14, 2021 at 18:52
  • Well, in context of the question "using params instead of..." it really doesn't matter how much of it works, when the central piece doesn't. I don't mean to be rude, but I don't see how "the rest of it works fine" would help either.
    – nvoigt
    Dec 14, 2021 at 19:31
  • Depends how you define "works". If "works" means "prevents the code from running with null parameters", the OP's approach does work. There is no requirement mentioned in the question, to return a user friendly Exception. I agree that would be nice. And some simple reworking of the Ensure.ArgumentsNotNull() code could easily return at least the index of the bad argument.
    – user949300
    Dec 21, 2021 at 19:24
  • @user949300 I would assume "works" means "throws an ArgumentNullException with the argument name as parameter", as obviously intended and as is best practice.
    – nvoigt
    Dec 23, 2021 at 10:37

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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