Less Is More

by October 25, 2008 01:37 PM

As Bob Koss (on Object Mentor) puts it in Size Matters:

classes are just too damn big and methods are just too damn long

I couldn't agree more! I was recently tasked with adding functionality to an existing website at work. The point in the process where I was to add functionality was in the middle of this nasty page where there were multiple panels that were just hidden or shown depending on where you were in this mini process flow. The aspx was over 500 lines long and the code behind over 1,000! I cried inwardly when I realized the desired functionality was right in the middle of this mess. I did some soul searching and analysis and determined that splitting this mess into multiple pages wouldn't take much longer than figuring out how to add to the existing page without breaking anything.

As I was breaking the page into multiple pages, what Jeff Attwood said in Size Is The Enemy really hit home.

Lines of code are, and always have been, the enemy. More lines of code means more to read, more to understand, more to troubleshoot, more to debug.

The code was extremely difficult to decipher. Not only was so much functionality packed into one page, each method was crazy long and overly complex. At the end of this post is one such method along with how I refactored it after moving it into the new page I created for that piece of the puzzle. I hope you'll agree that the refactored version is much easier to read and understand quickly. And that's the key - to add to a code base without breaking it, you have to be able to understand it and I want to do that quickly. When an analyst or a user says "it's only one new button" or some such nonsense, I want them to be right! Nothing frustrates me more than when a task that seems like it should only take a day or two actually takes a week or two and it's because of the unnecessary complexity left by the yahoo coder before you.

A couple of things about the code below. First, don't bother trying to grok the pre-refactored code - it's just there to note. I assure you I tested and the shorter version captures exactly the same thing as the the longer. Second, one might be tempted to say the shorter version is only better if you understand lambdas and LINQ. To that I say, true. But consider this. A new developer coming on who doesn't know them will only have to learn them once and will then be capably of understanding all the rest at first glance. Doing it the old-school looping, if-else, and temp variables way means the poor developer will have to decipher the craziness over and over again. And each and every time is a lot of unnecessary time added to that simple button request.

So, to summarize: Less Is More!! Make every attempt to accomplish your programming tasks in as few lines of code as possible (short of obfuscation of course!). Always keep it in your mind how difficult it would be for the programmer coming along behind you (that coder might be you months or years later).

Happy coding!

 

The code:

Before I refactored:

private int GetDfuCount()
{
   int maxCount = 0;
   ArrayList systemDFUList = new ArrayList();

   foreach (DFULibrary.DFU userItem in GetUserDfuList())
   {
      if (userItem.Exists == false && userItem.Staged == false)
      {
         if (!userItem.BusinessUnit.Equals("CASE READY BEEF-PORK"))
         {
            maxCount++;
         }
         else
         {
            if (userItem.Reservation == true)
            {
               maxCount++;
            }
         }
      }

      foreach (DFULibrary.DFU systemItem in userItem.SystemDfuList)
      {
         if (systemItem.Exists == false && systemItem.Staged == false)
         {
            bool match = false;
            if (!systemItem.BusinessUnit.Equals("CASE READY BEEF-PORK"))
            {
               foreach (DFULibrary.DFU listItem in systemDFUList)
               {

                  if (systemItem.Product == listItem.Product &&
                     systemItem.Location == listItem.Location &&
                     systemItem.DfuLevel == listItem.DfuLevel &&
                     systemItem.SellingGroup == listItem.SellingGroup &&
                     systemItem.DriverLevelLoc == listItem.DriverLevelLoc)
                  {
                     match = true;
                     break;
                  }
               }
               if (!match)
                  systemDFUList.Add(systemItem);
            }
            else
            {
               if (userItem.Reservation == true)
               {
                  foreach (DFULibrary.DFU listItem in systemDFUList)
                  {
                     if (systemItem.Product == listItem.Product &&
                        systemItem.Location == listItem.Location &&
                        systemItem.DfuLevel == listItem.DfuLevel &&
                        systemItem.SellingGroup == listItem.SellingGroup &&
                        systemItem.DriverLevelLoc == listItem.DriverLevelLoc)
                     {
                        match = true;
                        break;
                     }
                  }
                  if (!match)
                     systemDFUList.Add(systemItem);
               }
            }
         }
      }
   }
   return maxCount + systemDFUList.Count;
}

After I refactored:

private int GetDfuCount()
{
   Func<DFULibrary.DFU, bool> userDfusToCount =
      x =>
         !x.Exists && !x.Staged &&
         (x.BusinessUnit != "CASE READY BEEF-PORK" || x.Reservation);

   Func<DFULibrary.DFU, bool> sysDfusToCount =
      x =>
         !x.Exists && !x.Staged &&
         (x.BusinessUnit != "CASE READY BEEF-PORK" || x.UserDfu.Reservation);

   int userDfuCount = UserDfuList.Count(userDfusToCount);
   int sysDfuCount = SystemDfuList.Count(sysDfusToCount);

   return userDfuCount + sysDfuCount;
}

Tags: ,

Comments (4) -

Jeremy Sharp
10/26/2008 4:54:31 AM #

UGGGGG that code looks familiar Frown... Nice work and excellent blog!

**Could we mention that the app was originally written 4 or 5+ years ago[not that that makes everything ok , but we live and learn Smile ]  

I think I owe you a COLD beverage for making that one better!(cant wait to pull it up and see your handy work). All to often we get sucked into minimizing our support time and just “make it work” mentality… You have inspired me to rethink how much attention I give my support task's… The question When to refactor VS. Fix! Something to think about.

Anyway very nice and as always thanks for sharing! *Ohh yeah & sorry you had to work on that app Smile!!!!!!
-J

Rob
10/26/2008 6:11:03 AM #

You wrote that code?! Holy crap that is funny.

In your defense, the original rendition written by you was years ago. Since then, there have been multiple junior level programmers assigned to the project. I got the feeling from talking to the product owner that they are the ones responsible for the bloating of the one page mentioned above.

I'll still take the cold one though Smile

Jeremy Sharp
10/26/2008 12:10:04 PM #

SmileSmile

Anytime!

SmileSmile

Jay
2/18/2009 3:45:33 PM #

Less is more and we all know the famous quote: "It seems that perfection is reached not when there is nothing left to add, but when there is nothing left to take away."

I have to point out you're comparing .NET 1.x code to 3.x code.  You made use of lambda expressions that weren't present in .NET several years ago.

The first set of code while rather wordy and long is certainly readable, maintainable, and can be easily debugged.  More than likely the developer didn't have the leisure of creating "throw away" code to come back and re-factor at a later time.  I often find myself having "geez, why did I do that?" moments several months later.  But the answer is simple; I had to get it done; the business partners don't care if my code is elegant.  

I should also point out that  for me lambda expressions don't just roll off the keyboard naturally.  It's usually something I do in hindsight.  The price of living in corporate IT.

Comments are closed