Quick ‘n Dirty

Programmatically creating symbolic links between folders

I was creating some new folders for upcoming projects.  I got tired of launching a new command prompt and typing in long folder paths for mklink to create symbolic links.  So I whipped up a quick and dirty tool to make this process easier.  Yes it is ugly, but it works so its good enough for now.  If you want the source code and/or the executable, feel free to download the zip of the Visual Studio 2010 project via the link below.

Download the Visual Studio 2010 Project for the “Create Symbolic Link” tool

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)”)

Kicking It Old School

Sometimes, you just got to do it yourself

Have you ever had to resurrect an old code base because the business needs some new functionality added?  Of course you have.  If you haven’t, wait.  If you have, then you understand the pain you have to go through to get the new features added.  The hard part is not adding the features, it is verifying that the changes do not introduce defects in the existing code.  If the code base is old and without a test framework to support it, you are likely stuck with endless manual testing and a few prayers to back you up.

That’s the situation we found ourselves in recently.  We had a new major feature to add and no test framework to support our efforts.  We tried to leverage a few frameworks including Google Test, but found that we were spending a large portion of time troubleshooting incompatibilities with libraries.  Eventually I concluded that we had to build our own test framework.  We wanted to use an established test framework, but all we needed was a way to perform simple state-based, confirmatory tests, so I felt we could get by without one.

I spent a couple hours last Saturday afternoon prototyping and came up with the following code to kick start our framework.  Since I wrote the prototype I have added UI to present the outcomes & smart pointer support, so it looks like this code will grow to fit our needs.

If you are itching to build your own custom C++ test framework, feel free to build off of this code.  I used Visual Studio and the Boost libraries to built it.

 

   1: #include "stdafx.h"

   2: #include <list>

   3: #include <algorithm>

   4: #include <iostream>

   5: #include "boost/ptr_container/ptr_list.hpp"

   6:  

   7: class TestContract {

   8: public:

   9:     virtual std::string GetName() = 0;

  10:     virtual bool RunTest() = 0;

  11: };

  12:  

  13: class SingleTest : public TestContract

  14: {

  15: public:

  16:     std::string GetName() {

  17:         std::string name ("test 1");

  18:         return name;

  19:     }

  20:     bool RunTest() { return true; }

  21: };

  22:  

  23:  

  24: class TestRunner

  25: {

  26: public:

  27:  

  28:     void AddTest(TestContract* test)

  29:     {

  30:         Tests.push_back(test);

  31:     }

  32:  

  33:     void RunTests()

  34:     {        

  35:         for (    TestsIter = Tests.begin();

  36:                 TestsIter != Tests.end();

  37:                 TestsIter++)

  38:         {

  39:             executeTest(*TestsIter);

  40:         }

  41:     }

  42:  

  43: private:

  44:     

  45:     boost::ptr_list<TestContract> Tests;

  46:     boost::ptr_list<TestContract>::iterator TestsIter;

  47:  

  48:     void executeTest(TestContract& singleTest) {

  49:         

  50:         std::string name (singleTest.GetName());

  51:  

  52:         if (singleTest.RunTest())

  53:         {

  54:             std::cout << name.c_str() << ": passed" << std::endl;

  55:         } else {

  56:             std::cout << name.c_str() << ": failed" << std::endl;

  57:         }

  58:     }

  59: };

  60:  

  61:  

  62: int _tmain(int argc, _TCHAR* argv[])

  63: {

  64:     SingleTest* pSingle = new SingleTest();

  65:     TestRunner* pTestRunner = new TestRunner();    

  66:  

  67:     pTestRunner->AddTest(pSingle);

  68:  

  69:     pTestRunner->RunTests();

  70:  

  71:     return 0;

  72: }

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.

Memory Lane

An article back from the grave.

Today I stumbled upon an old article I wrote on our team wiki titled “Project Proposal – A Generic Approach to TDD”.  I had leveraged the TDD formula in a couple earlier projects and this was an attempt at informing my current team how we could leverage it in our current project.  After re-reading it I figured there is some useful content worth posting in my blog even though my opinion on some things have changed since then.  I think it is good enough just the way it it is and does not need to be perfect.  So with a little obfuscation on project names, here it is…

 

Project Proposal – A Generic Approach to TDD

March 13, 2006

Description

The engineering team has tentatively agreed to utilize the Test Driven Development (TDD) methodology in an effort to ensure the quality & testability of the proposed ProjectB enhancements. Now that ProjectA is almost gold it is time for the team to start discussing how it is going to implement this methodology. Once the initial investigation is over, and focus shifts from research to design, it is important that we as a development team have a clear understanding of how we plan to accomplish TDD. In short, to facilitate success, we need to agree on the techniques & tools we will use.

Objectives and Benefits

This proposal is only meant to spur discussion, thought, & ideas about how we will administer TDD. The proposals’ purpose is to drive the team toward a unified understanding of what we will do, & how we will do it. With a collective understanding of TDD we will be successful in implementing it. Not everything that should be discussed is included in this proposal (e.g. test frame work, legacy code test integration, code coverage, error tests, test reviews, etc.).

Development Requirements

Here is my initial listing of our TDD requirements for ProjectB (to be discussed and expanded by the development team):

1. New code needs to be testable via automated unit & integration tests.
2. Old code needs to support automated unit & integration tests.
3. Tests need to be run-able at any time, by anyone on the development team.
4. Test results need to be stored in a repository.
5. When a test fails, the appropriate people must be informed.
6. We want to create a class structure which makes testing as easy as possible.

Definitions:

Unit test: Engineering test which occurs at the class/method level of code.
Integration test: Test which spans multiple classes/methods.

The Approach

When designing a TDD solution from scratch, integrating one in to existing code, or working with legacy code, the approach is the same:

1. Write the test. Watch it fail.
2. Write the code. Watch the test pass.
3. Run the unit test suite. Watch it pass.
4. Refactor. Repeat.

Since the team has not discussed and agreed upon a test frame work (e.g. Boost Unit Test Library), I will not get in to the implementation details of each step.  I would like to mainly focus on step 2 (Write the code).  To some degree, steps 1 (write the test) & 4 (Refactor) will also be touched on.  I can discuss these steps without knowledge of the implemented test suite.

When writing code it is important that we focus on simplicity. Simplicity leads towards understandability & maintainability of the system. We’ve all heard of the ‘KISS‘ principle and its usefulness. KISS becomes critical when automated testing is considered. A complex system yields complex tests, and hence a greater chance for defects to occur both in the code & in the tests.

In order to apply KISS, you need to know what ‘complex’ is. Two methods in a system which may implement similar behavior could be vastly different in complexity. In order for the team to use KISS and tackle complexity we need a way to measure code complexity. Cyclomatic complexity is a software metric designed to measure the general complexity of code. Its goal is to identify code which is ‘complex’ by measurement. For example, a method by default has a CC (Cyclomatic Complexity) of 1. If you add an ‘if’ statement to that method, add 1 to the methods CC. Adding an ‘else’ to the ‘if’ statement adds 1 to the CC. The method now has a CC measurement of 3, and is measurably more complex then a method which has a CC of 1.

Why is CC useful? It has two main benefits. One, it allows the development team to understand which code (new & legacy) is complex and subject to refactoring. Two, it identifies the number of base tests which are required. The CC index is a reflection of the number of code paths through a section of code. Since a properly designed test is supposed to test all paths through code, the CC index is an accurate reflection of the number of tests required to test that code.

Since simplicity leads to maintainability and a reduction in defects, a CC index will allow the team to identify code which is not simple and flag it for refactoring (step 4 above).

Now that we have a technique for identifying complex systems (the ‘what’), the next step is ‘how’. How do we create simple software systems (i.e. Low CC code)? The answer is to create code in which methods and classes are isolated and loosely depended on each other. Following patterns like Strategy & Factory Method will act as a guide.

For example, let’s assume that you have a small method with a CC of 5. This would require 5 unit tests to correctly test the code paths through the method. Using the Factory Method Pattern, the method can be refactored to a CC of 1 and 4 additional classes or methods have been created, which are now isolated, require smaller tests (each have a CC of 1), are easier to understand, and are less defect prone. Applying the Factory Method Pattern to a method with a CC of 5 does not make the benefits apparent, but anyone who has worked on ProjectA code has seen methods which would have CC indexes of 25+. In cases like these, the Factory Method Pattern would make sense. If it’s easier to test, it’s easier to identify and resolve defects.

Conclusion

What I’ve done above is outlined a basic approach to identify and reduce complex code for the purpose of testing. With up to 4 different engineers working in the ProjectB code, simple, loosely coupled code will be a team performance advantage.

Summary

1. Focus on KISS principle.
2. Use Cyclomatic complexity to identify complex code for potential refactoring, & the number of tests required.
3. Create/Refactor in to simple, isolated, fine-grained code using Strategy & Factory Method patterns.

As I stated before, these are few ideas I wanted to use to spur discussions. In my opinion, it would be a good idea to have a TDD plan in place before we start creating engineering designs, which will likely happen next month.

Please provide me with your comments, suggestions, & ideas via email or in person (preferred). I would like to start these discussions in parallel with the creation of our use cases.

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?

The ViewModel Distilled

A MVVM starter project for your consumption.

For the past few weeks, I have spending my evenings learning Silverlight 3.  It has been fun & insightful.  I had experience developing in Adobe Flex, so aside from the syntax it was a smooth transition.  One of the goals I had when I started was understanding how to leverage the ViewModel pattern in my applications.  ViewModel architecturally structures your code in a manner similar to the Model-View-Controller & Model-View-Presenter patterns.  The benefit of using ViewModel is that it capitalizes on the context (client applications) and features (data binding) of rich internet applications.  However learning the pattern was a bit frustrating.  There was a shortage of programmatic examples for implementing the ViewModel pattern in Silverlight.  I found a few for Silverlight 2, and fewer for version 3.

There were a few excellent & helpful resources:

Model-View-ViewModel In Silverlight 2 Apps

ViewModel Pattern in Silverlight using Behaviors

Using Model – View – ViewModel with Silverlight

These sources greatly helped my conceptual understanding & implementation of the ViewModel pattern.  However, each implemented the pattern in slightly different ways using different requirements.  Without a standardized implementation, the learning curve was longer than I expected.

I got through it though (yay!).  I managed to create a couple different applications using ViewModel.  After writing the third application though I was getting tired of recreating the same classes and directory structures.  What I needed was a template.  Something I could open when I needed it and start coding.  Most of the online sources I reviewed were very rich solutions so they did not lend themselves as ‘starter’ projects.  Others did not provide a complete project I could reuse.  So I opted to create my own ‘distilled’ version of the ViewModel pattern in Silverlight 3 using Microsoft’s Visual Web Developer 2008 Express Edition.  The application itself is very basic (it only displays an employee’s name).  But using this project as a template allows me to shave a few minutes of writing whenever I create a new ViewModel-based Silverlight application.

Feel free to download it and tweak it to your desires:

  http://cid-a537fc3c292692a7.skydrive.live.com/embedrow.aspx/.Public/MVVM|_Simple.zip

Some time after I created the template project, I stumbled upon the Silverlight Model-View-ViewModel Toolkit.  It is an open source project designed to solve the problems I was facing when I was learning ViewModel.  The toolkit is in its infancy, but worth the look if you are interested in learning about implementing ViewModel in Silverlight.

“Don’t Worry”

How two words and once sentence made me who I am today.

Ten years ago the team sat down in a boardroom with expectations to plan the next major release of our software.  The entire team was there.  Release managers, business managers, engineers, testers, & documenters.  The business managers gave the “here’s what we’re going to build” pitch.  The release managers outlined the strategy.  Everyone else asked questions until we were clear on the general aspects of the release.

After the preliminaries we were instructed to provide estimates for our forthcoming work.  At the time it made sense.  They needed to know how long the project would take so that they could see if the work could be completed before the expected release date.  One manager stood at the whiteboard and walked through the various features and core components of the product.  For each work item he suggested a coworker who could work on it.  After some discussion, the assigned worker would provide an estimate in number of days.  We would log the estimate and then continue to the next item on the list.

This was the first time I had seen this done.  I was a new engineer to the company, as were most of the engineers on the team back then.  The manager had lots of experience and was guiding the flow of the meeting as seasoned managers should.  In the two years since I joined the company I had never seen this done.  We had meetings before to discuss the plan and how long it would take, but never with the entire team, and at that point, never with such detail.  I remembered I was excited.  I thought “We’re going to do great this time”.

Then something interesting happened.

Eventually it was my turn.  The manager stated that the preferences module needed some work and suggested that I take it on.  I agreed.  It was an area of the product that I had little knowledge of, so I was happy to do the work.  Then he asked “How long so you think it would take”?  I thought about it and asked some questions about the work.  I received answers and then I asked some more.  After a few more minutes of Q&A I could see some people in the room were getting annoyed.  Everyone had provided answers in a few minutes, and I was taking longer.

Eventually another engineer joined the manager at the whiteboard in an attempt to hurry the conversation.  I was resistant to provide any estimate because I had never worked in the code before and was not comfortable given the information I was provided.  I said “How can I give a number for something I haven’t seen before”?  To this day I have never forgotten the reply I received: “Don’t worry.  It doesn’t matter.  Just give a number”.

My heart felt like it hit the floor, everything else in the room faded, my eyes glazed over, and I blurted out a number.

There are two things I remember about that day.  The first was the disappointment in me for providing an estimate for something I knew nothing about.  The second was my reaction.  It was so sudden, vivid, and all encompassing.  Going forward I attributed my reaction to ‘just being angry at myself’.  It would not be until recently that I truly understood why I reacted the way I did.

—

Two years ago, the team was undergoing a new exercise.  We were exploring the thoughts and opinions of each other to understand our strengths, weaknesses, personal brand (how other see us), and our triggers (what enables us).  The goal was to understand each other so that we could improve ourselves and how we worked with each other.  It was a fun series of exercises that opened my eyes to my coworkers.  I learned as much about them as I did myself.  As we explored the subject of triggers, I thought back to that day when I provided my estimate, and after some thought, I finally realized what enabled my response.  It was not the fact that I was disappointed in myself.  It was something more obvious and mundane.  It was the word ‘”Don’t”.

It seems kind of odd that a single word could trigger such an acute response.  Then again, my coworkers described similar responses to things that triggered them.  They would become blinded to other influences, focused on the thing or person that triggered them, and emotionally involved themselves with what caused the trigger.  Triggers are neither good nor bad.  They are simply the things that enable us.  Understanding our triggers helps us understand why we react to certain influences the way we do.  The benefit in our coworkers knowing them is that they gain insight which helps them work more effectively with each other.  We can understand why we do and say the things we say.  In other words, we understand each other.

While I know that “don’t” triggers me, I am not clear on why it does.  My theory is that “don’t” is a limiting word, kind of like a virtual wall.  I am the kind of person who likes to challenge the status-quo and look to new and improved ways of accomplishing things.  Perhaps I see “don’t” as a barrier to be broken.

Overall I find that my triggers are very helpful.  They enable me to find alternative routes, identify root causes, and locate opportunities for improvement.  The down side is that there are days when I encounter so many triggers that I go to bed exhausted.  In my opinion, it is a small price to pay.

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

I have learned that there are three things that instantly trigger my attention: two words and one sentence:

1. Can’t
2. Don’t
3. and people who complain but do nothing

What are your triggers?

‘The’ Lifetime Activity

Why the approach to code reviews is all wrong.

Today we perform code reviews before we check-in to the source repository.  It was not always been that way.  I can remember when code reviews were first formally introduced to the company.  The response from the team was generally skeptical.  Team members were concerned that it would slow down development, not produce measurable improvement, & act as a distraction from other high-priority activities.  I support Demings idea that “an organization does not simply need good people, but people that are improving with education”.  The team came through on this point because today we cannot imagine any team not leveraging code reviews in its day-to-day activities.

The other day I was thinking about our practices regarding code reviews and why we execute them the way we do.  After repeatedly asking myself many questions that started with ‘why’ & ‘how’, it occurred to me that perhaps our perspective on code reviews is narrow, misguided, and thus limiting new opportunities to improve the team & code base.

Allow me to explain.

There are many different types of code reviews:

There is the ‘shoulder check’ where a couple engineers informally review changes in the code and asks questions while providing feedback.  This is the most common type of review in our company.

From time to time we also have ‘formal reviews’, where a group of engineers simultaneously review code as the author walks them through it.  Typically this is reserved for large scale changes in critical areas of the code and is very time consuming.

Occasionally we participate in ‘paired programming’ where code reviews happen on-the-fly as the code is written.  This is the fastest way that I know of to remove defects as they are created.

There are other varying types of reviews, and my goal here is not to list them all, but to provide some insight on their nature.  Code reviews are formal or informal, fast or time consuming, on-the-fly or planned, executed by the individual or in groups.  It is no wonder that when people hear about code reviews for the first time that they are apprehensive.  Depending on what kind of review you practice, your experiences and opinions may be different from your team mates.

Given such variation, I think that the common premise, that code reviews are mechanisms to locate and remove defects, is inaccurate and limiting in perspective.  While the outcome is less defects and improved quality, this is merely a side effect of the true nature of code reviews:

“To learn”

Ask any engineer or manager today what the goal of code reviews is and they will likely respond “to find and remove defects”.  In reality, code reviews are much more than this.  Not only do we want the code to be improved, but we also want the participants to improve as well.  If improvement did not occur, the same type of defects would manifest again and again.  Then, as the code base grows, reviews become longer because the ratio of defects per line of code increases, and the business suffers due to the increased effort to identify and remove defects.

This is not a desirable outcome.  We want those we work with to learn and understand why a change is needed.  This growth improves not only the individual, but also the team, and eventually the company.  People who regularly learn about how to avoid certain errors, will write better code.  People who write better code can pass those learning’s on to others through other code reviews.  Eventually, the network reaches everyone, and productivity is improved.  But to do this, you must have the mindset of a learner and not a teacher.

Teaching is a natural outcome of a learning mindset.  If I approach a code review with the mindset that I will find all of the defects, then I make statements like ‘This code should be extracted in to its own method and because of this reason’.  If I have a learners mindset then I ask ‘Can you tell me about how this code works in this context?’, or ‘Are there any opportunities to refactor this code?’.  The difference is subtle, but powerful.  Instead of directing to the solution I think is the best, I engage in open-ended conversation.  This allows the author of the code to provide his or her perspective, which is different from my own.  In turn I gain insight and understanding, which may or may not, sway my initial assessment of the code.  We exchange comments and ideas, and perhaps the code is changed to remove a defect.  Perhaps my assumptions are challenged when I learn something new.  More importantly, we walk away from the review more knowledgeable than when we started.  One or both of us have learned something that we can carry in to our next code review.

Not all code reviews will be that cut-and-dry.  That isn’t my point.  I wanted to point out the contrast between a review with the aim to find defects, and one aiming to learn from each other.  You will find defects and have the author correct them, but does the author understand how not to introduce the same defect again?  How do you know the author was attempting to express an idea, one that would have taken the code down a different and better path than the one you stated?  Don’t get me wrong.  You will have successful reviews with a ‘find the defect’ mindset.  I’m saying that you and your team won’t be as successful unless you adopt a learning mindset.

Code reviews are about learning, and learning is a lifetime activity.