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.

Shields Up!

Where do you hang your shield?

I spent the last week designing, writing, and testing a new error reporting system for our product.  I spent a lot of time reviewing the design ensuring that I did not introduce any unnecessary dependencies or wrote code that was not required.  While coding I intentionally made one omission to the design while knowingly ignoring one of our team rules.  I then submitted it for review knowing that someone would reject the change, request that the rule be upheld, and log a defect so that it would be fixed.  There will be debate.  Eventually other members of the team will be involved because it involves one of our team rules.  Sides will be picked and arguments for and against will be made.  It will bring to light individual perspectives and debate about what constitutes good design in software.

So what was the single change I made that violated a team rule and opened Pandora’s Box of emotional debate?  I did not check for a null pointer.

Is this a bad thing?  If you are a programmer, I am sure that you have reached your conclusion already.  Of course it is a terrible thing not to check your pointers before you use them.  I absolutely agree.  So would you be surprised that despite the fact I did not check for a null pointer, I did not violate the rule?

Here is some context.

The rule to check for a null was put in to place to ensure that code does not attempt to utilize a null pointer.  If a null pointer was used, the repercussions could not be predicted.  If you did not encapsulate it inside a try/catch, the code would likely crash.  The ‘check for null’ rule shields you from this undesired behaviour in your code.  Check it before you use it.  That makes sense.

The question I pose is where to put the check?  Where should you hang the shield that protects you for the highest return on investment?

Here is an example of the c++ code I was working with.

   1:  

   2: SomeClass.h

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

   4:  

   5: class SomeClass {

   6:     public:

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

   8:         void SomeMethod();

   9:  

  10:     private:

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

  12: };

  13:  

  14: typedef boost::shared_ptr<SomeClass> SomeClassPtr;

  15:  

  16:  

  17: SomeClass.cpp

  18: -------------

  19:  

  20: SomeClassPtr SomeClass::s_pInstance;

  21:  

  22: SomeClassPtr SomeClass::getInstance()

  23: {

  24:     if (!s_pInstance)

  25:     {

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

  27:     }

  28:  

  29:     return s_pInstance;

  30: }

  31:  

  32:  

  33: MyCode.cpp

  34: ----------

  35:  

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

  37:  

  38: pLocal->SomeMethod();

 

SomeClass is a singleton.  This means that only one copy of it can can exist within the system.  When an entity wants to use it, it calls “SomeClass::getInstance()”.  getInstance() checks if it has a pointer to an instance of SomeObject.  If not, it creates one.  Then it returns the pointer to the caller.  It is important to note that getInstance() will never return a null pointer.  The check “if (!s_pInstance)” enforces that s_pInstance can never be null when returned to the caller.  So I do not need to check if pLocal is null before calling SomeMethod().

It is important to note that all of this code is under the control of the team.  SomeClass is not some external dependency on a third party SDK.  We wrote it and maintain it.

Yet the argument was made that the ‘check for null’ was still needed even though getInstance() would never return a null pointer.  This is what was being asked for:

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

   2:  

   3: if (pLocal) {

   4:     pLocal->SomeMethod();

   5: }

 

The argument for adding the null check is two fold:

1. It is easy to add so why not just add it.
2. It makes your code more defensive because you cannot trust what getInstance() returns.

Something that is easy does not mean it is the right thing to do.  I agree it is better than not checking the pointer, but there is the perception that adding it is a low cost, easy win.  This is a perception I believe to be false.

Let’s explore the cost around the rule to have the caller always check for null with a couple scenarios.

 

Scenario 1:  “getInstance() never changes to return a null”

Let’s assume that getInstance() does not change for the life of the product.  This means:

• We spent time adding null checks each time getInstance() was called, even though they were not necessary
• We slowed code reviews when someone forgot to add a null check because we had to enter a defect for it.
  • The author then spends time making the change, retesting it, and resubmitting it for review.
  • This delays software being sent to QA to validate the engineers modifications.
• Most importantly, the null-check adds no value because getInstance() already guarantees never to return a null pointer.
• The code is not more readable because of it.  Extra null checks adds to the code you need to review when troubleshooting and making changes.
• It has become more complex because the null check adds a new execution path through the code.
• Unit tests are more complex because to ensure coverage you need to include the test if getInstance() returns a null.  Even though we know that getInstance() does not.

Scenario 2:  “getInstance() is changed and potentially returns a null”

Now let’s assume that someone does modify getInstance() several years from now to potentially return a null pointer.  Will the code crash if the caller does not check the response from getInstance() for a null.  It is absolutely possible.  You know what?  That someone could have done many things to SomeClass.  Anything is possible.  SomeClass could have been refactored in to a bridge pattern for a remote service for all we know.  Perhaps someone did modify getInstance() and break the contract it had with those entities that depended on it.  But someone could also leave getInstance() alone and remove the null checks too.  The callers to getInstance() are not immune from code refactorings.

The problem with someday/maybe arguments is that they are speculative & presumptuous.  You can’t make the argument that someone will do something to only one part of the code and ignore everything else.  Anyone with an opposing viewpoint could make the same argument and have equal weight.  Someday, someone might change ‘this’ code, and not that code.  It may not be caught in code reviews, QA regression tests, or beta tests.  It could make it in to production where a user might receive a null pointer and crash the product.  Sure it is possible and a null check would catch it potentially averting a crash.  Sure the code that uses the pointer does not execute and crash, but what about the objects and systems that depend on SomeClass performing its responsibilities?

Someone, someday willingly alters the code and breaks the contract in a way that might result in a null pointer, and the only way to defend against that person is check for a null pointer.  That argument doesn’t hold water for me.

Someone altering the code is not something you can defend against.  Why?  Maybe that someone is the same person modifying the caller.  Maybe someone goes through the code and strips code they feel is unnecessary and that they do not want or need to maintain.  Don’t laugh.  I’ve met several individuals and teams that have done just that.  The argument “someone/someday” is far to vague and hypothetical.  Picking one practice over another because ‘someone/someday’ might do something bad is not a good foundation to base a practice on.  Remember, in this case we are not writing an interface or an SDK where the inputs are completely foreign and unknown.  This is a cohesive, stand-alone product that we control.  We know how it works.

Ok, lets run with the argument that ‘someone/someday’ will change SomeClass so that getInstance could return a null pointer.  Lets also assume that SomeClass now has a newly introduced internal state that needs to be checked before it can be used.  For example, SomeClass now calls a factory to retrieve a pointer to a helper class.  Without the helper class, SomeClass cannot fulfill its responsibilities.  Lets’ assume that the caller also checks for a null pointer:

   1:  

   2: SomeClass.h

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

   4:  

   5: class SomeClass {

   6:     public:

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

   8:         void SomeMethod();

   9:  

  10:     private:

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

  12:         ObjectAPtr m_pObjAPtr;

  13: };

  14:  

  15: typedef boost::shared_ptr<SomeClass> SomeClassPtr;

  16:  

  17:  

  18: SomeClass.cpp

  19: -------------

  20:  

  21: SomeClassPtr SomeClass::getInstance()

  22: {

  23:     if (!s_pInstance)

  24:     {

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

  26:     }

  27:  

  28:     return s_pInstance;

  29: }

  30:  

  31: SomeClass::SomeClass() {

  32:     m_pObjAPtr = GetFromFactory();

  33:     assert(m_pObjAPtr);

  34: }

  35:  

  36:  

  37: MyCode.cpp

  38: ----------

  39:  

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

  41:  

  42: if (pLocal) {

  43:     pLocal->SomeMethod();

  44: }

In the SomeClass constructor, it acquires a pointer, stores it in m_pObjAPtr.  Following the ‘check for null’ rule we assert if it is null.  If it is not a release build of the product, the assert will trigger and the user will be notified of the problem.  In a release build, asserts are typically turned off.  In either case, the code could continue to execute.

Here’s the problem.  There are now two conditions to check for a valid instance of SomeClass, the null checks for s_pInstance & m_pObjAPtr.  s_pInstance is already checked by the caller and getInstance().  Here’s my question.  Where would you add the check for m_pObjAPtr?  Remember that the purpose of getInstance is to return a usable instance of SomeClass and the caller cares that SomeClass is usable because it is also validating what getInstance() returns.

(Continued in the next article:  “Shields Up! (continued)”)

Worst Error Detection Mechanism – Ever

When you don’t trust your code, something is wrong.

Take a look at this code:

   1: SomeClass* pSomeObject = GetInstance();

   2:  

   3: if(pSomeObject == NULL)

   4: {

   5:     DisplayErrorMessage();

   6:     return;

   7: }

   8:  

   9: pSomeObject->DoSomething();

I see this code this all the time.  I also admit to writing code like this (insert frown here).  You’re probably looking at it and thinking, there is nothing wrong with it.  Depending on the context you may be right.  Generally, this code just does not feel right to me.

Let me paraphrase the code:

   1: // Get me an object

   2:  

   3: // If the object is not what I wanted, inform about the error and leave

   4:  

   5: // Do something with the object

So the questions I have to ask are:

1. Why does the code think it would receive an invalid object?
2. If the code is concerned about invalid objects, why is this code only checking for a NULL?
3. Why does the code need to know if the object is valid?

Here are some possible answers:

1. The check is to ensure that certain code is not executed if the object is not capable.
2. If the object reference is not NULL, it is reasonable to assume that the object is valid so no other check is needed.
3. If the object is not valid, the code should not use it.

All are valid answers, but there is a better way.  One that streamlines your code and relieves it from the burden of validating what other code provides.  Take a look at the following:

   1: SomeClass* pSomeObject = GetInstance();

   2:  

   3: pSomeObject->DoSomething();

This code will still run without crashing even though I removed the NULL pointer check.  How?  You have probably guessed that the magic happens inside GetInstance.  Instead of GetInstance returning an potentially unusable instance, it guarantees a usable object everytime, even when one is not available.  Here is what it does:

   1: SomeClass* GetInstance() {

   2:  

   3:     if (pClass && pClass.IsValid()) {

   4:         return pClass;

   5:  

   6:     } else {

   7:         return pNullClass;

   8:     }

   9: }

SomeClass is a common interface for pClass and pNullClass.  This hides the concrete class from the code that is calling GetInstance.  pClass is the reference returned when the class has determined that it is ready to be exercised (i.e. IsValid()).  This increases the coverage on validating the object because its validation does not expose its internal state, but it can review everything it encapsulates.  GetInstance also checks if the pointer is NULL.  These checks only need to occur once, and no external code needs to worry about these validations, improving the cohesion of the code base.

Lastly, and this is where the magic happens, when pClass is not valid, a separate instance of a unique class is returned: pNullClass.  The purpose of this class is to do nothing, literally.  It exposes the same interface as pClass, but it has no functionality.  This way, if code calls a member of this class, there is no crash or invalid behaviour because the class is still valid.  If you really wanted to, you could log an error or notify the user of the problem when the class is constructed.

This is the “Null Object Pattern” in combination with a factory of some kind.  It is easy to implement and makes the kind of error handling mentioned earlier, easier to maintain.

There is some cost in taking this approach.  You have to create and maintain the Null Object.  Anytime the interface changes, this class must also change.  One more class you need to maintain in your code base.

In my opinion, the benefit outweighs the cost.  All code that calls the factory no longer has to worry if a valid object will be returned.  The dependency is removed, making that code easier to maintain.  It may be one more object, but it is easy to maintain, and simplifies the rest of your code base.

Bottom line.  Don’t push the responsibility of validating what you produce on those who need it.

The Beginning & the End

How does a developer follow your specification?

We’ve all have been there.  Endless chains of code.  We have no idea of where it started and where it will stop.  You have that sense of being lost.  The problem is not necessarily with the size of the implementation, but with understanding the high-level steps through the code.  If you ever have found yourself in the debugger for hours try to locate that elusive defect within a massive code base, you know what I mean.

Often the approach to finding the code of interest is a process of elimination.  You ask yourself “Is this what I am looking for?”  You review it, and if it does not meet your criteria you move on to the next likely candidate.  If it met your criteria, you then drill down in to the code asking the same question over and over again until you discover what you are looking for.

This blind search eats up your time, it costs businesses money, and it delays efforts on other work products.

There are several techniques you can use to mitigate this problem:  refactoring, behavior-driven-development, etc.  I want to outline a technique when you are designing code for your system.  It involves informing the developer about the shortcuts s/he can take.

When you are presented the option to create software using a user stories or requirements, you often break down the problem in to its high level entities and their interactions.  For example, if you are building a payroll system and you need to write code to e-file paycheques you have several possible entities:  Employee, PayCheque, Aggregators for paycheques & employees, etc.  Once you identify them, you would relate each entity in the system to each other.  This effectively produces a logical representation of the problem domain you are trying to solve.

What it does not do is specify the flow of the user story or requirement through the system.  You could represent this in UML, but like documents, over time UML diagrams can get out of step with the code.  The problem of understanding flow may not seem relevant when you are the author and implicitly understand the intricacies of what you designed.  However when you move on to another project and someone needs to fix a defect in the code you wrote, the problem becomes apparent.

The next opportunity you have to design the representation of a user story or requirement, I would suggest that you design a class or method, that only delegates, where the entrance and exit of your specification is clearly outlined.

Imagine the following system:

class foo
{
    void Process()
    {
        // start the work
        DoMoreStuff();
    }
    
    void DoMoreStuff()
    {
        DoYetMoreStuff();
    }
    
    void DoYetMoreStuff()
    {
        FinishUpTheWork();
    }
    
    void FinishUpTheWork()
    {
        // finish up the work.
    }
}

Now imagine that you have a defect you need to find.  Where do you begin?  If you know the system, you may remember that Process() is where you need to start.  For someone who does not understand the system, they may start at DoYetMoreStuff() because that is where the defect happens to manifest.  If that person does not find the error there, s/he needs to follow the call stack to find the source.  In the above code, this is a very simple activity.  Now imagine a specification in your product that involves tens of classes and ten times more interactions between them.  The complexity of the problem increases greatly.  How fast do you think you would find the defect then if you had no current knowledge of the code?

Unit tests may help find the source of a defect, but not all changes are defects.  They may be enhancements to existing systems, or perhaps the developer needs to review some code in order to understand it.  Without a clear entrance or exit for the specification in your code, the developer is left to his own devices to find what s/he is looking for.

Compare the same code using a method that encapsulates the specification: 

class foo
{
    void Process()
    {
        // start the work
        DoMoreStuff();
        DoYetMoreStuff();
        DoYetMoreStuff();
        FinishUpTheWork();
    }
    
    void DoMoreStuff()
    {        
    }
    
    void DoYetMoreStuff()
    {        
    }
    
    void FinishUpTheWork()
    {
        // finish up the work.
    }
}

It is far easier for someone to start at Process() and eliminate components that do not need to be observed.  The benefits using this technique are:

1. A single entity is responsible for fulfilling the specification
2. It is easier to run automated tests to validate the specification
3. New developers can be directed towards a single point of entry and systematically eliminate entire sections of the process when locating functionality

I will be honest.  I do not think that this can be done in all cases.  Like many things in software development your coding standards, the condition of the existing code, your company’s best practices, and your experience all influence your designs.  I will state that this is an idea worth your time exploring the next time you create or edit code.

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

How would you make a specification clear in your code?  Where does your specification begin and end?  Would this technique work for you?  an you leverage unit tests?  What about a combination of unit tests and this technique?

Sigh

No other word fits what I am thinking right now.

I always crack a smile when I hear someone say, “It’s a small change, why did it take so long?”  I smile because it is an opportunity to discuss the difference between good and poor designs in software.

More often than not, the problem stems from the roles and responsibilities between objects within a software system.  Occasionally I do come across those rare snippets of code that are easier to digest.

I came across the following function recently.  Another engineer wrote it several years ago.

   1: Boolean IsSpecialChar (TCHAR ch)

   2: {

   3:     Boolean    bRet = TRUE, done = FALSE; 

   4:  

   5:     while(!done) {

   6:         bRet = TRUE;

   7:         if(ch == '*') {

   8:         } else if(ch == '!') {

   9:         } else if(ch == ':')  {

  10:         } else {

  11:             bRet = FALSE;

  12:             done = TRUE;

  13:         }

  14:         if(bRet) {

  15:             done = TRUE;

  16:         }

  17:     }    

  18:     return bRet;

  19: }

Here are the major & minor issues I see:

1. Returns a Boolean: Why return a Boolean data type for a simple true/false condition?
2. Accepts a single character as a parameter:  Why force each caller to iterate through the string?  The caller should just ask the question: “Are there any special characters in this string?”
3. Unclear local variables:  What does ‘bRet’ mean?  The return value from the method?  A value returned from a dependent method?  Does it really mean ‘return’?  I have to read the entire function to understand its purpose.
4. A while loop is used for a single character:  Ok, this does not make sense at all.
5. Difficult to read:  It does not have a nice, straight flow.

Here is the revised version I created using our custom string class as a container.  It is easier to read & understand, and thus easier to maintain.  A change in this code will not take as long as the old code.

   1: bool ContainsSpecialCharacters (const Custom::String& targetString)

   2: {

   3:     if (    (targetString.Find("*") == -1) &&

   4:             (targetString.Find("!") == -1) &&

   5:             (targetString.Find(":") == -1))

   6:     {

   7:         return false;

   8:     }

   9:  

  10:     return true;

  11: }

Pay Down Your Debt

“The most powerful force in the universe is compound interest” – Albert Einstein

One hard lesson I have learned as an engineer is that technical debt pays in spades. If you continually modify a software system without paying down your technical debt, eventually you will have code that is difficult to maintain, unreadable, & a royal pain to work with. If you continually ignore the problem, you risk accumulating so much debt that it becomes very costly to correct the code.  Eventually the only option is to scrap the code and start from scratch.

Refactoring code periodically is one way to reduce technical debt. Refactoring is the activity of “modifying code to improve its structure or readability, while preserving its meaning or behavior” (thank-you Wikipedia). It is not a single technique, but a collection of techniques to remove technical debt. Think of it as a catalogue where you browse problem patterns & then read up on how to correct them.

It is important to embrace the practice of refactoring.  You will come across code that smells. The question is what you are going to do about it.

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

If you do plan to refactor code, keep in mind that refactoring does not dismiss professional judgement. You as the creator or maintainer of code need to ask yourself the following before refactoring:

• Does the refactoring solve a specific readability or structuring defect?
• Will the refactoring cause issues when merging code?
• How long will it take to refactor this code? Do I have higher priority items to deal with?
• Will I be able to fully test the refactoring before my code review?

Child Driven Design (CDD)

The connection between raising children & object oriented design (OOD).

Those of you who have experience raising children will relate to this article.  Those of you who have no experience, read on, the point I make will improve your designs.

I love my kids.  They make me laugh when they point out the silly things we adults do.  I marvel at their memory when they recall significant details of events which occurred years ago.  Their smiles & hugs cheer me up every time.  And when they snuggle next to me while we watch a movie, they remind me how much I cherish them.

I want to be a great father, so as they experience life I want be there to witness it & guide them when needed.  While I like to be involved, there are times when I step away and let them figure things out for themselves.  Dory from the movie "Finding Nemo" had the perfect saying for this "If you never let anything ever happen to him, then nothing would ever happen to him".

Sometimes my kids encounter a new problem and try to figure out how to solve it.  Like getting that toy under the couch which is out of arms reach, or practicing putting on their new shoes.  As long as they are focused & not getting frustrated, I stand back and let them work through the problem.  In some ways, Object Oriented Design (OOD) parallels raising children; specifically, the relationship between the parent and child sometimes mirrors the relationship between a software engineer and an object class.

When you create a class, you endow it with certain capabilities because the class needs to fulfill a role in your software ecosystem.  Over time, your class continues to evolve and its role becomes clearer and its character becomes more pronounced.  Eventually, when your class matures enough, you send it out in to the world to fulfill its destiny.

If, however, your class is highly dependent on other code to function effectively, then your class will not have the independence it needs to flourish and succeed.  Classes which are highly coupled to other classes are easily affected by changes in those classes, and vice versa.  You want your class to be independent & have relationships, not dependencies, with other code. If your class is highly coupled, then you need to refactor it and give the class what it needs: sufficient knowledge, skill, & intelligence so that it does not depend on others to fulfill its existence.

By the way, the idea for this article came when I witnessed the following code snippet.  It has been altered to protect the innocent:

UpdateVariableInTheObject(GetTheObject(), TheNewInformation);

You can see that the object relies on other code to fulfill its existence.  The class should instead control itself, like this:

GetTheObject()->UpdateVariableInTheObject(TheNewInformation);

Now the class has the tools to solve its own problems.

Getters Harmful?

Is the benefit of using a getter, greater then the potential to violate the ‘Tell, Don’t Ask’ principle?

"Tell, Don’t Ask" (TDA) is an object oriented design principle which states that one object should tell another to take an action, rather than asking for information from that object to make a decision.

Here is a trivial before & after example of TDA in action:

 

// --------------------------------------------------
// Before

// get the object
TheListElementType ListElement = ObjectWithAnObjectList.getListItem(indexOfTargetElement);

// determine its state
if (IsValid(ListElement))
{
     // decide to take an action
     ListElement.DoSomething();
}

// --------------------------------------------------
// After applying 'Tell, Don't Ask'

// let the object take the action if the element is valid
ObjectWithAnObjectList.DoSomething(indexOfTargetElement);

TDA is a great way to hide implementation details & ensure that object responsibilities are well segregated.  In the ‘before’, it is obvious that the code is not orthogonal.  If ObjectWithAnObjectList changed its list type, then this code may also have to change because it acquires a copy of a ListElement.  In the ‘after’ code, this problem is not present.

I was reading up on TDA today when it occurred to me that getters could potentially violate this paradigm.

A getter is an accessor method that provides public access to a private member of an object.  For example: 

private boolean member;

public boolean getMember (void)
{
     return member;
}

Using a getter is an excellent technique to control access to private information without directly exposing the member variable.  However, under TDA, the value that a getter provides is reduced.

Getters expose the state of private members, and force responsibilities of the state on to other objects.  An object that leverages the getter of another is coupled directly to that object.  With this dependency, when the type of the member variable is altered, the getter, and all objects & methods that leverage it, may need to be changed as well.  To properly handle the response from the getter, the object which calls the getter also needs to understand all of the states of the member variable.  This does not feel like encapsulation, does it?  In my opinion, you can maintain greater encapsulation & loose coupling if you do not expose the getter in the first place.

Getters have their place in object oriented design, but abuse of getters fundamentally breaks the ‘Tell, don’t ask’ design principle.  Try to reduce your consumption of getters.  If you need to use a getter, make sure it is for the right reasons & does not violate the TDA principle.

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

If you find yourself writing a getter, ask yourself the following:

1. Is it for public or private consumption?  If you do not know what ‘private consumption’ is, see Self Encapsulating Field.
2. If it is for public consumption, are you pushing responsibilities out to a foreign object whose role already encompasses different responsibilities?  If so, then perhaps you need to revisit the roles of the objects you are dealing with.  Perhaps you are missing an additional object.  Perhaps it should be a tell & not an ask.
3. Is the state of the member variable important?  If so, then why is another object or software system handling it?  Perhaps telling the object to check for a certain state is a better solution. 

Problems are Good

Dependency Injection may lead to problems, which is not a bad thing.

I was reading my previous blog post on ‘Post TDD‘ and it triggered a memory of a conversation I had with someone (I do not remember who) on Dependency Injection.  I was evangelizing the need to inject an objects dependencies via the constructor.  The idea was to control the objects dependencies so that you could test the object in isolation.  This would ensure that we could use mock objects to trigger specific behaviors in the object of interest and test execution paths that would normally be difficult without that level of control.  Additionally, supplying the dependencies also ensures that the object is immediately in a usable state once construction is completed.  Code that calls the constructor does not have to guess that the object has everything it needs.

The rebuttal to that argument was a concern over bloating the objects constructor. Using Dependency Injection would produce a very long-winded command to create the object.

This is a valid concern.  You definitely do not want to create an object constructor, or any method, that requires many parameters. Any message to invoke behavior in an object should be simple & well defined.  You should not have to pump in a lot of information to make it work.  If you do, then the object is likely heavily dependent on other objects or software systems, which makes the object very difficult to replace, test, or maintain over time.

So what do you do if a message to an object requires many parameters?

First, determine if the information passed to the object is truly needed.  If the object is acting as a go-between, and simply passing the information to another object, question why.  Perhaps the data could be passed directly to its destination.

Second, if the information is truly needed, then see if there are ways to consolidate the information.  For example, I have seen code whose sole purpose is to display a message to the user, but every snippet of data is broken out (message group id, title id, message id, etc.).  Why not consolidate the information in to a bite-sized package (e.g. MessageInfo struct)?  While the example is trivial, it does scale up to objects & interfaces.

Finally, if you have scaled back & consolidated the information, and still the data injected is more numerous then you would like, ask if the object, method, or function is performing too many tasks.  There may be an opportunity to refactor it in to smaller chunks.  This could be your first step if it is obvious that refactoring is needed.

Back to my point about using dependency injection leading to more problems (e.g. long-winded constructors), and that not being a bad thing.  By consolidating all of the dependencies in to the constructor, you may quickly observe that your class has too many dependencies on other objects or software systems.  In this case, using Dependency Injection did not create a new problem, it revealed it.  Now that you know the problem exists, you have an opportunity to correct it.

Return diamonds, not coal!

The following code makes me shutter:

boolean Foo ()
{
    // do stuff
    return true;
}

int Foo2 ()
{
    // do stuff
    return 1;
}

I have long been a proponent of returning status from a method, but the code above is useless.

The methods above have no business returning ‘true’ or ‘1’, because these responses are meaningless.  You may be thinking that the answer is obvious: the return values are static and are not subject to change.  If there was an opportunity for variability, like returning ‘true’ or ‘false’, then the returned boolean would have meaning.  I would argue that this is only a partial solution.

Let me explain.

Returning ‘true’ or ‘false’ is a useless activity.  Keeping with the theme, ‘-1’, ‘0’, & ‘1’ are also useless.  Obviously this does not apply to methods whose responsibility requires that they return a numerical value (e.g. int GetCurrentCount()) or a boolean result (e.g. boolean IsThisEnabled()).  This argument applies to those methods that return numerical or boolean values in place of status.

Let me ask you a simple question:  what does ‘-1’ mean?  Your response may be something like ‘nothing’, ‘not found’, ‘ not initialized’, or ‘error’.  That is my point, it has many meanings.  You have to look at the context under which it is used to understand its meaning.  This on-the-fly translation leads to complexity because the reviewer must remember its meaning under the context of the code s/he is reviewing.  Since ‘-1’ depends on the context, and the meaning of ‘-1’ could be different between coders, the meaning of ‘-1’ must be constantly re-interpreted by the person reviewing the code.

Now this may seem like a frivolous argument to you.  You may be thinking, ‘so what’.  We will just standardize ‘-1’ to mean ‘error’.  You may decide to define a macro to encode the meaning, and use the macro instead of the value (e.g. #define ERROR -1;).  But this mindset is akin to ‘putting lipstick on a pig’; no matter how much you dress it up, you still have the same problem: ‘true’, ‘false’, ‘-1’, ‘0’, & ‘1’ are meaningless when used to reflect status.

Time for a thought experiment.

Let us say you have the following code:

 

if (ERROR == pClass->Method())
{
    // handle the error
}

public int Class::Method()
{
    // do stuff

    return ERROR; // where ERROR is defined as -1
}

This code works.  There is no functional problem with it.  To see the problem you have to look beyond the function of the code & interpret the philosophy behind it.  The philosophy is "Class::Method encountered an error, so you handle it".  This is not a good design.

To understand why, think about how you would handle the error.  Without any additional information about the error, you are left with a generic handler for the problem.  Sure, you could log it & inform the user.  Perhaps you decide to stop whatever processing you were doing, clean it up, & exit.  However, ask yourself, did you truly handle the error?  In reality, you did not.  You only mitigated it.  Mitigating the error creates a situation where depended code is tightly coupled to all potential responses provided from the method.

Without context, you cannot properly handle the response.  There is not enough meaning behind the responses to have dependent code properly handle what is receives.  This issue becomes evident when the method could potentially respond with a second return value.  Are you willing to go back to every call to ‘Method’ and double check if the new return value needs to be handled?  If a new response is added, and you do not update the dependent code to handle it, you are ignoring potential responses which dependent code should handle.  The dependent code remains ignorant and assumes everything is fine.

Return values should not force the caller to interpret the response. 

Here is how the code should read:

 

pClass->Method();


public void Class::Method()
{
    // do stuff

    if (an_error_occurred)
    {
        // handle the error
    }

    // -- or --

    AnErrorOccurred:
        // handle the error
}

‘Handle the error’ means whatever your environment will allow.  It could be as simple as logging the error and throwing an exception.  Adding code to handle the error at the point of failure, where there is context for the error, results in the following:

Result	Benefit
Clear indication of where the failure occurred	Easier debugging since you now know where to look
Error handling is contained under the context which the error occurred	Clarity of the problem due to more available information Dependent code does not need to process & potentially misinterpret the response from the method Removes the potential for the response to be ignored by dependent code
Code which depends on the response does not need to be updated when the context changes	Less likely that the program will continue operating under the assumption that everything is ok Do not have to update dependent code whenever the response changes or a new response is added

Though my example focuses on returning an error, the philosophy applies to anything that could be used as a response, especially with ambiguous information like a boolean or number. 

Using this approach you refine the information internally before concluding that a response is needed, which leads to meaningful, valuable responses (i.e. diamonds).  Dependent code will not need to interpret a vague response without context (i.e. coal) to make a decision on what to do next.  The philosophy of the design has changed from "I got a response, what does it mean" to "I got a response, what action do I need to take".  The difference is subtle but powerful.

————–

If you find yourself in a method that has a response, ask yourself the following:

Q1: Does it need to respond with anything at all?

Q2: Can you handle it internally?

Q3: Will the next engineer or software system, which relies on the code, always understand the definition of the response?

Q4: Will dependent code need to be updated to handle a new or modified response? 

Q5: Can you handle it internally? (had to ask this twice)