More than one way to sort a List<>

One operation you occasionally need to perform is sorting a generic list of objects. Often developers code handle with an inline delegate. If I wanted to sort a collection of boardgames by rating, my code might look like this:

  games.Sort(

      delegate(BoardGame a, BoardGame b)

      {

          if (a.Rating < b.Rating)

              return 1;

          if (a.Rating > b.Rating)

              return -1;

          return 0;

      });

Although this works, it tends to look a bit cluttered. It also increases a method's complexity and doesn't adhere to the idea of separation of concerns. If we only need to sort Boardgames in this one location, a better solution is to move the comparison code into a separate method in the same class. The new method looks like this:

  public int CompareBoardgamesByRank(BoardGame a, BoardGame b)

  {

      if (a.Rating < b.Rating)

          return 1;

      if (a.Rating > b.Rating)

          return -1;

      return 0;

  }

Our call to Sort looks like this:

  games.Sort(CompareBoardgamesByRank);

This makes the code much cleaner for a single sort. If we want to sort Boardgames by rating from multiple locations, we need a better place to store the comparison method. If we implement IComparable on our Boardgame class, we can create a CompareTo method on the class like so:

  public int CompareTo(object obj)

  {

      BoardGame b = (BoardGame)obj;

      if (Rating < b.Rating)

          return 1;

      if (Rating > b.Rating)

          return -1;

      return 0;

  }

Since the class now has a default comparison, we no longer need to specify a delegate when calling Sort - the CompareTo method will automatically be executed with an empty call:

  games.Sort();

What if we need additional comparison methods for our Boardgame? An easy way to handle this is to create static methods on the class, which will be available anywhere that Boardgame can be accessed. Here are a couple methods I've added to my Boardgame class:

  public static int CompareByName(BoardGame a, BoardGame b)

  {

      return string.Compare(a.Name, b.Name);

  }

  public static int CompareByRankAssending(BoardGame a, BoardGame b)

  {

      if (a.Rating > b.Rating)

          return 1;

      if (a.Rating < b.Rating)

          return -1;

      return 0;

  }

Which I can now pass into the Sort method:

  games.Sort(BoardGame.CompareByRankAssending);

Refactoring assemblies without breaking existing apps

Over time, most development shops identify common code that needs to be used across multiple projects. This code is eventually collected into a single assembly, usually something like Common.dll or Utilities.dll. In the beginning this is a decent way to eliminate code duplication. Over time, however, this single assembly becomes difficult to maintain.

At this point the obvious fix is to split the assembly into multiple smaller ones. Unfortunately, by then the single assembly is used on numerous projects. A major refactoring now requires extensive changes across all of these referencing applications. This realization usually stops any refactoring effort, leaving the utilities assembly to continue growing larger and more unwieldy.

To look at potential solutions, I've created a Utilities assembly with the following Logger class:

  namespace Utilities

  {

      public class Logger

      {

          public void LogError(string message)

          {

              Debug.WriteLine("Error: " + message);

          }

      }

  }

The goal is to move this class to a separate assembly called LoggingUtilities.

One solution is to use the TypeForwardedTo attribute. This is an assembly-level attribute that flags a specified class as having moved. To use this, I start by moving the Logger class to my new assembly. Note that I keep the same namespace as before - this is required for the forwarding to work.

Next I add a reference to LoggingUtilities within Utilities.

Finally, I open up AssemblyInfo.cs file in the Utilities project and add the following line:

  [assembly: TypeForwardedTo(typeof(Utilities.Logger))]

If I recompile the dlls and drop them in a folder with my existing application, it will continue to function even though the class has been moved.

This takes care of keeping the current compiled code running, but what about future versions? If I open up the source for one of my applications and attempt to compile, I now receive errors stating "The type or namespace name 'Logger' could not be found." It seems the redirection works at runtime but not at compile time. For someone not familiar with the previous refactoring, this could prove an interesting issue to track down.

In my opinion, there is a far better solution than using the TypeForwardedTo attribute. Going back to the original code, this time I copy the code to the new assembly (as opposed to moving it.) On the copy I change the namespace to match my new assembly.

  namespace LoggingUtilities

  {

      public class Logger

      {

          public void LogError(string message)

          { 

              Debug.WriteLine("Error: " + message);

          }

      }

  }

In my original Logger class, I create an instance of my new Logger. Each method in the original class now forwards requests to the new Logger instance. In this way, I am wrapping the new class in the original. This allows applications to still use the old class, though the functionality has been moved.

  public class Logger

  {

      LoggingUtilities.Logger _logger =

          new LoggingUtilities.Logger();

      [Obsolete("Use LoggingUtilities.Logger instead")]

      public void LogError(string message)

      {

          _logger.LogError(message);

      }

  }

As before we need to evaluate referencing projects. Because our original class still exists, these applications will continue to compile.

Note that I've added an "Obsolete" attribute to the LogError method. This means we will receive a compiler warning (or error) that we need to change our application to use the new class. This makes it clear what needs to be modified, saving time on any rework.

Cleaning up enumerations

When working on existing code I occasionally run into an enumeration class similar to this:

  public class PowertoolsConstants

  {

      public enum Powertools

      {

          PowerDrill = 0, // Standard, corded drill

          Chainsaw,       // Everyone's favorite

          CircularSaw     // If the chainsaw is out of gas, use this

      }

      public static Powertools ConvertFromString(string s)

      {

          switch (s)

          {

              case "PowerDrill":

                  return Powertools.PowerDrill;

              case "0":

                  return Powertools.PowerDrill;

              case "Chainsaw":

                  return Powertools.Chainsaw;

              case "1":

                  return Powertools.Chainsaw;

              case "CircularSaw":

                  return Powertools.CircularSaw;

              case "2":

                  return Powertools.CircularSaw;

              default:

                  throw new Exception("Unknown Powertool");

          }

      }

      public static string ConvertFromPowertool(Powertools p)

      {

          switch (p)

          {

              case Powertools.Chainsaw:

                  return "Chainsaw";

              case Powertools.CircularSaw:

                  return "CircularSaw";

              case Powertools.PowerDrill:

                  return "PowerDrill";

              default:

                  return "Unknown";

          }

      }

  }

Nothing too complex, but it can be cleaned up a bit. For starters, accessing the enum currently requires referencing the class:

  PowertoolsConstants.Powertools tool =

      PowertoolsConstants.Powertools.Chainsaw;

If we move the enum declaration above the class we can remove the class reference:

  Powertools tool = Powertools.Chainsaw;

Next is addressing the two methods in the class: ConvertFromString and ConvertFromPowertool. The purpose of these methods is to switch between our enumeration and a string representation of the enum, perhaps to store values in an xml file or database. As the .Net Framework already contains this functionality, the methods are not necessary and can be deleted.

To convert from an enum value to a string we can use Enum.GetName

  string toolName = Enum.GetName(typeof(Powertools), tool);

To convert from a string to an enum value we can use Enum.Parse. Note that this will throw an ArgumentException if an invalid string is passed in.

  tool = (Powertools)Enum.Parse(typeof(Powertools), toolName);

With the two methods removed, the class PowertoolsConstants is empty and can be deleted.

One final thing to look at are the comments beside the enum values. These appear to be usage notes. If a developer using the enum needs to know this information, he shouldn't have to open this code to get it. The way to correct this is to replace the existing comments with xml-style comments.

  public enum Powertools

  {

      /// <summary>

      /// Standard, corded drill

      /// </summary>

      PowerDrill = 0,

      /// <summary>

      /// Everyone's favorite

      /// </summary>

      Chainsaw,

      /// <summary>

      /// If the chainsaw is out of gas, use this

      /// </summary>

      CircularSaw

  }

Doing this will provide the developer with Intellisense hints as they code:

Quickly escape strings in xml

If you spend much time working with xml, you will find yourself needing to escape strings. Replacing '<' with '&lt;' for example. Usually the code written to do so looks something like this:

escapedItem = itemToEscape.Replace("&", "&")

                          .Replace("<", "&lt;")

                          .Replace(">", ">")

                          .Replace("'", "'")

                          .Replace("\"", """);

Though this technically works, there is an easier way built right in to the .Net framework. If we reference System.Security we can replace the above code with

escapedItem = SecurityElement.Escape(itemToEscape);

In both cases, the string

If (x < 2) & (y > 3), where \"x\" isn't...

is replaced with

If (x < 2) & (y > 3), where "x" isn't...

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.