Protecting Invariants of Classes

Question

I have a bit of a storm in a teacup at work, and I'm trying to figure out if I'm in the right, in the wrong or maybe a little bit of both.

It all started out innocently enough; a developer from another team was commenting during a code review of my team's code. We're a [relatively] new team and we tend to write our code a little different to the other teams - we try to follow Clean Code, SOLID principles, do TDD, etc. The rest of the codebase tends towards more...classical...enterprise design and implementation with fixed layers, huge interfaces/ god objects, anemic domain models, and so on.

The codebase itself is for a flagship application that processes a lot of business critical transactions

Historically it was built in a hurry and only in the last few months have we had time to try to remedy a lot of the previous design choices. I'm not a fan of the way it was done, but I'm willing to make tradeoffs where I have to.

Anyway the code looked something like this

public class Something
{
    readonly IFoo _foo;
    public Something(IFoo foo)
    {
        _foo = foo.ThrowIfNull("foo");
    }
}

(ThrowIfNull is a simple generic extension method that wraps the usual "if x==null throw new ArgumentNullException"). I personally am not a fan of it but the others like it so it's cool.

Now, here is what was suggested:

public class Something
{
    readonly IFoo _foo;
    public Something(IFoo foo)
    { 
      #if debug
        _foo = foo.ThrowIfNull("foo");
     #else
        _foo=foo;
    }
}

The rationale was that this could somehow save some overhead in production code.I pointed out that this was not really something to worry about, and that to me it's more important (as in, many many more orders of magnitude more important) that the class invariants(in this case, its dependencies) be protected (i.e. _foo cannot be null) because we need to fail fast if for some reason someone or something tried to construct a Something with a null IFoo.

But then a Senior Architect chimed in to argue that this was something that we should not be concerned about; these are more or less the arguments, along with my rebuttals (all paraphrased of course). I am also an Architect but newly minted so I don't have that much clout yet :)

• We're tightly coupled to our IOC library and it will take care of making sure dependencies are not null. Therefore this code is only testing the IOC library indirectly and should be dropped This assumes that all classes with dependencies are managed by IOC, and even if they are in practice, I believe it's not the point - we should not be coupling ourselves, period.

• The code that tries to use _foo later on will fail anyway To me, this is like justifying skipping pre-flight checks in an airplane because "hey, if the wings fall off mid flight, well we'll know about it then, right?"

(This last one was what made me very scared)

• We have tons of classes that don't already do this, so we don't really want to have to go back and break the (very few) tests we have that will fail when we pass null into the class contstuctors (as we sometimes do) I really have nothing good I can say about this one - as far as I can tell, if this is not an horrendously wrong approach then I think I'll take up farming for a living. It was at this point that I decided to just stop taking part in the conversation, but I have to know if I'm crazy or not :)

So the question is this.

Am I wrong, right or a little bit of both? I was always taught that protecting invariants is basic stuff, something that you just have to do, for the reasons I've listed in my rebuttals. Should I just drop this (not asking in terms of politics or career-wise, just if others feel as I do that's it's really something important and worth fighting for)

Answer 1

You are definitely in the right here. This is pretty basic defensive programming and there are no real arguments against it.

Now for the arguments :

The coupling to IoC and fail-fast issue is just like you said.

As for the third argument, just because this one class has argument null check doesn't mean you have to go back and change all other classes. It just means this piece of code is nicer than the rest.

And as for the "overhead in production", that is just silly. That constructor will be called few times. Hell, even if it was called million times during application's life, even that would not justify complicating the code with compiler conditional. And if they really want to do it like that, better way would be to create two versions of the ThrowIfNull method. One would throw in debug mode and other will be empty in release mode. Then, the compiler will optimize it away.