Thursday, October 13, 2005

Refactoring

I just spent the last day and a half fixing some bugs and adding some features to a relatively simple VB6 application. The first half day was really stressful because the code was not up to my very high standards - 300+ line routines, no objects, schizophrenic routines (save also validated), operational data split between memory and screen controls, etc. I couldn't understand the logic nor the functionality. Then it hit me ... REFACTOR!

I read Martin Fowler's book, Refactoring over the summer and it gave structure to a practice I already followed and renewed my interest in continuing. For those who are not familiar with the term refactoring, it is the process of changing code structure without changing the behavior to increase legibility, decrease complexity, etc.

Back to my hell, one piece of functionality I had to add involved the followng code:
    If Not IsNumeric(tvItems.Tag) Then tvItems.Tag = -1
    If CInt(tvItems.Tag) <> tvItems.SelectedItem.index Then
        If tvItems.Tag <> -1 And Me.chkIgnoreThisJob <> 1 Then
            ...
I knew I could figure it out if I beat my head against a wall for a few hours, which I actually started doing (by actually trying to understand the code). Then I took a different tack. I realized that "Not IsNumeric(tvItems.Tag)" actually meant we do not know which item is selected and that "tvItems.Tag = -1" actually meant no job is selected, so I wrote the following functions:
    Private Function WeDontKnowWhichJobIsSelected() As Boolean
        WeDontKnowWhichJobIsSelected = (IsNumeric(tvItems.Tag) = False)
    End Function

    Private Sub SetThatNoJobIsSelected()
        tvItems.Tag = -1
    End Sub

and changed the original code to read:
    If WeDontKnowWhichJobIsSelected Then
        SetThatNoJobIsSelected
    End If
    If CInt(tvItems.Tag) <> tvItems.SelectedItem.index Then

        If tvItems.Tag <> -1 And Me.chkIgnoreThisJob <> 1 Then
            ...
Can you see where this is going? Eventually I added a few more functions and the original block of code looked like this:
    If WeDontKnowWhichJobIsSelected Then
        SetThatNoJobIsSelected
    End If
    If TheSelectedJobHasChanged Then

        If AJobIsSelected And IgnoreThisJob = False Then
            ...
Now the code code was completely clear and I knew where to make my change. Not only that, but I knew life would be easier for the next person.

So the real purpose of this story is to ask the following questions:

  • Is there anyone else who does this?
  • Did you know you were doing it?
  • Are you glad you do it?
  • Will you do it in the future?
  • What is the airspeed of an unladen swallow?

Andrew