What Is Your Legacy?

Between a year and 18 months ago a former Dev Leader who managed me recommended the book "Working Effectively With Legacy Code" by Michael C. Feathers. It turned out to be a very good recommendation, given that I've spent the last year working on and around a legacy codebase. In the book Feathers gives his definition of what legacy code is, which he accepts is controversial. He says that it is code without unit tests. The book explains why he believes that to be the case and I'm not going to go into it here - I would wholeheartedly recommend it though. Having looked through some of the new code that we have created recently I've come to believe that some code is more 'legacy'* than other code.

Here's an example**:

if (order.OrderTypeId == Order.OType.NORMAL && processingStack.CanBeProcessed(order.Id, maxProcessingAttempts, minutesDelayBeforeProcessing))
{
da = null;
if (order.Lock(moduleUserId, ref rl, -1, ref da, ref pe))
{
CancelAuthorizedOrders(order, moduleUserId, countries, providerConfig, ref processingStack, maxProcessingAttempts);
da = null;
order.Unlock(moduleUserId, ref da);

LoggingModule.Increment(LoggingModule.LogCategory.Orders, LoggingModule.LogCounter.Cancel);
}
}

Aside from the fact that this snippet sits within a public method that has no tests (immediately then, according to Feathers, legacy code), what stands stands out about this code? How easy is it going to be for future developers to read, understand and maintain?

I'm going to break down why I think this is so bad, starting with the first line:

if (order.OrderTypeId == Order.OType.NORMAL && processingStack.CanBeProcessed(order.Id, maxProcessingAttempts, minutesDelayBeforeProcessing))

Why not have a boolean variable with a meaningful name for your second condition here, which would make the 'if' statement much smaller and more readable?
Something like:

bool canBeProcessed = processingStack.CanBeProcessed(order.Id, maxProcessingAttempts, minutesDelayBeforeProcessing);
if (order.OrderTypeId == Order.OType.NORMAL && canBeProcessed)
{
...
}

Since we're doing that, why not do the same for the first condition as well e.g.

bool canBeProcessed = processingStack.CanBeProcessed(order.Id, maxProcessingAttempts, minutesDelayBeforeProcessing);
bool orderIsNormalType = order.OrderTypeId == Order.OType.NORMAL;
if (orderIsNormalType && canBeProcessed)
{
...
}


I think that is much more readable and easier to debug should you need to. Of course you can debate whether you need to put in a variable or not depending on the circumstance. Had the 'if' conditional been "if (order.OrderTypeId == Order.OType.NORMAL)" only, I would almost certainly have left it alone, since that is simple to read and understand.

Next, variable names like 'da' and 'pe' and most heinously 'r1'. Is that 'rOne' or 'rEl'? What does it reprasent? It's declared 45 lines away. Give your variables meaningful names so the developers who inherit your legacy have less to do.

Next: if (order.Lock(moduleUserId, ref rl, -1, ref da, ref pe))
What is -1 here? Why do I have to look at the signature of the 'Lock' method to figure out what you're doing here? Magic numbers, meh!

None of the above suggestions change anything about how the code works, they are just about making it more readable. But code that is more readable is eminently easier to maintain. Of course if you have vast methods, within vast classes and no unit tests then maintaining and refactoring them is going to be difficult, but starting with good, clear names and readable code is a great first step towards helping those that follow after you.


* 'Legacy' being a euphemism for s***.
** Some method and variable names have been changed to protect the innocent.

Boolean Polymorphism

(AKA Mangling Your Understanding Of OO With Flags.)

For the past few months I've been working on the partial translation of, and integration with, a legacy VB codebase. The new code is all in C# but on occasion it has to call the old code. During this time I've seen a number of methods that have multiple boolean flags as parameters. For instance this classic call: "OrderTotalIncludingTax(order, true, true, false, false, false, true)"*.

Fundamentally what this means is that your method - OrderTotalIncludingTax - is doing more than one thing, in this case there are a possible 64 options (the number of options a boolean gives you to the power of the number of booleans in the parameters; nice). The method itself doesn't have that many execution paths in it - in fact it has 6 - but the point remains, you have a method that does more than one thing. Take a look at "Clean Code" pp35** for more detail on why this is "A Bad Thing".

So what's the connection between this and polymorphism? Well, according to Wikipedia polymorphism is "... the ability of one type, A, to appear as and be used like another type, B". In practical terms this means that we can hide the implementation of a class - and therefore its methods - from whatever calls it. Which means that at runtime we can get whatever implementation we want. For instance, in some circumstances we want to get data from a .csv file, in others an .xml file. Put the implementations behind an interface and you don't need to make the decision about which file you want to read until runtime and can easily use whichever implementation is needed at the time.

But ... another way to achieve that would be to have a boolean parameter called "dataSourceIsCsv" and have an "if ... else" clause in your code. Pass in 'true' and the 'if' clause means you execute code that uses a .csv file, otherwise code that uses an .xml file. Brilliant, job done! All the benefits of polymorphism and you've kept your code nice and readable - after all, who can't understand a simple 'if' statement? Hence "boolean polymorphism" - implementation neatly tucked away from the client code.

If you don't understand why this is a problem then I suggest you start thinking about the 'Open/closed' principle. In the meantime I'm going to try to stop people from using flags as parameters.


*names have been changed to protect the innocent, but the signature is absolutely what's found in the code.
** Clean Code, Robert C. Martin. Prentice Hall.

NUnit and config - part 3: the strategy

So far on this blog I've posted two articles (is that what blog posts are, 'articles'?) (1 & 2) about handling config issues with NUnit. As I was writing them it became apparent to me that while I was using a particular set of tools (the MSBuild command line tasks in Visual Studio, CruiseControl and NAnt) there was a higher level strategy that they were a solution to. I think there's value in spelling out that strategy. I've made it very, very simplistic, so that even I can understand it when I read it back in 6 months time ...

First, the problem: Unit testing code that relies on values from configuration files is problematic, since NUnit operates outside the context of those config files.

The solution: NUnit has the facility to read its own config file in its own context (i.e. when the tests are running), therefore we should copy our config to that file.

The strategy:

• Copying files should not be done manually as this is almost guaranteed to screw up - it's also not logically possible for an automated build.
• The automatic process will have to rename the config file to match the NUnit project dll name.
• Any configuration sections that have been split out into separate files will have to be copied across as well and the relative paths to them maintained.
  
• Each project to be tested must have its own test project. For instance if you have a 'web' project, you should have a 'web.tests' project. Likewise if you have 'DataLayer' project you should have a 'DataLayer.tests' project. This means that you will not have to try to merge configuration from different projects into a single test project configuration.
  

I think that's the crux of it and I know it sounds simple, but I've seen plenty of projects where code isn't tested because of config issues, which frankly, is not a good enough reason.

NUnit and config - part 2: automated builds

Yesterday I ran through an example of how to use MSBuild BuildEvents to copy config files so that NUnit has access to them. Today I'm going to take that a step further and go over how I do a similar thing for my automated builds.

For the MVC project I mentioned yesterday I am using tortoise SVN for version control and CruiseControl.net for automated builds. Given that it's only me on the project this might seem like overkill, however I believe that it's good practice to use continuous integration for every project and SVN makes me feel more secure about my code (although to feel really happy I should probably have a separate server that houses SVN ... but that's a different geek conversation altogether). Anyway, the point being that when I commit changes to my code a build is triggered via CruiseControl (using an MSBuild task) and my unit tests are run (via an NUnit task). Of course the problem with this is that when the NUnit project builds on the build server it doesn't have access to the config and the tests fail. This makes my CCTray icon go red because the build has failed and this upsets me.

The solution, once again, is to use a machine to copy the relevant config to the right locations. In my case, because I'm comfortable with it, I'm using a NAnt task after my MSBuild task in my ccnet.config file that calls a NAnt build file that copies the relevant project config files to the NUnit project. An NUnit task then runs the tests (see the code below)
CCNet.config:

            <nant>
               <executable>C:\Program Files\nant-0.85\bin\nant.exe</executable>
               <baseDirectory>H:\ccbuilds\[my project name]\build</baseDirectory>
               <buildFile>copyConfig.build</buildFile>
           </nant>

NAnt build file to copy config files:

<?xml version="1.0"?>
<project name="[my project name]" default="copy">
   <target name="copy" description="copy config files required for unit tests">
       <copy
           file="svnroot\[my project namespace].web\web.config"
           tofile="svnroot\[my project namespace].tests\bin\Release\ogilvieinternational.tests.dll.config" />
       <copy file="svnroot\[my project namespace].web\Configuration\CssConfiguration.config"
           tofile="svnroot\[my project namespace].tests\bin\Release\Configuration\CssConfiguration.config" />
   </target>
</project>

Hopefully you can see that the NAnt build file just tells NAnt to move the relevant config files (and rename them where necessary) from the main project to test project in much the same way the MSBuild command line tasks did for the local version of the build.

In order to really get to grips with this stuff I needed to get my head around using CruiseControl and NAnt, but I would very much recommend doing so as they are incredibly powerful and make your life so much easier. Having said that, the tools used to achieve this are not what's important. What's important is the strategy, which (having written all this down) I now have more of an understanding of, so I'll note that down in part 3 of this series of blog posts.

NUnit and config - part 1

As part of an MVC project I'm working on at home I hit an issue that consistently comes up when unit testing apps with NUnit, that being how you handle testing parts of your code that rely on configuration files. Often this comes up when you are dealing with your Data Access Layer (DAL) since you'll have a connection string that is held in config that is needed in order to access your database. However in this instance my app doesn't yet have a DAL (it's only a baby, just me playing with the MVC framework) it does however have some magic strings to do with the CSS of the menu. Rather than keep those magic strings in code I prefer to keep them in my config files.

In a sense these are actually integration tests, since I need to test that my code integrates as expected with my config. Leaving that aside for a moment, the actual issue is this: in order for the code to work it needs to retrieve a value from config. NUnit cannot read the Web.config file of your app, so the tests fail. To give a bit more technical context my Web.config file has a section declared in it like this:

<CssConfiguration configSource="Configuration\CssConfiguration.config" />

As you can see this then refers to a file that called CssConfiguration.config that lives in a folder called Configuration in my project. Along with this are two classes:

    public static class WebConfiguration
 {
     public static CssConfigurationManager CssConfiguration
     {
         get { return (CssConfigurationManager)ConfigurationManager.GetSection("CssConfiguration"); }
     }
 }

and

 public sealed class CssConfigurationManager : ConfigurationSection
 {
     [ConfigurationProperty("whyCssClass", DefaultValue = "", IsRequired = false)]
     public String WhyCssClass
     {
         get
         { return (String)this["whyCssClass"]; }
     }

     [ConfigurationProperty("leadCssClass", DefaultValue = "", IsRequired = false)]
     public String LeadCssClass
     {
         get
         { return (String)this["leadCssClass"]; }
     }

(etc etc)
which allow a single point of programmatic access to the config elements.

Which leads me, finally, to the point of this whole exercise. This code needs tests (in fact the tests were written first, but that is not important now) and the test code looks like this:

[TestFixture]
  public class CssConfigurationManagerTests
  {
      private CssConfigurationManager _cssConfigurationManager;

      [SetUp]
      public void SetUpTests()
      {
          _cssConfigurationManager = WebConfiguration.CssConfiguration;
      }

      [Test]
      public void WhyCssClassValueIsReadCorrectly()
      {
          Assert.That(_cssConfigurationManager.WhyCssClass == "why");         
      }

      [Test]
      public void LeadCssClassValueIsReadCorrectly()
      {
          Assert.That(_cssConfigurationManager.LeadCssClass == "lead");
      }

and so on for each element to be tested. And guess what? The tests fail, because when NUnit runs it has no idea about the config that has been set up. The simplest solution I found searching around the internet was to copy the config file to the location of the dll of the test suite and rename it [testsuitename].dll.config. This will work, however it is subject to idiots like me forgetting about it, doing it wrong and generally messing it up. A better solution is to get a machine to do it for you (which is, let's face it, why we're programmers).

In the Build Events section of the Properties of the project you wish to test (not the test project), you will need to put copy commands into your Post-build event command line box. In my case they looked like this:
copy "$(ProjectDir)Web.config" "$(ProjectDir)..\[my project name].tests\bin\Debug\[my project name].tests.dll.config" /y

xcopy "$(ProjectDir)Configuration" "$(ProjectDir)..\[my project name].tests\bin\Debug\Configuration" /s /i /y

The first line here copies and renames your Web.config file into the config file for the dll of your tests. The second line copies the 'Configuration' folder that contains the config section for the CSS config management. I'm not too clued up about the DOS copy and xcopy commands so it could be useful to look into those in more detail ... but it seems to work.

Hopefully this approach ensures that the config used by both your tests and your actual code is always the same which should make config less brittle and susceptible to ill thought through tampering.

The second part of this blog post will be about how you then get all this to work with your automated builds, hopefully making the process even more robust.