2

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)

  • You said that this was "built in a hurry." Is it permanent, critical, high-profile code that adds ongoing value to the business concern, or is it one-shot, throwaway code that will never be used again? Is it somewhere in-between? – Robert Harvey Sep 21 '16 at 19:57
  • Good question. The codebase itself is for a flagship application that processes a lot of business critical transactions. So not a throwaway :) – Stephen Byrne Sep 21 '16 at 19:59
  • 1
    If you're really going to use #if debug, why not just put that into the ThrowIfNull() method? That way, you only have to say it once. – Robert Harvey Sep 21 '16 at 20:00
  • Well the thing is, I disagree with disabling the guard at all. maybe it's not clear from the question. But what is being proposed means the guard doesn't exist in production. – Stephen Byrne Sep 21 '16 at 20:01
  • 1
    "We have tons of classes that don't do this" is a straw man (or "appeal to authority," "non-sequitur," and perhaps a few others, take your pick). Improvements to the reliability of classes by adding guard clauses do not have to happen all at once; they can be undertaken one class at a time, or not undertaken at all. – Robert Harvey Sep 21 '16 at 20:10
5

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.

  • It would be most optimal to implement the ThrowIfNull as an inline. In that case, if (since) the body is simple enough for the optimizer, the resulting code has no function call, and it would be dumb to invoke "too costly" as a reason to not do the check; it's only an extra if. – BenPen Sep 21 '16 at 20:52
  • yes I thought the initial argument about overhead was silly, and actually I'm glad you mentioned "defensive programming", I knew there was a succint way to sum it all up, just couldn't remember :) Thanks! – Stephen Byrne Sep 21 '16 at 21:00

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.