Code can be both clean and efficient

Chapter 26 of Code Complete focuses on code tuning - the art of modifying code to improve performance. One example given is a switched loop:

  for (i = 0; i < count; i++)

  {

      if (sumType == SUMTYPE_NET)

      {

          netSum = netSum + amount[i];

      }

      else

      {

          grossSum = grossSum + amount[i];

      }

  }

Notice the 'if' statement inside the loop. If the array is rather large, this statement will be evaluate numerous times, despite the fact that the result will never change. The recommended solution is to unswitch the loop, so the 'if' statement is only evaluated once:

  if (sumType == SUMTYPE_NET)

  {

      for (i = 0; i < count; i++)

      {

          netSum = netSum + amount[i];

      }

  }

  else

  {

      for (i = 0; i < count; i++)

      {

          grossSum = grossSum + amount[i];

      }

  }

This recommendation was given with one warning: this code is harder to maintain. If the logic for the loops needs to change, you have to make sure to change both loops to match.

As with most coding tasks, there is more than one possible solution. In this case the ideal approach is to have both a single comparison and a single loop. If we throw one additional variable into the code, we can calculate the summation and then add it accordingly:

  for (i = 0; i < count; i++)

  {

      arraySum = arraySum + amount[i];

  }

  if (sumType == SUMTYPE_NET)

  {

      netSum = netSum + arraySum;

  }

  else

  {

      grossSum = grossSum + arraySum;

  }

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:

Save WinForm control values between executions

Say I have a test application that submits info to a webservice. Because this webservice has a few issues, I may need to retry a request several times to get a valid response. I've added a control to set the number of retries.


Specifying the control's Value property at design-time sets the starting value at runtime. If a user wants to set a different value, he must do so each time he starts the app. To make things more user-friendly it would be better to save the control's value between executions.

To do so, start by selecting the desired control in the designer. In the Properties window, expand the ApplicationSettings entry and select the ellipsis (...) for the PropertyBinding sub-entry.


In the Application Settings dialog that appears, select the dropdown next to the 'Value' entry, as this is the property we wish to persist.


Select the '(New...)' link from the popup, which brings up the New Application Setting dialog.


Specify the default value and name of the config setting (and modify the Scope if necessary) and press OK. This will update the Application Settings dialog to show the newly added entry.


Press OK to close the dialog. With the property binding set, the application will automatically load the value at startup. The only thing left is to save the modified value. In the form's FormClosing event handler, add the following line:

Properties.Settings.Default.Save();

At this point the change is ready to test. Start the application and modify the control's value. Without doing anything else, close the application. Now restart the app. Note the control contains the modified value.

It would seem the desired functionality is complete but there is one more item that needs to address. If you look for the saved settings file, you will find it under

C:\Documents and Settings\<username>\Local Settings\Application Data\<Company>\<AppName>\<version>

If the version number for the application is changed, the previously saved setting won't be loaded. The application needs to know to upgrade the saved settings the first time it runs.

Start by adding a new application setting called UpgradeSettings. This will be used to make sure we only upgrade the settings once. Otherwise, any newly saved settings will be replaced by the previous version's settings every time the app starts.


The final step is to add the upgrade logic to Program.cs. In the Main method, add the following code immediately before the call to Application.Run.

if (Convert.ToBoolean(Properties.Settings.Default["UpgradeSettings"]))

{

    Properties.Settings.Default.Upgrade();

    Properties.Settings.Default["UpgradeSettings"] = false;

}

No-hassle SQL connection strings

Most applications working with a database handle the connection string in one of two ways: Hard-coding the full string or doing some amount of string concatenation. A typical concatenation method looks something like this:

public string OldMethod(string server, string database,

                        string username, string password)

{

    string connectionString = "Data Source=" + server + ";";

    connectionString += "Initial Catalog=" + database + ";";

    connectionString += "User ID=" + username + ";";

    connectionString += "Password=" + password;

    return connectionString;

}

I admit this is fairly simple code. The only potential issues might be a property name typo or a misplaced (or missing) semicolon. But why hassle with even that much when the dotNet Framework has the same functionality built into the SqlConnectionStringBuilder class? With a reference to System.Data.SqlClient, the above method can be replaced with:

public string NewMethod(string server, string database,

                        string username, string password)

{

    SqlConnectionStringBuilder connBuilder 

        = new SqlConnectionStringBuilder();

    connBuilder.UserID = username;

    connBuilder.Password = password;

    connBuilder.InitialCatalog = database;

    connBuilder.DataSource = server;

    return connBuilder.ToString();

}

In either case, the output is identical:

Data Source=myServer;Initial Catalog=myDatabase;User ID=myUser;Password=myPassword

Note: If you are working with a database other than MSSQL, there are several other classes derived from the common DbConnectionStringBuilder base class, such as OdbcConnectionStringBuilder and OracleConnectionStringBuilder.

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.

Verify user membership in an AD group

Occasionally, when working on a WinForms application, I need to validate the current user against an Active Directory group before letting the application run. Though not a complex task, it usually takes me a bit of time to research (Translation: I have to copy code from several web sites and modify to suit my needs.) So I figured I'd post my own implementation here for others to use in their research. That, and maybe save myself a bit of time in the future.

The first step is to get the list of AD groups the current user belongs to. The magic necessary to make this happen is contained in the System.DirectoryServices namespace. The code to return the list looks like so:

internal static List<string> GetADGroupsForCurrentUser()
{
    List<string> groups = new List<string>();
    string domainName = Environment.UserDomainName;
    DirectoryEntry entry = new DirectoryEntry("LDAP://"
          + domainName);
    string filter = "(samAccountName="
          + Environment.UserName + ")";

    DirectorySearcher search = 
          new DirectorySearcher(entry, filter);
    search.PropertiesToLoad.Add("memberOf");

    SearchResult results = search.FindOne();
    int groupsCount = results.Properties.Count;
    for (int i = 0; i < groupsCount; i++)
    {
        string groupString = 
            results.Properties["memberOf"][i].ToString();
        groups.Add(groupString);
    }

    return groups;
}

Once we have the list, we need to loop through it looking for the desired AD group. The strings returned from the above code contain comma-delimited properties (something like "CN=MyDomainGroup,OU=Security Groups,DC=local".) Because of this I'm using the Contains method to search the string for the desired domain name. I'm sure there's another way to do this, but whatever works...

public static bool CurrentUserIsAuthorized(string authorizedGroupName)
{
    List<string> activeDirectoryGroups =
          GetADGroupsForCurrentUser();
    foreach (string group in activeDirectoryGroups)
    {
        if (group.Contains(authorizedGroupName))
            return true;
    }

    return false;
}

Wrapping a class in a singleton

My current task at work has me building a new application, using an existing framework as the base. One of the classes in this framework wraps access to the config file (which is actually kept in the database.) As soon as this class is instantiated, it retrieves the config info from the database. I wanted a way to keep a single instance around for the lifetime of the application. Otherwise, I'd be hitting the database multiple times to pull down the same information.

One option would have been to create an instance in the app's constructor. The downside to that approach is that I'd have to pass it into every method that might need it. Instead I chose to use the singleton pattern.

Below is a quick example. Marking the MyConfig class as static means it can't be instantiated. The static constructor will be called once and only once - the first time the class is accessed. This constructor creates an instance of the CommonConfig class, which can be accessed through the Config property. Now, regardless of how many times my code accesses MyConfig.Config, the database is only hit once.

    public static class MyConfig

    {

        static readonly CommonConfig _commonConfig;

        static MyConfig()

        {

            _commonConfig = new CommonConfig();

        }

        public static CommonConfig Config

        {

            get

            {

                return _commonConfig;

            }

        }

    }

Implicit conversions

I'm working on a project where request and response messages are serialized before sending across the wire. We do this by calling an override of ToString(). So a line in the code might look like

    response = myProvider.ProcessMessage(myMessage.ToString());

If I try to type

    response = myProvider.ProcessMessage(myMessage);

I get a build error stating that it cannot implicitly convert type 'SomeCoolMessage' to 'string.' So, what if I want to implicitly convert between the two? In the SomeCoolMessage class I add a new operator

    public static implicit operator string(SomeCoolMessage m)
    {
        return m.ToString();
    }

Now I can write

    response = myProvider.ProcessMessage(myMessage);

and everything just works.

WinForms testing using NUnitForms

I've been spending a lot of time lately coding screens for a WinForms application. Although I've written unit tests for the server components, the client side is somewhat lacking. Each time I change a screen I have to manually test those changes and verify I didn't break anything. In an attempt at automating the process, I've started looking at NUnitForms, an extension to NUnit.

Setup

1) Install NUnitForms
2) Create a project for your NUnit tests. Configure NUnit as normal.
3) Add references to your application project, NUnitForms, System.Windows.Forms, and any third-party controls you use on your application's forms.
4) In the test fixtures, add 'using NUnit.Extensions.Forms'

Basic forms testing

Let's say my application under test (AUT) contains a form, Form1. The first step is to create and display an instance of that form

    Form1 testForm = new Form1();
    testForm.Show();

Say the form has a button - btnOne. Pressing the button programatically looks like this

    ButtonTester btnOneTester = new ButtonTester("btnOne");
    btnOneTester.Click();

Upon clicking this button, a label was updated with the everyone's favorite power tool. To verify

    LabelTester labelOneTester = new LabelTester("labelOne");
    Assert.AreEqual("Chainsaw", labelOneTester.Text, "Label text doesn't match expected");

Though it's not required, it's probably a good idea to close the form instead of letting it fall out of scope

    testForm.Close();

Using the hidden desktop

Running these tests on your local machine doesn't pose any issues. If you run them on a build server, however, you'll likely find that they fail - there's no window for the forms to be displayed in. Fortunately, you can send them to a hidden desktop.

To do so, your test fixture must inherit from NUnitFormTest. Then, add the following property to the class

    public override bool UseHidden
    {
        get { return true; }
    }

One warning - In most of my test fixtures, I tend to put common code in a [Setup] method. I'm finding that when I have this defined, forms are not being sent to the hidden desktop. I've not spent much time on it, so I'm not sure if it's something I'm doing wrong, or if it's a bug in the tool. I'll post more info if/when I figure it out.

Compiler warnings are a Bad Thing

Most projects I've worked on contained a sizeable number of compiler warnings. These are usually ignored by developers as being unimportant. So there are a few (dozen) variables defined that are never used - so what? Duplicate 'using' statements - that can't hurt anything...

It's true that some warnings are mostly harmless. The problem is that these warnings often hide other, more important ones. A few serious ones that come to mind:

- Unreachable code. Perhaps there's a return statement halfway through a function. Or a conditional that will always evaluate true (or false.) These often have unintended side effects.

- A variable is never assigned to, and will always have a default value. You've created a variable, foo, that you're using in your code, perhaps in a method call or conditional. The problem is, you never set foo to anything. It's possible the variable should be removed or replaced with a constant. But it's also possible you forgot to retrieve or calculate something.

- A class member hides an inherited member. Maybe you intended to replace it with your own. In that case, use 'new' to explicitly document the desired effect. If hiding was unintentional, you're better off renaming the member. Otherwise, you may later decide you need that hidden item. At which point you end up with a bit of code rework.

Always address failing unit tests

I cannot stress enough the value of unit tests. Whether adding new features or refactoring existing code, you want some validation that you haven't broken existing functionality. Unit tests are usually the fastest way to accomplish this.

This is why it annoys me when unit tests are left in a failing state. In some cases, the tests are invalid and need to be removed. In other cases, they need to be updated to match code changes. Regardless of why a test now fails, it needs to be corrected.

On a project I'm currently involved in, there was a significant rework of data access code. As you can imagine, this introduced a number of bugs, many of which were caught by now-failing unit tests. This is a good thing - that's what the unit tests are for. The problem is that the 50 newly-failing unit tests are mixed in with the 40 already-failing unit tests. We need the new failures fixed quickly, but sorting these from the "junk" unit tests will take time.

Notes On Unit Testing

Lately, I seem to be spending a lot of time writing and updating unit tests. Along the way, I've made a number of observations. The syntax I'm using is taken from NUnit, but the ideas apply to any unit test framework.

1) If you choose to Ignore a unit test, note why it's being ignored and optionally how it is to be addressed. In NUnit, the attribute looks like this:

 [Ignore("We can't run this test until the hardware is in place")]

Not only is it easier for a dev to figure out later, but it will show up in the build report, making it easy to see the reason without opening up the code.

2) Similarly, use comments in all Assert statements noting the reason for the failure. Again the NUnit syntax

 Assert.AreEqual(expectedAge, actualAge, "Actual age doesn't match expected");

Without this, the NUnit report tells you that 12 didn't match 45, but it doesn't tell you whether you were comparing age or IQ.

3) Never copy/paste code. We all follow this rule in production code, but it is often ignored when writing unit tests. If every test requires a login before doing anything useful, move the login code to a separate method and mark it with a SetUp attribute. If some tests need to fail login, leave off the Setup attribute, but call the method from those unit tests that need it. Note also that you can use Asserts in these methods, so you can still verify proper execution within the methods.

4) Use constants whenever appropriate. If every unit test accesses the same server, move the server name to a string constant. This way, when you later shift to a new test box, you only replace one string.

5) Use a TearDown method to clean up after a test. Largely, this is to reduce the copy/paste of code (see item #3.) This also has to do with not using "try/finally" blocks. It is true that a failed Assert statement is simply throwing an exception. And technically, you could do code cleanup inside a finally block. But this isn't a clean solution (which is why the TearDown attribute exists.) One note: you may have to do some checking within the method. You wouldn't want to dispose a null object.

6) Never hard-code expected values from a database. I've seen numerous unit tests similar to:

 Assert.AreEqual(14, user.ID);

Primary keys are particularly problematic, but any field could cause issues, especially if users (or other unit tests) update values in the table.

One approach is to pull data using some other means. Perhaps by using inline SQL. Perhaps there's another stored proc or service that returns the same info. Either method means the tests don't need to be updated simply because the data changes.

A second approach is to use mock objects and avoid real data altogether. Assuming you've coded to an interface, you can use a tool like NMock or Rhino.Mocks to simulate data retrieval.

7) Test for exception cases using the ExpectedException attribute. There's no need to write a try/catch block, just to verify that a particular exception was thrown. ExpectedException can handle that for you, saving time writing code. One argument against it is that you can only test one exception case per test. You could instead have multiple try/catch tests within a single unit test. The problem here is that NUnit logs one error per test. If you have multiple errors, only one will be logged. You won't notice the other issues until the first has been addressed.

And Most Importantly:

8) Fix broken unit tests (Keep the unit tests current.) I've seen instances where unit tests were written to verify current behavior, but were then ignored as the code was modified. A large portion of a unit test's value comes from its ability to monitor code, flagging errors as soon as they are introduced.