Home > ASP.NET, Extreme Programming, Refactoring, Software Development, TDD, Test Driven Development > Refactoring Code: A Programmers Challenge Part 2

Refactoring Code: A Programmers Challenge Part 2

Kevin Jones: Refactoring

April 9th 2009 | Kevin Jones

Refactoring Code: A Programmer’s Challenge

In a previous blog post– Refactoring Code: A Programmer’s Challenge Part 1—we went over refactoring basics, and took a not-so-easily-tested piece of code and made it easy to test.

Our final bit of code was this:

public class DisplayPanelDecider
{
    public bool ShouldDisplayPanelOnPage(string pageName)
    {
        string[] pagesToHideElement = new[]
        {
            "/index.aspx",
            "/sitemap.aspx",
            "/404.aspx",
            "/status.aspx",
            "/online_help.aspx",
        };
        return !Array.Exists(pagesToHideElement, s => s == pageName);
    }
}

The objective of the code is clear: “Given the name of a page, should I display a panel?”

Furthermore, the code is compact and does no more or less than it should – so we can test it easily.

Some of our tests would look like this:

[TestClass]
public class DisplayPanelDeciderTests
{
    [TestMethod]
    public void ShouldDisplayThePanelIfThePageIsNotInTheExclusionList()
    {
        DisplayPanelDecider decider = new DisplayPanelDecider();
        Assert.IsTrue(decider.ShouldDisplayPanelOnPage("/foobar.aspx"));
        Assert.IsTrue(decider.ShouldDisplayPanelOnPage("/main.aspx"));
        Assert.IsTrue(decider.ShouldDisplayPanelOnPage("/blog.aspx"));
    }

    [TestMethod]
    public void ShouldNotDisplayThePanelIfThePageIsInTheExclusionList()
    {
        DisplayPanelDecider decider = new DisplayPanelDecider();
        Assert.IsFalse(decider.ShouldDisplayPanelOnPage("/index.aspx"));
        Assert.IsFalse(decider.ShouldDisplayPanelOnPage("/map.aspx"));
        Assert.IsFalse(decider.ShouldDisplayPanelOnPage("/status.aspx"));
    }
}

An interesting side note: Did you notice that the names of our tests start with the word “should?” It’s such a simple thing, but the name of tests is important. One should be able to figure out the purpose of the test by reading its name. Using the prefix “should” forces you to think about the test name.

But as we know, software requirements grow and change. What’s happened to our software developer a few months down the line? (Put on your pretend cap!)

Well, at this point, his list of pages grown considerably. Rather than the handful we have in our code, we now need to hide the panel for many pages. As an additional requirement, it now needs be configurable too, so compiled code is not the best solution.

A natural place to put configuration for now is in the appSettings section of the web.config file, and for simplicity sake, we’ll separate our pages with a semicolon, so they’ll look like this:

<configuration>
    <appSettings>
        <add
key="PagesToHidePanel"
value="/index.aspx;/map.aspx;/404.aspx;/status.aspx;/online_help.aspx"/>
    </appSettings>
</configuration>

Now, we need a class that is responsible for retrieving these page names and parsing them. Rather than throw that code in our existing class, introduce a new dependency.

The implementation I came up with looks like this:

public interface IDisplayPanelExclusionProvider
{
    string[] GetPageNames();
}

public class DisplayPanelExclusionProvider : IDisplayPanelExclusionProvider
{
    public string[] GetPageNames()
    {
        string unparsedContent =
            ConfigurationManager.AppSettings["PagesToHidePanel"];
        string[] pageNames = unparsedContent.Split(';');
        return pageNames;
    }
}

Notice I created an interface. This will have a key role later on. The next step is getting our two classes, DisplayPanelExclusionProvider and DisplayPanelDecider talking to each other.

The constructor is a simple and useful approach to dependency injection. Our goal here is to get the DisplayPanelDecider to ask the DisplayPanelExclusionProvider, “On which pages should I not display this panel?”

We’ll modify the DisplayPanelDecider to take in a IDisplayPanelExclusionProvider. Again, notice I used the interface in this example. So our class now looks like this:

public class DisplayPanelDecider
{
    private readonly IDisplayPanelExclusionProvider _displayPanelExclusionProvider;

    public DisplayPanelDecider(IDisplayPanelExclusionProvider displayPanelExclusionProvider)
    {
        _displayPanelExclusionProvider = displayPanelExclusionProvider;
    }

    public bool ShouldDisplayPanelOnPage(string pageName)
    {
        string[] pagesToHideElement = _displayPanelExclusionProvider.GetPageNames();
        return !Array.Exists(pagesToHideElement, s => s == pageName);
    }
}

Our DisplayPanelDecider now asks the IDisplayPanelExclusionProvider for a list of pages. However, by introducing our constructor, we broke our tests! The next step is to start getting our tests working again.

Important note: even though our class examples seem contrived, we are keeping decision-making logic separate. This is important from the testability aspect as well as for future maintenance. The key part here is single dependency, and that applies for our tests as well. We don’t want our tests testing multiple classes at once: keep them separate.

This is where the interface saves us: we can inject our own classes for the purpose of testing.

[TestClass]
public class DisplayPanelDeciderTests
{
    private class MockDisplayPanelExlusionProvider : IDisplayPanelExclusionProvider
    {
        public string[] GetPageNames()
        {
            return new[]
                   {
                       "/index.aspx",
                    "/map.aspx",
                    "/404.aspx",
                    "/status.aspx",
                       "/online_help.aspx"
                   };
        }
    }

    [TestMethod]
    public void ShouldDisplayThePanelIfThePageIsNotInTheExclusionList()
    {
        DisplayPanelDecider decider = new DisplayPanelDecider(new MockDisplayPanelExlusionProvider());
        Assert.IsTrue(decider.ShouldDisplayPanelOnPage("/foobar.aspx"));
        Assert.IsTrue(decider.ShouldDisplayPanelOnPage("/main.aspx"));
        Assert.IsTrue(decider.ShouldDisplayPanelOnPage("/blog.aspx"));
    }

    [TestMethod]
    public void ShouldNotDisplayThePanelIfThePageIsInTheExclusionList()
    {
        DisplayPanelDecider decider = new DisplayPanelDecider(new MockDisplayPanelExlusionProvider());
        Assert.IsFalse(decider.ShouldDisplayPanelOnPage("/index.aspx"));
        Assert.IsFalse(decider.ShouldDisplayPanelOnPage("/map.aspx"));
        Assert.IsFalse(decider.ShouldDisplayPanelOnPage("/status.aspx"));
    }
}

Note that we are injecting a fake class that allows us to implement a hand rolled mock. The single purpose of this test class is to ensure that the DisplayPanelDecider decision logic works. It isn’t the DisplayPanelDecider’s responsibility to know the real list of pages from our configuration. That should be tested somewhere else. In effect, we are testing against dummy data. When we want our class to use the real provider, we just create an instance of that and hand it in. In fact, with constructor chaining we can implement a default constructor that uses the real provider:

public DisplayPanelDecider() : this(new DisplayPanelExclusionProvider())
{
}

We have the option of using the DisplayPanelDecider with the real provider, or handing in our own – which we do for testing.

Now the thought on most people’s mind at this point is most likely “This seems like overkill.” If you stand back and look at it for all it is now, I might agree. However, maybe the programmer we’re helping finds himself with more challenging requirements. The other part is the hand rolled mock.

Rewinding a bit, in our new tests we saw a dummy class that provided fake data for the sake of testing. There are mock frameworks that remove the need for these dummy classes, but we’ll examine that in a future blog post. If you’re up for some homework, check out Rhino Mocks.

I pose this question to you: What advantages and disadvantages do you see with our approach? Is it worth the effort? I’d love to know what you think.

Kevin Jones is a Senior .NET developer and Product Team Leader at Thycotic Software Ltd. Thycotic is recognized for providing Agile TDD Training and Agile .NET Consulting Services, and its flagship password management software Secret Server. On Twitter? — > Follow Kevin

  1. April 10, 2009 at 7:48 am

    Now that you test against a mock with dummy data, do you really need more than two items in your white list?

  2. Stupid picture
    September 15, 2011 at 12:21 pm

    Stupid picture: the gears won’t turn.

  1. No trackbacks yet.

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s

%d bloggers like this: