20

My domain consists of lots of simple immutable classes like this:

public class Person
{
    public string FullName { get; }
    public string NameAtBirth { get; }
    public string TaxId { get; }
    public PhoneNumber PhoneNumber { get; }
    public Address Address { get; }

    public Person(
        string fullName,
        string nameAtBirth,
        string taxId,
        PhoneNumber phoneNumber,
        Address address)
    {
        if (fullName == null)
            throw new ArgumentNullException(nameof(fullName));
        if (nameAtBirth == null)
            throw new ArgumentNullException(nameof(nameAtBirth));
        if (taxId == null)
            throw new ArgumentNullException(nameof(taxId));
        if (phoneNumber == null)
            throw new ArgumentNullException(nameof(phoneNumber));
        if (address == null)
            throw new ArgumentNullException(nameof(address));

        FullName = fullName;
        NameAtBirth = nameAtBirth;
        TaxId = taxId;
        PhoneNumber = phoneNumber;
        Address = address;
    }
}

Writing the null checks and property initialization is already getting very tedious but currently I write unit tests for each of these classes to verify that argument validation works correctly and that all properties are initialized. This feels like extremely boring busywork with incommensurate benefit.

The real solution would be for C# to support immutability and non-nullable reference types natively. But what can I do to improve the situation in the meantime? Is it worth writing all these tests? Would it be a good idea to write a code generator for such classes to avoid writing tests for each one of them?


Here is what I have now based on the answers.

I could simplify the null checks and property initialization to look like this:

FullName = fullName.ThrowIfNull(nameof(fullName));
NameAtBirth = nameAtBirth.ThrowIfNull(nameof(nameAtBirth));
TaxId = taxId.ThrowIfNull(nameof(taxId));
PhoneNumber = phoneNumber.ThrowIfNull(nameof(phoneNumber));
Address = address.ThrowIfNull(nameof(address));

Using the following implementation by Robert Harvey:

public static class ArgumentValidationExtensions
{
    public static T ThrowIfNull<T>(this T o, string paramName) where T : class
    {
        if (o == null)
            throw new ArgumentNullException(paramName);

        return o;
    }
}

Testing the null checks is easy using the GuardClauseAssertion from AutoFixture.Idioms (thanks for the suggestion, Esben Skov Pedersen):

var fixture = new Fixture().Customize(new AutoMoqCustomization());
var assertion = new GuardClauseAssertion(fixture);
assertion.Verify(typeof(Address).GetConstructors());

This could be compressed even further:

typeof(Address).ShouldNotAcceptNullConstructorArguments();

Using this extension method:

public static void ShouldNotAcceptNullConstructorArguments(this Type type)
{
    var fixture = new Fixture().Customize(new AutoMoqCustomization());
    var assertion = new GuardClauseAssertion(fixture);

    assertion.Verify(type.GetConstructors());
}
  • 3
    The title/tag talks about (unit) testing of field values implying post-construction unit testing of the object, but the code snippet shows input argument/parameter validation; these are not the same concepts. – Erik Eidt Nov 16 '16 at 16:08
  • 2
    FYI, you could write a T4 template that would make this kind of boilerplate easy. (You might also consider string.IsEmpty() beyond == null.) – Erik Eidt Nov 16 '16 at 16:13
  • 1
    Have you taken a look at autofixture idioms? nuget.org/packages/AutoFixture.Idioms – Esben Skov Pedersen Nov 16 '16 at 19:51
  • 1
    Related: stackoverflow.com/questions/291340/… – Doc Brown Nov 16 '16 at 21:09
  • 1
    You could also use Fody/NullGuard, though it looks like it doesn't have a default-allow mode. – Bob Nov 16 '16 at 23:05
5

I created a t4 template exactly for this kind of cases. To avoid writing lots of boilerplate for Immutable classes.

https://github.com/xaviergonz/T4Immutable T4Immutable is a T4 template for C# .NET apps that generates code for immutable classes.

Specifically talking about non null tests then if you use this:

[PreNotNullCheck, PostNotNullCheck]
public string FirstName { get; }

The constructor will be this:

public Person(string firstName) {
  // pre not null check
  if (firstName == null) throw new ArgumentNullException(nameof(firstName));

  // assignations + PostConstructor() if needed

  // post not null check
  if (this.FirstName == null) throw new NullReferenceException(nameof(this.FirstName));
}

Having said this, if you use JetBrains Annotations for null checking, you can also do this:

[JetBrains.Annotations.NotNull, ConstructorParamNotNull]
public string FirstName { get; }

And the constructor will be this:

public Person([JetBrains.Annotations.NotNull] string firstName) {
  // pre not null check is implied by ConstructorParamNotNull
  if (firstName == null) throw new ArgumentNullException(nameof(firstName));

  FirstName = firstName;
  // + PostConstructor() if needed

  // post not null check implied by JetBrains.Annotations.NotNull on the property
  if (this.FirstName == null) throw new NullReferenceException(nameof(this.FirstName));
}

Also there are a few more features than this one.

  • Thank you. I've just tried it and it worked perfectly. I mark this as the accepted answer as it addresses all the issues in the original question. – Botond Balázs Nov 17 '16 at 23:36
16

You can get a bit of improvement with a simple refactoring that can ease the problem of writing all those fences. First, you need this extension method:

internal static T ThrowIfNull<T>(this T o, string paramName) where T : class
{
    if (o == null)
        throw new ArgumentNullException(paramName);

    return o;
}

You can then write:

public class Person
{
    public string FullName { get; }
    public string NameAtBirth { get; }
    public string TaxId { get; }
    public PhoneNumber PhoneNumber { get; }
    public Address Address { get; }

    public Person(
        string fullName,
        string nameAtBirth,
        string taxId,
        PhoneNumber phoneNumber,
        Address address)
    {
        FullName = fullName.ThrowIfNull(nameof(fullName));
        NameAtBirth = nameAtBirth.ThrowIfNull(nameof(nameAtBirth));
        TaxId = taxId.ThrowIfNull(nameof(taxId));
        PhoneNumber = phoneNumber.ThrowIfNull(nameof(fullName));
        Address = address.ThrowIfNull(nameof(address));
    }
}

Returning the original parameter in the extension method creates a fluent interface, so you can extend this concept with other extension methods if you wish, and chain them all together in your assignment.

Other techniques are more elegant in concept, but progressively more complex in execution, such as decorating the parameter with a [NotNull] attribute, and using Reflection like this.

That said, you may not need all these tests, unless your class is part of a public-facing API.

  • 3
    Odd that this was downvoted. It's very similar to the approach provided by the most upvoted answer, except that it does not depend on Roslyn. – Robert Harvey Nov 16 '16 at 20:12
  • What I don't like about this is that it's vulnerable to modifications to the constructor. – jpmc26 Nov 17 '16 at 1:24
  • @jpmc26: What does that mean? – Robert Harvey Nov 17 '16 at 21:38
  • 1
    As I read your answer, it's suggesting that you don't test the constructor, but instead you create some common method called on each parameter and test that. If you add a new property/argument pair and forget to test for null on the argument to constructor, you won't get a failed test. – jpmc26 Nov 17 '16 at 21:40
  • @jpmc26: Naturally. But that's also true if you write it the conventional way as illustrated in the OP. All the common method does is move the throw outside of the constructor; you're not really transferring control within the constructor somewhere else. It's just a utility method, and a way to do things fluently, if you so choose. – Robert Harvey Nov 17 '16 at 21:42
7

In the short term, there's not much you can do about the tediousness of writing such tests. However, there is some help coming with throw expressions due to be implemented as part of the next release of C# (v7), likely due in the next few months:

public class Person
{
    public string FullName { get; }
    public string NameAtBirth { get; }
    public string TaxId { get; }
    public PhoneNumber PhoneNumber { get; }
    public Address Address { get; }

    public Person(
        string fullName,
        string nameAtBirth,
        string taxId,
        PhoneNumber phoneNumber,
        Address address)
    {
        FullName = fullName ?? throw new ArgumentNullException(nameof(fullName));
        NameAtBirth = nameAtBirth ?? throw new ArgumentNullException(nameof(nameAtBirth));
        TaxId = taxId ?? throw new ArgumentNullException(nameof(taxId)); ;
        PhoneNumber = phoneNumber ?? throw new ArgumentNullException(nameof(phoneNumber)); ;
        Address = address ?? throw new ArgumentNullException(nameof(address)); ;
    }
}

You can experiment with throw expressions via the Try Roslyn webapp.

  • Much more compact, thank you. Half as many lines. Looking forward to C# 7! – Botond Balázs Nov 16 '16 at 17:02
  • @BotondBalázs you can try it in the VS15 preview right now if you want – user1306322 Nov 16 '16 at 20:25
7

I'm surprised no one mentioned NullGuard.Fody yet. It's available via NuGet and will automagically weave those null checks into the IL during compile time.

So your constructor code would simply be

public Person(
    string fullName,
    string nameAtBirth,
    string taxId,
    PhoneNumber phoneNumber,
    Address address)
{
    FullName = fullName;
    NameAtBirth = nameAtBirth;
    TaxId = taxId;
    PhoneNumber = phoneNumber;
    Address = address;
}

and NullGuard will add those null checks for you transforming it into exactly what you wrote.

Note though, that NullGuard is opt-out, that is, it will add those null checks to every method and constructor argument, property getter and setter and even check method return values unless you explicitly allow a null value with the [AllowNull] attribute.

  • Thank you, this looks like a very nice general solution. Though it is very unfortunate that it modifies language behavior by default. I would like it much more if it worked by adding a [NotNull] attribute instead of not allowing null being the default. – Botond Balázs Nov 16 '16 at 21:42
  • @BotondBalázs: PostSharp has this along with other parameter validation attributes. Unlike Fody it isn't free though. Also, the generated code will call into the PostSharp DLLs so you need to include those with your product. Fody will only run its code during compilation and won't be required at runtime. – Roman Reiner Nov 16 '16 at 21:51
  • @BotondBalázs: I also found it kind of weird at first that NullGuard applies per default but it makes sense really. In any reasonable code base allowing null should be much less common than checking for null. If you really want to allow null somewhere the opt-out approach forces you to be very thorough with it. – Roman Reiner Nov 16 '16 at 21:57
  • 5
    Yes, that's reasonable but it violates the Principle of Least Astonishment. If a new programmer doesn't know that the project uses NullGuard, they're in for a big surprise! By the way, I don't understand the downvote either, this is a very useful answer. – Botond Balázs Nov 16 '16 at 22:05
  • 1
    @BotondBalázs / Roman, looks like NullGuard has a new explicit mode that changes the way it works by default – Kyle W Apr 29 at 17:13
5

Is it worth writing all these tests?

No, probably not. What is the likelihood that you're going to screw that up? What is the likelihood that some semantics will change out from under you? What is the impact if someone does screw it up?

If you're spending a bunch of time making tests for something that will rarely break, and is a trivial fix if it did... maybe not worth it.

Would it be a good idea to write a code generator for such classes to avoid writing tests for each one of them?

Maybe? That sort of thing could be done easily with reflection. Something to consider is doing code generation for the real code, so you don't have N classes that may have human error. Bug prevention > bug detection.

  • Thank you. Maybe I wasn't clear enough in my question but I meant to write a generator that generates immutable classes, not tests for them – Botond Balázs Nov 16 '16 at 16:05
  • 2
    I've also seen several times instead of an if...throw they will have ThrowHelper.ArgumentNull(myArg, "myArg"), that way the logic is still there and you don't get dinged on code coverage. Where the ArgumentNull method will do the if...throw. – Matthew Nov 16 '16 at 16:20
2

Is it worth writing all these tests?

No.
Because I am pretty sure you have tested those properties through some other tests of logic where those classes are used.

For example, you can have tests for Factory class which have assertion based on those properties (Instance created with properly assigned Name property for example).

If those classes are exposed to the public API which used by some third part/end user (@EJoshua thanks for noticing), then tests for expected ArgumentNullException can be useful.

While waiting for C#7 you can use extension method

public MyClass(string name)
{
    name.ThrowArgumentNullExceptionIfNull(nameof(name));
}

public static void ThrowArgumentNullExceptionIfNull(this object value, string paramName)
{
    if(value == null)
        throw new ArgumentNullException(paramName);
}

For testing you can use one parameterized test method which use reflection to create null reference for every parameter and assert for expected exception.

  • 1
    That's a convincing argument for not testing property initialization. – Botond Balázs Nov 16 '16 at 18:02
  • That depends on how you're using the code. If the code is part of a public-facing library you should never blindly trust other people to know how to your library (especially if they don't have access to the source code); however, if it's something you just use internally to make your own code work you ought to know how to use your own code, so the checks have less of a purpose. – EJoshuaS - Reinstate Monica Nov 16 '16 at 18:26
2

You could always write a method like the following:

// Just to illustrate how to call this
private static void SomeMethod(string a, string b, string c, string d)
    {
        ValidateArguments(a, b, c, d);
        // ...
    }

    // This is the one to use as a utility function
    private static void ValidateArguments(params object[] args)
    {
        for (int i = 0; i < args.Length; i++)
        {
            if (args[i] == null)
            {
                StackTrace trace = new StackTrace();
                // Get the method that called us
                MethodBase info = trace.GetFrame(1).GetMethod();

                // Get information on the parameter that is null so we can add its name to the exception
                ParameterInfo param = info.GetParameters()[i];

                // Raise the exception on behalf of the caller
                throw new ArgumentNullException(param.Name);
            }
        }
    }

At a minimum, that'll save you some typing if you have several methods that require this kind of validation. Of course, this solution assumes that none of your method's parameters can be null, but you can modify this to change that if you so desire.

You can also extend this to perform other type-specific validation. For example, if you have a rule that strings can't be purely whitespace or empty, you could add the following condition:

// Note that we already know based on the previous condition that args[i] is not null
else if (args[i].GetType() == typeof(string))
            {
                string argCast = arg as string;

                if (!argCast.Trim().Any())
                {
                    ParameterInfo param = GetParameterInfo(i);

                    throw new ArgumentException(param.Name + " is empty or consists only of whitespace");
                }
            }

The GetParameterInfo method I refer to basically does the reflection (so that I don't have to keep typing the same thing over and over, which would be a violation of the DRY principle):

private static ParameterInfo GetParameterInfo(int index)
    {
        StackTrace trace = new StackTrace();

        // Note that we have to go 2 methods back to get the ValidateArguments method's caller
        MethodBase info = trace.GetFrame(2).GetMethod();

        // Get information on the parameter that is null so we can add its name to the exception
        ParameterInfo param = info.GetParameters()[index];

        return param;
    }
  • 1
    Why is this being downvoted? It's not too bad – Gaspa79 Feb 20 '18 at 14:02
0

I do not suggest throwing exceptions from the constructor. The problem is you will have a hard time to test this as you have to pass valid parameters even they are irrelevant for your test.

For example: If you want to test if the third parameter throws an exception you have to pass in the first and the second correctly. So your test is not isolated anymore. when the constraints of the first and second parameter change this test case will fail even it tests the third parameter.

I suggest to use the java validation API and externalize the validation process. I suggest to have four responsibilities (classes) involved:

  1. A suggestion object with Java validation annotated parameters with a validate method that returns a Set of ConstraintViolations. One advantage is you can pass around this objects without the assertion to be valid. You can delay validation until it is neccessary without having a try to instantiate the domain object. The validation object can be used in different layers as it is a POJO with no layer specific knowledge. It can be part of you public API.

  2. A factory for the domain object that is responsible for creating valid objects. You pass in the suggestion object and the factory will call the validation and create the domain object if everything is fine.

  3. The domain object itself which should be mostly inaccessible for instantiation for other developers. For testing purposes I suggest to have this class in package scope.

  4. A public domain interface to hide the concrete domain object and make misusage hard.

I do not suggest to check for null values. As soon as you get into the domain layer you have get rid of passing in null or returning null as long as you do not have object chains that have ends or search functions for single objects.

One other point: writing as less code as possible is not a metric for code quality. Think about 32k games competitions. That code is the most compact but also the messiest code you can have because it does only care about technical issues and does not care about semantics. But semantics are the points that make things comprehensive.

  • 1
    I respectfully disagree with your last point. Not having to type the same boilerplate for the 100th time does help reduce the chance of making a mistake. Imagine if this were Java, not C#. I would have to define a backing field and a getter method for each property. The code would become completely unreadable and what is important would be obscured by the clunky infrastructure. – Botond Balázs Nov 17 '16 at 7:33

protected by gnat Nov 17 '16 at 21:04

Thank you for your interest in this question. Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site (the association bonus does not count).

Would you like to answer one of these unanswered questions instead?

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