The other day, a colleague and I were having a quite heated debate about the need to check an interface passed into a class as a collaborator argument in the ctor. (see Dependency Injection)
This is a technique we use extensively using frameworks such as StructureMap and Castle Windsor, in order to make our classes small, loosely coupled and testable by allowing mocked implementations to be injected in within unit/integration tests.
The problem is, as you are allowing an interface to be passed in to the constructor to be translated to a member variable, there is a chance that within your class you will be attempting to access a property of a null, and therefore an evil NullReferenceException will be thrown! <<shudder>>
My colleague pointed out that we should always check for null, without exception (if you pardon the pun) and return an ArgumentNullException instead, and spent the next 15 minutes bombarding me with internet links stating that it was standard practice. It was argued that a developer will benefit from a more useful message, stating which parameter was null (fair point). Most of these articles did refer to null checks on parameters within a class method rather than a constructor, which I would actually agree with, but my beef was about having them in the constructor.
Now my argument is that, within our code base, you DON’T need to check for null everywhere as you run the risk of the following:
- Violating DRY
- Sullying your nice clean code with boilerplate try catch checks
- Optimising too far down the stack
These kind of checks should be made as close to the top of the stack as possible, if at all. Within our code base all of our classes are consumed internally, we don’t really offer OS compiled class libraries for use by third parties. There are occasions where compiled libraries are passed around between teams, and in this regard, we can use the built in class access modifiers to prevent access (via the internal keyword) to classes that would allow a null argument to be passed in the ctor.
If we really want to help out our fellow developers, we can insert our null checks with meaningful messages as close to the point of usage as possible, within a public class that we have correctly managed the access of, again at the top of the stack/point of entry for that developer.
Also, our DI frameworks generally offer the concept of a Bootstrap “facility” (as its called in Castle), that allow a developer the ability to offer the standard top-of-the-stack resolution of all the concrete types we will need within the code-base. This is a method used in third party OSS libraries like SolrNet.
Finally, try/catch is ugly, I would be moved to tears to see it used in every single constructor in every single class I had created within a project, just to give a developer a helpful indicator as to which param was null. I know not everyone will agree, but my view is that I would rather use clean code and tests to help a future developer, than assert for null.
I’d like to go out with a quote from this post:
“Checking for nulls usually goes against testability, and given a choice between well tested code and untested code with asserts, there is no debate for me which one I chose.”
Amen to that……