Skip to main content
Source Link
atconway
  • 409
  • 1
  • 4
  • 12

I think there are a lot of pitfalls to Example 2, that down the road could lead to unintended code. 1st off the focus here is based on logic surrounding the 'myString' variable. Therefore to be explicit, all testing of conditions should happen in a code block accounting for known and default/unknown logic.

What if later on code was introduced unintentionally to example 2 that altered the ouput significantly:

   if (myString == null)
   {
      return false;
   }

   //add some v1 update code here...
   myString = "And the winner is: ";
   //add some v2 update code here...
   //buried in v2 updates the following line was added
   myString = null;
   //add some v3 update code here...
   //Well technically this should not be hit because myString = null
   //but we already passed that logic
   myString = "Name " + myString;
   // Do something more here...
   return true;

I think with the else block immediately following the check for a null would have made programmers that added the enhancements to the future versions add all the logic together because now we have a string of logic that was unintended for the original rule (returning if the value is null).

I lend my belief in this heavily to some of the C# guidlines on Codeplex (link to that here: http://csharpguidelines.codeplex.com/) that state the following:

"Add a descriptive comment if the default block (else) is supposed to be empty. Moreover, if that block is not supposed to be reached throw an InvalidOperationException to detect future changes that may fall through the existing cases. This ensures better code, because all paths the code can travel has been thought about."

I think it is good programming practice when using logic blocks like this to always have a default block added (if-else, case:default) to account for all code paths explicitly and not to leave code open to unintended logic consequences.

Post Made Community Wiki by atconway