Finding the Seams – Applying the Single Responsibility Principle to Get Code Under Test

Last night, I attended Cincy Clean Coders and ended up having a fairly long conversation with another attendee about testing legacy code. In her particular case, she had a component that needed to be put under test, but she was having trouble figuring out how to get it done. As is often the case with legacy code, what we found was that the component in question had too many (read >1) responsibilities and, by identifying those responsibilities and splitting them out into appropriate classes, we could in fact test the code in question.

Here’s an incredibly simplified version of the class in question (which, in real life, created many different message types, but in the end always sent an XML document):

using System;
using System.Xml.Linq;
using System.Collections.Generic;

namespace MessageManager
{
	public class MessageCreator
	{
		private static Queue messageQueue = new Queue();

		public static XDocument GetNextMessage ()
		{
			return messageQueue.Dequeue ();
		}

		public MessageCreator ()
		{
		}

		public void CreateAndSendMessage(int id)
		{
			XDocument message = this.CreateMessage(id);
			this.SendMessage(message);
		}

		private XDocument CreateMessage(int id)
		{
			var data = this.GetDataFromDatabase(id);
			data.Element ("Document").Add (
				new XElement("AdditionalData", "Some Other Data")
				);
			return data;
		}

		private XDocument GetDataFromDatabase (int id)
		{
			// In the real world, this actually calls a stored procedure which returns XML
			var doc = new XDocument(
				new XElement("Document",
			             new XAttribute("Id", id)
			             )
				);
			return doc;
		}

		private void SendMessage(XDocument message)
		{
			// Send the message across the wire to somewhere
			// This message mutates state in another system, which is bad for unit testing the MessageCreator
			// for now, just add the message to an in-memory queue
			MessageCreator.messageQueue.Enqueue (message);
		}
	}
}

As you can see, there are three related but different responsibilities in this code. First, we load some data from the database (in XML format). Next, we add some additional data to the base message, and then we send the message to the external system. Unfortunately, sending that data alters the state of the external system, and any other developer hitting that system will run into problems with their local systems being out of sync.
So, the first goal is to break apart the sending of messages from the creation of messages, which ends up being a fairly simple task. However, as with any changes to legacy code, if we can create an integration test to prove we’re not breaking existing functionality while refactoring, we should do that. Therefore, we start with an end-to-end integration test that clarifies how the system works today and ensures that the appropriate message is sent:

[TestFixture()]
public class MessageManagerIntegrationTests
{
	[Test()]
	public void TestMessageSending ()
	{
		var creator = new MessageCreator();
		var otherData = "Some Other Data";
		var id = 1;
		creator.CreateAndSendMessage(id, otherData);
		var message = MessageSender.GetNextMessage ();
		var root = message.Root;
		Assert.That (root.Attribute("Id").Value, Is.EqualTo (id.ToString()));
		Assert.That (root.Element ("AdditionalData").Value, Is.EqualTo(otherData));
	}
}

With our first integration test in place, we can attempt to break appart some of the functionality of the MessageCreator for better testability. Our next step is to move the sending of messages to a separate class, and implement an interface that we can later use to mock the message sending.

public interface IMessageSender
{
	void SendMessage(XDocument message);
}

And the implementation:

public class MessageSender: IMessageSender
{
	private static Queue messageQueue = new Queue();

	///
<summary>
	/// Gets the next message on the queue. Note that in reality, this class sends messages to an external system,
	/// and validating the message send is more difficult than merely checking a queue.
	/// </summary>
	///
	/// The next message.
	///
	public static XDocument GetNextMessage ()
	{
		return messageQueue.Dequeue ();
	}

	public MessageSender ()
	{
	}

	public void SendMessage(XDocument message)
	{
		// Send the message across the wire to somewhere
		// This message mutates state in another system, which is bad for unit testing the MessageCreator
		// for now, just add the message to an in-memory queue
		messageQueue.Enqueue (message);
	}
}

Now, we can use a mock implementation of IMessageSender in our unit tests without actually sending data to the external system and getting a developer’s local database out of sync with the other system. Using Moq as our mock framework, this might look something like:

[TestFixture()]
public class MessageManagerTests_CreateAndSendMessage
{

	int id = 1;
	string otherData = "Some other data";
	XDocument result;

	[SetUp()]
	public void SetUp ()
	{
		var sender = new Mock(MockBehavior.Strict);
		sender.Setup (s => s.SendMessage(It.IsAny()))
			.Callback(d => result = d);
		var creator = new MessageCreator(sender.Object);
		creator.CreateAndSendMessage(id, otherData);
	}

	[Test]
	public void should_add_correct_id ()
	{
		Assert.That (result.Root.Attribute ("Id").Value, Is.EqualTo (this.id.ToString ()));
	}

	[Test]
	public void should_set_other_data ()
	{
		Assert.That (result.Root.Element ("AdditionalData").Value, Is.EqualTo (this.otherData));
	}
}

f you’d like to see the complete example, including all the commits along the way, you can check out the code at https://github.com/JeetKunDoug/LegacySeamsExample. In that repository, I’ve applied the same techniques to the data access portion of the MessageCreator class, so that our unit tests can now run without access to a database at all.

Working with legacy code can be a daunting task, but with some simple, mechanical refactorings, and the support of unit/integration tests, you can get your code under test.

Pragmatism Killed the Codebase

As a software developer, how many times have you heard:

• We don’t quite have time in the budget to do full-blown TDD, so be pragmatic.
• We do “pragmatic pair programming”
• We do Scrum, but some things just don’t work in our environment, so we are pragmatic about it.

So you join a new project and start hearing these things.  You think to yourself “sure, being pragmatic is good – being realistic with your code quality is important, and, really, we can’t do everything perfectly.”

A few months go by, and you realize that you’ve become very pragmatic.  So much so that you’re only writing unit tests if you think it’ll be really easy to do and nobody will ask you why task X took so much time to complete.  You’ve given up on two of the three stages of “Red->Green->Refactor” as the unit tests have been failing for the last month, and build breaks don’t really phase you any more.

Unit test coverage continues to decline, and tests fail for no reason at all other than someone sneezed near the build machine.  You keep shoehorning new features into the existing application without building characterization tests around the functionality you’re modifying, figuring that it’s “too hard” to find seams in the existing codebase into which you can insert mocks or stubs.

The few “unit tests” you do have take several hours to run, and can really only be run on the build server.  Therefore, people check in code arbitrarily to the main branch just to see if the tests pass or not.  Of course, when they don’t pass, they take several days to get fixed because the tests fail in ways that takes significant time and effort to debug and fix.  And that’s only if the thing that broke is actually covered by a unit test.  Otherwise, you don’t find out for weeks.

So a year’s gone by, and now you have an extra 20,000 lines of production code, and 7 more unit tests which are testing the data source for a combo box on a form (since nothing else is “pragmatically testable”) and your regression testing cycle (which is completely manual of course) takes 6 weeks and counting, as more and more regression defects are being introduced with each successive build/release.

I don’t know about you, but “pragmatic” seems to me to be the easiest way to pay lip service to “doing the right thing” without actually doing it and, more insidiously, accusing those who want to really do whatever that is of being dogmatic and unreasonable about whatever topic is currently being discussed.

Don’t let pragmatism kill your codebase!

Brownfield projects and Sprint Duration (or, is the 50-yard dash to short?)

I’m working on a brownfield .Net project where the original codebase was started about 10 years ago, and has, like most systems, evolved organically over time.  Additionally, the code in question is from an acquisition, so many of the developers (myself included) have had very little exposure to the application.

We originally settled on 2-week sprints, but, given the complexity of the application, lack of unit tests, and general lack of knowledge on the team, we struggled to get any real user story completed from a development perspective within a sprint, much less tested, signed off, and “done”.  Things I’ve noticed that lead me to question 2-week sprints (which have been helped somewhat by the extra week, but not completely) include:

1. Implementation of any non-trivial user story requires a tech spike in one iteration to determine how it will be accomplished, followed by one or more additional iterations to complete the story itself.
2. Simple things seem to take way too long (even if they fit in a sprint, our estimates are way shorter than actuals).
3. It’s next to imposible to really understand the number of places within the application that are affected by what seems like a simple change.  Again, given the nature and history of the application, the lack of unit tests/documentation/SOLID design principles, every code change can require several days of “archeology” to even have a hope that we’re not breaking something else in the system.
4. Even simple user interface changes can have fairly far-reaching consequences, as there are so many places where classes have been hacked to be used in places for which they weren’t originally designed.

So, we’re trying to adjust by doing several things:

1. The move from 2-3 week sprints so we can do investigation and development (and hopefully testing) in one sprint.
2. Slowly adding seams for unit tests, and, when time permits, creating characterization tests (ala Working Effectively with Legacy Code).
3. Trying to break down user stories into smaller pieces to help make achieving “done” doable within a sprint.

These things are starting to pay off, but if anyone has more suggestions on ways to make this process more productive I’d love to hear them.  I’ll post updates as time goes on if there’s any new techniques we figure out along the way.