Of Mice and Anti-Patterns


This post primarily focuses on initiating behavior in a constructor. In order to use a real world illustration, there are a few other anti-patterns initiated, which I might further expound on in later posts. 😉

Background

I started out this morning examining new code (from 2008) that was produced in Visual Basic 6. I know some may see something inherently wrong with initiating new development in VB6 (and others who will hate me for the mere suggestion it might be wrong to start new development in VB6). I am not here to focus on the language, but that patterns.

As I do not have Visual Studio 6 installed, I was forced to read the code in notepad. Opening one of the two forms in the project, I find the following form load (shortened a bit, but still illustrating the point):

Private Sub Form_Load()

strCommand = Command()
If strCommand = "" Then
Else
    If (strCommand = 1) Then
            ‘Code for command 1
        ElseIf (strCommand = 3) Then
            ‘Code for command 3
            Unload Me
        ElseIf (strCommand = 2) Then
            ‘Code for command 2
            Unload Me
    End If
End If

End Sub

Note that command 1 does not unload the form, as this is where the issue started.

Here is the basic usage scenario:

  1. User starts windows form application
  2. User clicks on first command
  3. Command event handler loads Inet form with command id
  4. When the form loads, it uses command id to run code
    But this allows the loading of information messages and other buttons – more on this in a bit

Now, let’s look at the code you would use for this scenario. First let’s look at the main form, which the original developer did attempt to refactor a bit by putting all command buttons through the same event handler.

public partial class Form1 : Form
{
    public Form1()
    {
        InitializeComponent();
    }

    private void commandButton_Click(object sender, EventArgs e)
    {
        Inet form = null;
        Button button = (Button)sender;

        if (button.Text == "Command 1")
        {
            form = new Inet(1);
        }
        else if (sender.ToString() == "Command 2")
        {
            form = new Inet(2);
        }
        else if (sender.ToString() == "Command 3")
        {
            form = new Inet(3);
        }
        form.Show();
    }
}

As in the original code, all of the code is sent through a single event handler. And here is how the Inet form is set up (using the same pattern as the original):

public partial class Inet : Form
{
    public int Command { get; set; }

    public Inet()
    {
        InitializeComponent();
    }

    public Inet(int command) : this()
    {
        Command = command;
    }

    private void Inet_Load(object sender, EventArgs e)
    {
        if (Command == 1)
        {
            MakeButtonsVisible();
            RunCommand1();
        }
        else if (Command == 2)
        {
            RunCommand2();

            //Can’t close on load, as in the original
        }
        else if (Command == 3)
        {
            RunCommand3();
            //Can’t close on load, as in the original
        }
    }   
}

The original code is much more complex than this (the original does not have RunCommandX() methods, it embeds the entire code in the Load() routine, with the exception of the command 4 and command 5 button events). The code does run like this, however.

How did this code get like this?

This is an interesting question. The history is this is part of an “automation” process, whereby automation means creating a batch script that runs an executable that then calls an Access file. And that is a simple step in the process, as often times a single process has multiple vbs files prior to getting to the executable. It is a nightmare, to say the least. It appears each developer developed with the tool he was familiar with, with absolutely no regard for consistency.

I am not sure exactly what happened, but running through the code, I think I have the basic process. It runs something like this:

Developer created VB app, as he was familiar with VB and not .NET or batch scripts, etc. He created a simple windows forms application that fired a macro in an Access database (don’t ask). he then found out the process needed additional user input, so he created a second form to actually run the command. The only way he could figure to accomplish this was to have the form instantiated with a command number and then check the command number in the Load() method.

If this was written up as a usage scenario, it would look like this (including the scripts that call the process):

  1. Operator runs script 12A
  2. Script does some processing and then fires off the application in question
  3. Operator presses the topmost button
  4. Second form is spun up and command runs (unloading for command 2 and 3)
    1. If is command 1
      1. form buttons are visible
      2. User presses command 4 button and it runs
      3. User presses command 5 button and it runs
      4. At end of command 5, form unloads
  5. Repeat for other commands
  6. At the end of command 3, application closes with a dialog stating “Compete”

Looking at the original specifications, the goal was to automated the process of creating clearing files, so this scenario really misses the mark.

Anti-Patterns and the Universe

The way anti-patterns are introduced, normally, is a combination of ignorance and sunk costs. Let’s run through this idea:
The developer, not truly understanding the language or the problem comes up with something he has used before that appears to work. He then begins to work on it and after spending hours he does not want to throw it away. So he continues to kludge things up until the code has the desired effect.

In this case, the user needed extra buttons, which, in his mind, dictated an extra form. When he spun up the extra form, however, he was unable to get the code running in the middle of the process, so he moved the process code to the second form. The only way he could figure to get the code running was to put it in form load method. And, having already spent time on this pattern, he set up the other commands in the same way. At least consistency was in order.

There are so many things wrong with this pattern, but my biggest beef with this pattern is the load is initiating behavior.

Perhaps someone can show me an exception to this rule, but I am firm on separating state from behavior. When an object loads, there is nothing wrong with setting up state. For example, we might have a person object:

public class Car
{
    public string Make { get; private set; }
    public string Model { get; private set; }
    public int Year { get; private set; }
   
    public Car(string make, string model, int year)
    {
        Make = make;
        Model = model;
        Year = year;
    }
}

I also have the ability to add behavior to an object’s constructor. But is it correct? Look at the following code and think about your own car. Would you want it to act as it does in the following code?

public class Car
{
    public string Make { get; private set; }
    public string Model { get; private set; }
    public int Year { get; private set; }
   
    public Car(string make, string model, int year)
    {
        Make = make;
        Model = model;
        Year = year;
        Accelerate();
    }

    public void Accelerate()
    {
         //Code to pump extra gas in engine to make it move faster
    }
}

What is wrong with this idea?
First, it is extremely hard to test a system that has behavior spin up when an object instantiates. You cannot independently test the instantiation of the object unless you code in a way out. Examine this, for example:

public class Car
{
    public string Make { get; private set; }
    public string Model { get; private set; }
    public int Year { get; private set; }
   
    public Car(string make, string model, int year, int command)
    {
        Make = make;
        Model = model;
        Year = year;

        if(command == 1)
           Accelerate();
        else if (command == 2)
           PlayRadio();
        else
        {
          //added so I can send in bogus command to test instantiation without action

        }

    }

    public void Accelerate()
    {
         //Code to pump extra gas in engine to make it move faster
    }

    public void PlayRadio()
    {
    }
}

This solves the problem in the same manner as the application that started this blog. There is a fall through condition to ensure action does not take place if an unknown command number is sent to the form. If you unit test, this means you end up with a test like the following:

[TestMethod]
public void should_have_model_of_Mustang()
{
     //Command = 42 to avoid running other commands
     Car car = new Car(“Ford”, “Mustang”, 1965, 42);
    
     Assert.IsTrue(car.Model == “Mustang”);
}

I would normally set up the test with variables, but the point is my short circuit of behavior allows me to actually test if an object instantiates. It solves the problem, but look at how kludgy the code is now.

Getting Help

I participate in UseNet groups and forums on a regular basis. Often I see people asking for help asking a “how do I” question that makes me question whether or not they have moved into an anti-pattern. For example, this query was recently posted:

I need to be able to include an .aspx or .ascx file that will then process
simple variable substitutions prior to sending the file to the browser.

In the following example when I call sample.aspx I would like the string
printed at the browser to be …
    loaded-2.aspx … sample string.
… it is currently printed as …
    loaded-2.aspx … .

Suggestions would be greatly appreciated.

After the poster answering back, it was determined that using the Membership and Profile bits along with themes made the most sense. But, had the questioned been answered at face value, the poster might have developed a very complex application with different controls “included” on the page depending on user preference.

Unfortunately, for each one where the poster is asked for clarification, I see at least one illustrating how to make the car automatically accelerate when it is instantiated. Thus the anti-patterns propagate from developer to developer.

Final Notes

The code in this post illustrate, at least to me, why we developers need to go back to basics. Until we look at the rules, we should not be breaking them willy nilly. There are some great books out there, like McConnell’s “classic” Code Complete.

One thing to remember is anti-patterns are generally born out of ignorance, not maliciousness or stupidity. Education is the key to stomping them out, not punishment. The idea “the beatings will continue until you are educated” is about as counter productive as “the beatings will continue until the morale improves”.

Peace and Grace,
Greg

Twitter: @gbworld

Advertisements

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s

%d bloggers like this: