Conditional dispatcher to Command Pattern

Some time ago, a customer asked me for a solution for the following problem:

They had to send messages to certain people. These messages where kept in a database table. The messages contained variables which had to be filled in by the person who sends the message. For example:

I will be [NumberInput] minutes late.

You need to make a delivery to address [AddressList].

Please call telephone number [TelephoneList].

When the operator selects the first message, a textbox is shown where he has to input a delay.

When the second message is selected, a combobox will be shown containing addresses.

When the third message is selected, a listbox will be shown containing telephone numbers.

So I created a little application which retrieved the messages from the database in a Combobox. When selecting one of the messages, the following code runs

Label lblTitle;
if (messageSelector.SelectedItem.ToString().IndexOf("NumberInput") != -1)
{
Label lblTitle = new Label();
lblTitle.Text = "Input Number"
messagePanel.Controls.Clear();
TextBox txtNumberInput = new TextBox();
txtNumberInput.Name = "NumberInput"
messagePanel.Controls.Add(lblTitle);
txtNumberInput.Width = 120;
txtNumberInput.Location = new Point(100, 0);
messagePanel.Controls.Add(txtNumberInput);
}
else if (messageSelector.SelectedItem.ToString().IndexOf("AddressList") != -1)
{
Label lblTitle = new Label();
lblTitle.Text = "Choose Address"
messagePanel.Controls.Clear();
ComboBox cmbAdressList = new ComboBox();
cmbAdressList.Name = "AddressList"
// Get addresses from the database.
GetAddresses(cmbAdressList);
messagePanel.Controls.Add(lblTitle);
cmbAdressList.Width = 120;
cmbAdressList.Location = new Point(100, 0);
messagePanel.Controls.Add(cmbAdressList);
}
else if (messageSelector.SelectedItem.ToString().IndexOf("TelephoneList") != -1)
{
Label lblTitle = new Label();
lblTitle.Text = "Choose Telephone"
messagePanel.Controls.Clear();
ListBox lstTelephoneList = new ListBox();
lstTelephoneList.Name = "TelephoneList"
// Get telephone numbers from the database.
GetTelephoneNumbers(lstTelephoneList);
messagePanel.Controls.Add(lblTitle);
lstTelephoneList.Width = 120;
lstTelephoneList.Location = new Point(100, 0);
messagePanel.Controls.Add(lstTelephoneList);
}

This is definitely a violation of the open-closed principle. And duplicate code? A lot of it.

But the customer wanted a quick solution and they said no other messages will be added with different controls. One week later, a new message was added :)

Please call telephone number [TelephoneInput].

This message resulted in a special textbox GsmNumberTextBox which did some extra validations using regular expressions.

So, I could add another conditional statement but I was sure that there would be additional messages in the future. So time to change some stuff.

I read an article, "Replace conditional dispatcher with command pattern".

First I would do the refactor step "Extract Method":

if (messageSelector.SelectedItem.ToString().IndexOf("NumberInput") != -1)
{
ShowNumberInput();
}
else if (...)

private Label ShowNumberInput()
{
Label lblTitle;
lblTitle = new Label();
lblTitle.Text = "Input Number"
messagePanel.Controls.Clear();
TextBox txtNumberInput = new TextBox();
txtNumberInput.Name = "NumberInput"
messagePanel.Controls.Add(lblTitle);
txtNumberInput.Width = 120;
txtNumberInput.Location = new Point(100, 0);
messagePanel.Controls.Add(txtNumberInput);
return lblTitle;
}

I do the same for the other conditional steps.

The next refactor step is "Extract class":

class NumberInputTextBox
{
TextBox _txtNumberInput;

public NumberInputTextBox()
{
_txtNumberInput = new TextBox();
}

public Control CreateNumberInput()
{
Control control = new UserControl();
control.Width = 276;
Label lblTitle;
lblTitle = new Label();
lblTitle.Text = "Input Number"
_txtNumberInput.Name = "NumberInput"
control.Controls.Add(lblTitle);
_txtNumberInput.Width = 120;
_txtNumberInput.Location = new Point(100, 0);
control.Controls.Add(_txtNumberInput);
return control;
}

public string Text
{
get { return _txtNumberInput.Text; }
set { _txtNumberInput.Text = value;}
}
}

I create a NumberInputTextBox and move all the code from the ShowNumberInput method to a public class method CreateNumberInput. The Text property has to be exposed so I create a public property. In the CreateNumberInput method I create some sort of placeholder named control which I decorate with the necessary controls.

I do the same for the other conditional steps. So now we have three classes.

Now I change the conditional steps to call the new classes and I also remove the call to the old ShowNumberInput method.

if (messageSelector.SelectedItem.ToString().IndexOf("NumberInput") != -1)
{
messagePanel.Controls.Clear();

NumberInputTextBox numberInputTextBox =
new NumberInputTextBox();

messagePanel.Controls.Add
(numberInputTextBox.CreateNumberInput());
}
else if (...)

I apply the step "Extract Interface" on the classes. I have to decide a common command execution method name. Now we have :

• CreateNumberInput
  
• CreateAddressListBoxCombo
  
• CreateTelephoneListBox

Let's change these names to CreateParameterControl and create the interface:

public interface IHandler
{
Control CreateParameterControl();
}

Let the parameter classes implement IHandler. Now every class' Create... method is renamed to CreateParameterControl.

The next step is to change the calls in the conditional statements to the new CreateParameterControl method.

if (messageSelector.SelectedItem.ToString().IndexOf("NumberInput") != -1)
{
messagePanel.Controls.Clear();

NumberInputTextBox numberInputTextBox = new
NumberInputTextBox();

messagePanel.Controls.Add(numberInputTextBox.CreateParameterControl());
}

Now we have a couple of different solutions to go further with the refactoring:

• Create a Command map and fill it with key value pairs being the name of the parameter to create and the resulting parameter class.
  
• Create a Factory which uses reflection (see my article Reflection instances ) to let a config file decide which parameter requires which control.

First let's see how the the Command map solution works:

First of all we create a map. I'll use a generic dictionary for it like this:

IDictionary<string,Control> _paramCreationMap;

During initialization we fill the map:

private void CreateHandlerMap()
{
_paramCreationMap = new Dictionary<string,Control>();
_paramCreationMap.Add("NumberInput", new NumberInputTextBox().CreateParameterControl());
_paramCreationMap.Add("AddressList", new AddressListComboBox().CreateParameterControl());
_paramCreationMap.Add("TelephoneList", new TelephoneListBox().CreateParameterControl());
}

the previous conditional dispatcher, can now be completely removed and replaced with this code:

// Get the param string out of the selectedItem using reflection
string paramName = Regex.Match(messageSelector.SelectedItem.ToString(), @"(?<=\[).*(?=\])").Value;
messagePanel.Controls.Clear();
Control paramControl = _paramCreationMap[paramName];
messagePanel.Controls.Add(paramControl);

So no more conditional if...then...else...else if... or switch case statements.

Now, lets look at the second solution:

The result I'm trying to achieve is to get rid of the filling of a map with instances. Because when a new control is created, the instance has to be added to the map which I don't particularly like.

Then, a friend of mine showed me the beauty of reflection. I created my reflection based factory like this:

public static class ParameterFactory
{
public static Control CreateParameter(string paramName)
{
string assemblyName =
ConfigurationManager.AppSettings["ParameterControlsProject"];

string typeToLoad =
ConfigurationManager.AppSettings[paramName];

ObjectHandle objectHandle =
Activator.CreateInstance(assemblyName,typeToLoad);

IHandler parameter = (IHandler)objectHandle.Unwrap();
Control control = parameter.CreateParameterControl();
control.Name = typeToLoad;

return control;
}
}

The factory gets called like this:

// Get the param string out of the selectedItem using reflection
string paramName =
Regex.Match(messageSelector.SelectedItem.ToString(), @"(?<=\[).*(?=\])").Value;

messagePanel.Controls.Clear();

Control paramControl = ParameterFactory.CreateParameter(paramName);

messagePanel.Controls.Add(paramControl);

In the config file, in the AppSettings section we declare some key-value pairs which specify, which paramName corresponds with which type to load:

<appSettings>
<add key="ParameterControlsProject" value ="ParameterControls"></add>
<add key="NumberInput" value ="ParameterControls.NumberInputTextBox"></add>
<add key="AddressList" value ="ParameterControls.AddressListComboBox"></add>
<add key="TelephoneList" value ="ParameterControls.TelephoneListBox"></add>
</appSettings>

The key corresponds with paramName. ParameterControlsProject resembles the project where the actual control classes reside. Maybe we can change this to get the name from the calling assembly or something. I'll have to try this out.

That's it,

Greetz, G

Refactoring to Null Object

On my holiday, I was reading the book refactoring to patterns and I came to a topic called "Introducing Null Object".

It seemed to be extremely promising for some duplicate code I wrote some time ago. Let me explain what the problem is.

I had written code to do some actions on a logfile. Like writing, reading and more similar actions.

The log class looked like this:

public class Log
{

StreamWriter textWriter;

public Log()
{
}

 

public void Write(string logMessage)
{
      textWriter = new StreamWriter(@"<path>\log.txt");
      textWriter.WriteLine(logMessage);
}

 

public string Read()
{
      return "Some log entries";
}

 

public void DoSomeLogAction()
{}

 

public void DoSomeOtherLogAction()
{}

}

Then I created the following service LogService:

public class LogService
{

Log _log;

public LogService(){}

 

public LogService(Log log)
{
      _log = log;
}

public void WriteToLog(string logMessage)
{
        if (_log != null)
       {
                this._log.Write(logMessage);
       }
       else
      {
              DoSomethingElse();
      }
}

public string ReadFromLog()
{
       string result="";

       if (_log != null)
      {
            result = _log.Read();
      }
      else
     {
            DoSomethingElse();
     }
     return result;
} 

public void SomeLogAction()
{
      if (_log != null)
      {
            _log.DoSomeLogAction();
      }
      else
     {
            DoSomethingElse();
     }
}

 

public void SomeOtherLogAction()
{
      if (_log != null)
     {
           _log.DoSomeOtherLogAction();
     }
     else
    {
           DoSomethingElse();
    }
}

 

private void DoSomethingElse()
{}

}

We can see that there's a lot of duplicate code in it. Because of the possibility that logging is not enabled, we repeatedly have to test against the null value. If we don't do this, at some point in time we will definitely going to receive some exceptions.

Isn't it possible to reduce the duplicate code? Yes it is. By using refactoring to Null Object. This is what we have to do.

First of all we extract an interface from the Log Class. Like this:

public interface ILog
{
      void Write(string logMessage);
      string Read();
      void DoSomeLogAction();
      void DoSomeOtherLogAction();
}

 We let Log implement this interface.

Then we create a new class NullLog and let it also implement the ILog interface. But, the difference with the Log class is that NullLog takes care of the actions when a null value is passed to the service. Like this:

public class NullLog : ILog
{
      public NullLog()
     {}

      public void Write(string logMessage)
      {
            DoSomethingElse();
      }

      public string Read()
      {
            string result=""
            DoSomethingElse();
            return result;
      }

      public void DoSomeLogAction()
      {
            DoSomethingElse();
      }

      public void DoSomeOtherLogAction()
      {
            DoSomethingElse();
      }

      private void DoSomethingElse()
      {}

}

It's also common to let the methods do nothing at all.

Now comes the fun part. Change the LogService class to remove redundant code. Like this:

public class LogService
{

Log _log;

public LogService(){}

 

public LogService() : this(new NullLog()){}

public LogService(Log log)
{
      _log = log;
}

public void WriteToLog(string logMessage)
{
        if (_log != null)
       {
                this._log.Write(logMessage);
       }
       else
      {
              DoSomethingElse();
      }
}

public string ReadFromLog()
{
       string result="";

       if (_log != null)
      {
            result = _log.Read();
      }
      else
     {
            DoSomethingElse();
     }
     return result;
} 

public void SomeLogAction()
{
      if (_log != null)
      {
            _log.DoSomeLogAction();
      }
      else
     {
            DoSomethingElse();
     }
}

 

public void SomeOtherLogAction()
{
      if (_log != null)
     {
           _log.DoSomeOtherLogAction();
     }
     else
    {
           DoSomethingElse();
    }
}

 

private void DoSomethingElse()
{}

}

First of all we changed the default constructor to pass an instance of the NullLog class to the overloaded constructor. So whenever the Service is instantiated without a Log object, everything will be delegated to the NullLog object.

Because of this change, all the null testing is redundant and can be removed.

Finally the private method DoSomethingElse is no longer necessary and can be safely removed.

The new ServiceLog class now looks like this:

public class LogService
{
      private ILog _log;

      public LogService() : this(new NullLog()){}

      public LogService(ILog log)
      {
            _log = log;
      }

      public void WriteToLog(string logMessage)
      {
            _log.Write(logMessage);
      }

      public string ReadFromLog()
      {
            return _log.Read();
      }

      public void SomeLogAction()
      {
            _log.DoSomeLogAction();
      }

      public void SomeOtherLogAction()
      {
            _log.DoSomeOtherLogAction();
      }

}

This looks a lot better doesn't it.

Comments are always welcome.

Greetz,

G

Refactoring to patterns by Joshua Kerievsky

Hi,

I've just got back from a holiday. One week holiday means read, read, read.

So I bought and read the book "Refactoring to patterns" and this is my conclusion:

I found it not particularly easy. But it is really great. I read stuff I already did but could be improved enormously. One thing I have to say is that reading "Refactoring: Improving the Design of Existing Code" from Martin Fowler is a must before reading this one. There are a lot of pointers to Fowler's book.

Greetz,

G