Shields Up! (continued)

(Continued from the previous article:  “Shields Up!”)

Here is where I think the argument to ‘check for null in all cases’ breaks down under the defensive programming paradigm.  The best opportunity to check the state of m_pObjAPtr and s_pInstance is the moment SomeClass is created or requested.  We want to identify the problem early to provide opportunities to correct or avoid issues when they happen.  So you could write the following code:

   1:  

   2: SomeClass.h

   3: -----------

   4:  

   5: class SomeClass {

   6:     public:

   7:         static boost::shared_ptr<SomeClass> getInstance();

   8:         bool ReadyToBeUsed();

   9:  

  10:     private:

  11:         static boost::shared_ptr<SomeClass> s_pInstance;

  12:         ObjectAPtr m_pObjAPtr;

  13: };

  14:  

  15: SomeClass.cpp

  16: -------------

  17:  

  18: SomeClassPtr SomeClass::getInstance()

  19: {

  20:     if (!s_pInstance)

  21:     {

  22:         s_pInstance.reset( new SomeClass() );

  23:     }

  24:  

  25:     assert(s_pInstance->ReadyToBeUsed());

  26:  

  27:     return s_pInstance;

  28: }

  29:  

  30: SomeClass::SomeClass() {

  31:     m_pObjAPtr = GetFromFactory();

  32:     assert(m_pObjAPtr);

  33: }

  34:  

  35: bool SomeClass::ReadyToBeUsed() {

  36:     return (pObjAPtr) ? true : false;

  37: }

 

The new code checks the internal state of the SomeClass instance and asserts if there is a problem.  Now SomeClass has a publically accessible method to validate its internal state.  Given that you added the null check in the caller because the response from getInstance() cannot be trusted, by extension the same rule is applied to checking ReadyToBeUsed().  You could argue that the null check is enough, but lets be honest, just because it is not null, does not mean that it is usable.  Especially considering that the object is leveraged by other systems since it is a singleton in this example.  Checking for null is a very limited validation.  This is an opportunity to further ensure that the object at the end of the pointer is usable.  If you are truly concerned about dodging erroneous behaviours, you have to run ReadyToBeUsed() next to the null check in the callers code.

So now you have the following:

   1: MyCode.cpp

   2: ----------

   3: SomeClassPtr pLocal = SomeClass::getInstance();

   4:  

   5: if (pLocal && pLocal->ReadyToBeUsed()) {

   6:     pLocal->SomeMethod();

   7: }

 

An easy win and the code is safe right?  Yes, but at what cost?  The logic for what constitutes a valid instance of SomeClass is duplicated across two entities in the software.  This would not be be much of a problem if SomeClass is only used once.  In our code SomeClass is used in several places.  Which means the validations for a valid SomeClass instance is duplicated across several systems.  Should the requirements change for ReadyToBeUsed (perhaps passing flag for deep or shallow check of its member vars), or if a new entity needs checking before SomeClass (a user security check), you may need to edit each system and update each check – everywhere there is a response from getInstance().

Let’s not forget that SomeClass is not the only object usable in the system.  Several classes use multiple instances of other classes.  If you truly want your code to test for a usable instance of the objects it uses you could see this in your code:

   1: SomeClassPtr pLocal1 = SomeClass1::getInstance();

   2: SomeClassPtr pLocal2 = SomeClass2::getInstance();

   3: SomeClassPtr pLocal3 = SomeClass3::getInstance();

   4:  

   5: if (pLocal1 && pLocal1->ReadyToBeUsed()) {

   6:     pLocal1->SomeMethod();

   7: }

   8:  

   9: if (pLocal2 && pLocal2->ReadyToBeUsed()) {

  10:     pLocal2->SomeMethod();

  11: }

  12:  

  13: if (pLocal3 && pLocal3->ReadyToBeUsed()) {

  14:     pLocal3->SomeMethod();

  15: }

 

An additional problem with this mindset is that several entities in the software ecosystem now have knowledge of what a valid SomeClass is.  This not only breaks the DRY principle, but also erodes the encapsulation of SomeClass.  After all, getInstance() was supposed to guarantee a valid instance of SomeClass.

I don’t like that direction.  Forget the calls to ReadyToBeUsed().  Even if all you check for is null in this code, it adds zero value if a null pointer is not possible, duplicates responsibilities, and clutters the code.

Here is what I would prefer to see:

   1: SomeClass.cpp

   2: -------------

   3:  

   4: SomeClassPtr SomeClass::getInstance()

   5: {

   6:     if (!s_pInstance)

   7:     {

   8:         s_pInstance.reset( new SomeClass() );

   9:     }

  10:  

  11:     // invariants

  12:     if (    !s_pInstance                         // can never be null

  13:             || !s_pInstance->ReadyToBeUsed()) {  // must be in a usable state

  14:  

  15:         // log or assert that there was a problem

  16:  

  17:         s_pInstance.reset ( new NullObject() );

  18:     }

  19:  

  20:     return s_pInstance;

  21: }

  22:  

  23: MyCode.cpp

  24: ----------

  25:  

  26: SomeClassPtr pLocal = SomeClass::getInstance();

  27:  

  28: pLocal->SomeMethod();

 

The above code uses invariants or validations to enforce the contract with the callers.  This code solves every issue the repeated ‘Null checks by the caller’ solves with less overhead and greater versatility.  The summaries below states the reasons why.

Null-Checks by the Caller

Pros

• Easy to implement
• Guarantees that the code does not attempt to use a null pointer

Cons

• Duplicate the null check (if that’s all you check for) every time you acquire the pointer
• Null check does not guarantee a useable object
• Clutters the code (reduced readability and maintainability)
• Increased effort to unit test (multiple systems to test if the system fails to deal with a null pointer vs. a single system)
• It does not enforce the contract (unless the null checks are wrapped in asserts and executed in debug mode), otherwise the code silently moves on
• A null check could be missed by the author and reviewers

Invariants to Enforce the Contract

Pros

• Easy to implement
• It guarantees a usable pointer every time (read the Null Object Pattern)
• No duplication of code / encapsulates the validations
• Less code clutter (increased readability and maintainability)
• Less effort to unit test (less execution paths, only one system to test for a null pointer)
• Frees engineers from performing monkey work (i.e. adding checks for nulls everywhere)
• Makes the contract explicit for future engineers so that they must make the conscious decision to maintain or reject the contract
• Invariants/Validations are always executed

Cons

• Just one…

Cons (common to both approaches)

• Someone could always change the code and break the contract (remove the invariants or the null checks).  This cannot be enforced in the code.

I think I’ll invest in the solution that has more pro’s than cons, providing me with the greatest return.  That’s where I’ll hang my shield.

———-8<—————————————————–

Update:  I wrote the article above a couple days ago when the ‘you are missing a null check’ defect was originally logged.  The team met today to discuss it.  Everyone outlined their points of view, concerns, and goals.  In the end, we agreed that the null check was not needed, and decided to use the solution I proposed.  It was a great conversation, not because we chose my strategy, but because we got a chance to talk software design.  It is something I wish we would do more often.