Refactor - Moving duplicate code to a separate method

I've mentioned in previous posts that it pains me to see developers use copy/paste to write code. It's more error prone, as you have to make sure all necessary changes are made to the copy. It's also harder to maintain since finding a bug in one copy of the code means making changes to the others - assuming you can find those copies. Depending on the circumstances, it can also be harder to read the code. As I keep running into less-than-ideal code, I decided a few posts were in order.

Today, let's look at a method that builds an XmlDocument based on values from a database. Here, the info is passed in a DataRow object.

public XmlDocument GenerateMailingLabel(DataRow subscriber)

{

  XmlDocument doc = new XmlDocument();

  doc.LoadXml("<MailingLabel><SendTo /></MailingLabel>");

  XmlNode sendToNode = doc.SelectSingleNode("//SendTo");

  XmlAttribute attribute = doc.CreateAttribute("FirstName");

  attribute.Value = subscriber["FirstName"].ToString();

  sendToNode.Attributes.Append(attribute);

  attribute = doc.CreateAttribute("LastName");

  attribute.Value = subscriber["LastName"].ToString();

  sendToNode.Attributes.Append(attribute);

  attribute = doc.CreateAttribute("StreetAddress");

  attribute.Value = subscriber["StreetAddress"].ToString();

  sendToNode.Attributes.Append(attribute);

  attribute = doc.CreateAttribute("City");

  attribute.Value = subscriber["City"].ToString();

  sendToNode.Attributes.Append(attribute);

  attribute = doc.CreateAttribute("State");

  attribute.Value = subscriber["State"].ToString();

  sendToNode.Attributes.Append(attribute);

  attribute = doc.CreateAttribute("Zip");

  attribute.Value = subscriber["Zip"].ToString();

  sendToNode.Attributes.Append(attribute);

  return doc;

}

There's nothing terribly complex going on here. It starts by creating a new, nearly empty XmlDocument. It then pulls necessary values out of the DataRow and adds them as attributes to the <SendTo> node. Modifying existing attributes appears to be easy, as each one is isolated to a single block of three lines. Adding new attributes is similarly straightforward. Copy an existing block, paste it just before the 'return', and then modify the two string literals. If it's so easy to work with, why change the code?

First, you have to make sure you modify both string literals correctly. Fail to modify one or both of them and you have a bug that may not be caught any time soon.

Second, these types of methods tend to become long. It may be easy to read currently, but what happens when you have twenty attributes to add to the document? Picking out a particular block to modify becomes a bit more challenging. This is especially true if you're currently lacking in sleep and/or caffein.

Third, a structural change to the xml document becomes a slow, tedious task. Maybe halfway through the project someone decides you need to create sub-elements instead of attributes for each value. In the above code, you have six sets of changes to make.

A better solution would be to pull the block of three lines into a separate method:

private void CreateAttribute(string attributeName, string columnName,

  XmlDocument doc, XmlNode sendToNode, DataRow subscriber)

{

  XmlAttribute attribute = doc.CreateAttribute(attributeName);

  attribute.Value = subscriber[columnName].ToString();

  sendToNode.Attributes.Append(attribute);

}

Once we've done this, each block of code in the GenerateMailingLabel method can be reduced to a single line:

public XmlDocument GenerateMailingLabel(DataRow subscriber)

{

  XmlDocument doc = new XmlDocument();

  doc.LoadXml("<MailingLabel><SendTo /></MailingLabel>");

  XmlNode sendToNode = doc.SelectSingleNode("//SendTo");

  CreateAttribute("FirstName", "FirstName", doc,

    sendToNode, subscriber);

  CreateAttribute("LastName", "LastName", doc,

    sendToNode, subscriber);

  CreateAttribute("StreetAddress", "StreetAddress", doc,

    sendToNode, subscriber);

  CreateAttribute("City", "City", doc,

    sendToNode, subscriber);

  CreateAttribute("State", "State", doc,

    sendToNode, subscriber);

  CreateAttribute("Zip", "Zip", doc,

    sendToNode, subscriber);

  return doc;

}

Now adding a new attribute requires one additional line instead of three. Even better, bug fixes or structural changes will be limited to a single section of code. In either case, changes can be made faster and with less risk of introducing bugs.