*Words of Wisdom From The Elder

How Clean is Your Closet?

Have you ever been instructed to make an enhancement to an existing project where the codebase looked like the picture above?  I have.  As a matter of fact, I’ve done my fair share of creating messy code just like that.  I’m not saying I’m proud of admitting that but as they say, “The truth hurts”.

Let’s pretend for a moment this is my closet and you were visiting.  We were about to leave to go to town and you said, “Brrrr, it is cold outside, do you have a scarf?”.  I said, “Sure, it is in the closet, there is a purple one in there.”. 

You then stumble into the closet to find the scarf.  How long do you think it would take you?  The likelihood of you finding it within a few seconds is slim to none. 

Let’s switch gears to programming.  How long would it take to implement a task if the picture above represented the codebase of an existing project?  A long time for sure, definitely longer than it should take no doubt.  Why?  Because things that are messy take longer!  Here is an Elderism (common sense words of wisdom to live by) that sums up messy projects:

ELDERISM – “A messy project takes longer to understand and will only get messier.”

Switch gears for a moment and let’s say you needed to find the purple scarf in this closet.  Obviously since things are organized and well laid out, it becomes easy to locate, add, and remove things. 

The really sad part about this whole analogy is that for some reason, a lot of developers are happy with wallowing in a perfect mess.  Not only does this make things hard for them but for other developers on the team as well. 

A Real World Mess

I was reminded of this the other day when I started to work on Witty Twitter, an open source WPF Twitter client several of us work on.  I hadn’t worked on the project very much lately but I wanted to take some time during vacation to clean some things up and add some new features.  I know how most things work in codebase but the biggest problem with the codebase is the lack of structure and organization to it.  I started to implement a new feature and suddenly I realized I was contributing to the code rot.  Remember the Elderism, “A  messy project takes longer to understand and will only get messier.”?  Well, here I was adding more mess to the codebase to get something to work. 

Finally I said, enough is enough.  My contribution is going to be to simplify and clean up this code.  In other words, clean up and organize the closet.  To give you an idea of the mess staring me in the face, here is the constructor of the main application.  This is ONE function which initializes the main form Witty uses.

   1: public MainWindow()

   2: {

   3:     this.InitializeComponent();

   4:  

   5: #if DEBUG

   6:     Title = Title + " Debug";

   7: #endif

   8:  

   9:     // Trap unhandled exceptions

  10:     LayoutRoot.Dispatcher.UnhandledException += new DispatcherUnhandledExceptionEventHandler(Dispatcher_UnhandledException);

  11:  

  12:     #region Minimize to tray setup

  13:  

  14:     _notifyIcon = new System.Windows.Forms.NotifyIcon();

  15:     _notifyIcon.BalloonTipText = "Right-click for more options";

  16:     _notifyIcon.BalloonTipTitle = "Witty";

  17:     _notifyIcon.Text = "Witty - The WPF Twitter Client";

  18:     _notifyIcon.Icon = Witty.Properties.Resources.AppIcon;

  19:     _notifyIcon.DoubleClick += new EventHandler(m_notifyIcon_Click);

  20:  

  21:     System.Windows.Forms.ContextMenu notifyMenu = new System.Windows.Forms.ContextMenu();

  22:     System.Windows.Forms.MenuItem openMenuItem = new System.Windows.Forms.MenuItem();

  23:     System.Windows.Forms.MenuItem exitMenuItem = new System.Windows.Forms.MenuItem();

  24:  

  25:     notifyMenu.MenuItems.AddRange(new System.Windows.Forms.MenuItem[] { openMenuItem, exitMenuItem });

  26:     openMenuItem.Index = 0;

  27:     openMenuItem.Text = "Open";

  28:     openMenuItem.Click += new EventHandler(openMenuItem_Click);

  29:     exitMenuItem.Index = 1;

  30:     exitMenuItem.Text = "Exit";

  31:     exitMenuItem.Click += new EventHandler(exitMenuItem_Click);

  32:  

  33:     _notifyIcon.ContextMenu = notifyMenu;

  34:     this.Closed += new EventHandler(OnClosed);

  35:     this.StateChanged += new EventHandler(OnStateChanged);

  36:     this.IsVisibleChanged += new DependencyPropertyChangedEventHandler(OnIsVisibleChanged);

  37:  

  38:     // used to override closings and minimize instead

  39:     this.Closing += new CancelEventHandler(MainWindow_Closing);

  40:  

  41:     #endregion

  42:  

  43:     #region Single instance setup

  44:     // Enforce single instance for release mode

  45: #if !DEBUG

  46:     Application.Current.Exit += new ExitEventHandler(Current_Exit);

  47:     _instanceManager = new SingleInstanceManager(this, ShowApplication);

  48: #endif

  49:     #endregion

  50:  

  51:     // Set the data context for all of the tabs

  52:     LayoutRoot.DataContext = tweets;

  53:     RepliesListBox.ItemsSource = replies;

  54:     UserTab.DataContext = userTweets;

  55:     MessagesListBox.ItemsSource = messages;

  56:  

  57:     // Set how often to get updates from Twitter

  58:     refreshInterval = new TimeSpan(0, int.Parse(AppSettings.RefreshInterval), 0);

  59:  

  60:     this.Topmost = AlwaysOnTopMenuItem.IsChecked = AppSettings.AlwaysOnTop;

  61:  

  62:     // Does the user need to login?

  63:     if (string.IsNullOrEmpty(AppSettings.Username))

  64:     {

  65:         PlayStoryboard("ShowLogin");

  66:     }

  67:     else

  68:     {

  69:         LoginControl.Visibility = Visibility.Hidden;

  70:  

  71:         System.Security.SecureString password = TwitterNet.DecryptString(AppSettings.Password);

  72:  

  73:         // Jason Follas: Reworked Web Proxy - don't need to explicitly pass into TwitterNet ctor

  74:         //twitter = new TwitterNet(AppSettings.Username, password, WebProxyHelper.GetConfiguredWebProxy());

  75:         twitter = new TwitterNet(AppSettings.Username, password);

  76:  

  77:         // Jason Follas: Twitter proxy servers, anyone?  (Network Nazis who block twitter.com annoy me)

  78:         twitter.TwitterServerUrl = AppSettings.TwitterHost;

  79:  

  80:         // Let the user know what's going on

  81:         StatusTextBlock.Text = Properties.Resources.TryLogin;

  82:         PlayStoryboard("Fetching");

  83:  

  84:         // Create a Dispatcher to attempt login on new thread

  85:         NoArgDelegate loginFetcher = new NoArgDelegate(this.TryLogin);

  86:         loginFetcher.BeginInvoke(null, null);

  87:     }

  88:  

  89:     InitializeClickOnceTimer();

  90:  

  91:     InitializeSoundPlayer();

  92:  

  93:     ScrollViewer.SetCanContentScroll(TweetsListBox, !AppSettings.SmoothScrolling);

  94:  

  95:     //Register with Snarl if available

  96:     if (SnarlConnector.GetSnarlWindow().ToInt32() != 0)

  97:     {

  98:         //We Create a Message Only window for communication

  99:  

 100:         this.SnarlConfighWnd = Win32.CreateWindowEx(0, "Message", null, 0, 0, 0, 0, 0, new IntPtr(Win32.HWND_MESSAGE), IntPtr.Zero, IntPtr.Zero, IntPtr.Zero);

 101:         WindowsMessage wndMsg = new WindowsMessage();

 102:         SnarlConnector.RegisterConfig(this.SnarlConfighWnd,"Witty",wndMsg);

 103:         

 104:         SnarlConnector.RegisterAlert("Witty", "New tweet");

 105:         SnarlConnector.RegisterAlert("Witty", "New tweets summarized");

 106:         SnarlConnector.RegisterAlert("Witty", "New reply");

 107:         SnarlConnector.RegisterAlert("Witty", "New direct message");

 108:         

 109:     }

 110: }

In case you don’t scroll down and actually glance at the code, there are over 100 lines of code for this constructor.  No matter how smart you are, there is no way one can read that code and fully understand “what” it is doing without devoting an enormous amount of time to it.  Everything in there, with the exception of a few functions that get called, the code is all about “how”.  All of the code in that constructor is needed too.  It isn’t like it can be taken out, it must be there to set things up in the application. 

Cleaning Up The Closet

If you don’t take anything else away from this article remember this one fact.  Developers reading your code rarely are worried about the “how”, they need to know the “what” so they can figure out where to put their code.  This concept is earth shatteringly simple, yet as you can see with the above code, we have a perfect real world example of a “how” that has turned itself into a mess. 

Code Smell #87 – Comments

A code smell is something we refer to in the business as something that doesn’t look right, or that shouldn’t be done that way, or something that will lead the developer down a dark path.  It means many things and that’s why I labeled this as #87. 

Comments in code have saved many developers endless hours, but they have also cost many developers thousands of hours.  A rule I have, is code shouldn’t have any comments in the code.  My main reason for this is refactoring. 

Code refactoring is the process of changing a computer program‘s code to make it amenable to change, improve its readability, or simplify its structure, while preserving its existing functionality.

In software development we constantly refactor our code.  Guess what happens to those comments though?  They don’t get refactored! 

A developer can completely change how a program works and those nasty comments are still there.  Then along comes someone else who reads the comment and thinks the code is broken because it doesn’t do what the comment said it was doing.  They then refactor it back and things break.

The best thing to do is not comment your code.  If you have the urge to put a comment on something, that means you didn’t express the intent of the code clearly enough, thus it is a code smell.  Let’s look at a few examples from our example above to see how this plays out.

Let’s start with line #95 in the example above.  Here is that example:

//Register with Snarl if available

if (SnarlConnector.GetSnarlWindow().ToInt32() != 0)

{

    //We Create a Message Only window for communication

    this.SnarlConfighWnd = Win32.CreateWindowEx(0, "Message", null, 0, 0, 0, 0, 0, new IntPtr(Win32.HWND_MESSAGE), IntPtr.Zero, IntPtr.Zero, IntPtr.Zero);

    WindowsMessage wndMsg = new WindowsMessage();

    SnarlConnector.RegisterConfig(this.SnarlConfighWnd, "Witty", wndMsg);

    SnarlConnector.RegisterAlert("Witty", "New tweet");

    SnarlConnector.RegisterAlert("Witty", "New tweets summarized");

    SnarlConnector.RegisterAlert("Witty", "New reply");

    SnarlConnector.RegisterAlert("Witty", "New direct message");

}

Notice the comment in the first line of the code?  The comment is telling us “what” the if block is doing.  Even though I can read this:

(SnarlConnector.GetSnarlWindow().ToInt32() != 0)

I honestly do not know the Snarl API and thus I have no idea “what” that is doing, or why.  In this case, the developer that wrote this in their mind said this was something they had to figure out “how to do” so they wanted to leave a bread crumb here to tell everyone “what it’s doing”.

Now think for a second.  Without using a comment, how else could they have told us the exact same thing?  The answer: CODE. 

If a developer focuses on making their code more readable the end result will be much neater and it will also express the “what it is doing”.  This means other developers can move quickly through the project / closet to figure out where things are.

For those cleaning up code like this, more times than not you can just take the comments and turn that into functions.  This is the case so much in fact that Visual Studio plug-ins like Refactor! Pro automatically turn comments like that into the name of the function when extracting code into a method. 

In this example there are two comments.  We’d refactor this to read like this:

public MainWindow()

{

    RegisterWithSnarlIfAvailable();

}

private void RegisterWithSnarlIfAvailable()

{

    if (SnarlConnector.GetSnarlWindow().ToInt32() != 0)

    {

        CreateSnarlMessageWindowForCommunication();

    }

}

private void CreateSnarlMessageWindowForCommunication()

{

    this.SnarlConfighWnd = Win32.CreateWindowEx(0, "Message", null, 0, 0, 0, 0, 0, new IntPtr(Win32.HWND_MESSAGE), IntPtr.Zero, IntPtr.Zero, IntPtr.Zero);

    WindowsMessage wndMsg = new WindowsMessage();

    SnarlConnector.RegisterConfig(this.SnarlConfighWnd, "Witty", wndMsg);

    SnarlConnector.RegisterAlert("Witty", "New tweet");

    SnarlConnector.RegisterAlert("Witty", "New tweets summarized");

    SnarlConnector.RegisterAlert("Witty", "New reply");

    SnarlConnector.RegisterAlert("Witty", "New direct message");

}

With a simple refactor we have fully expressed the “what” the code is doing and made it more readable. 

Applying these simple concepts to the original example that was over 100 lines long, we can refactor this into readable code with zero comments that expresses what is going on instead of how.  The end result is less code to read in the constructor that reads more like a story describing what the constructor is doing. 

   1: public MainWindow()

   2: {

   3:     this.InitializeComponent();

   4:  

   5:     TrapUnhandledExceptions();

   6:  

   7:     SetupNotifyIcon();

   8:     

   9:     SetupSingleInstance();

  10:  

  11:     SetDataContextForAllOfTabs();

  12:     

  13:     SetHowOftenToGetUpdatesFromTwitter();

  14:  

  15:     InitializeClickOnceTimer();

  16:  

  17:     InitializeSoundPlayer();

  18:  

  19:     InitializeMiscSettings();

  20:  

  21:     RegisterWithSnarlIfAvailable();

  22:  

  23:     DisplayLoginIfUserNotLoggedIn();

  24: }

Do you see the difference?  It is night and day isn’t it?  There is no more wondering, guessing, and also worrying about doing something in the wrong place. 

If you are dealing with messy closets, I mean code, apply these basic principles and you’ll wind up with codebases that are more readable and easier to refactor down the road.  This is only one thing you can do to clean up messy projects.  There are others but we have to start somewhere. 

Once things are refactored so they express the what things will get more clear and other refactorings will start to bubble up.  For example, if you look at the final code above, you’ll notice the DisplayLoginIfUserNotLoggedIn() method is called last in the constructor.  This makes sense.  We should call the login screen after everything else is initialized.  However, in the original code it wasn’t last.  How would I have figured that out if I hadn’t made the code more readable?  Exactly my point!

It is a very powerful concept, give it try.