Refactoring: Extracting the Singleton Pattern

With this post I will be starting a series of posts on refactoring. The purpose is to use real production code (whenever possible) as a start point and improve it, instead of just random pointless examples.

Starting point

So the main class we will be changing is the DeviceManager. This class manages a collection of Devices (or a dictionary, to be more precise) and allows us to set which is the active device and to get a device by id. Note that this last feature could be handled by the Devices collection, but we are taking advantage of the dictionary to make that look-up more efficient.

One of the key points is that it has a singleton Instance property, that we use throughout the code.

namespace PSampaio.Refactoring.Example1.Old
{
	using System.Collections.Generic;
	using System.Linq;

	public class DeviceManager
	{
		private static DeviceManager deviceManager;
		private static readonly object singletonLock = new object();
		private readonly Dictionary<string, Device> devices;

		public static DeviceManager Instance
		{
			get
			{
				lock (singletonLock)
				{
					if (deviceManager == null)
					{
						deviceManager = new DeviceManager();
					}
					return deviceManager;
				}
			}
		}

		public Device ActiveDevice { get; set; }

		public IEnumerable<Device> Devices
		{
			get { return devices.Values.AsEnumerable(); }
		}

		public DeviceManager()
		{
			devices = new Dictionary<string, Device>();
		}

		public void AddDevice(Device device)
		{
			if (!this.devices.ContainsKey(device.DeviceId))
			{
				this.devices.Add(device.DeviceId, device);
			}
		}

		public bool RemoveDevice(Device device)
		{
			var result = false;
			if (this.devices.ContainsKey(device.DeviceId))
			{
				result = this.devices.Remove(device.DeviceId);
			}
			return result;
		}

		public Device GetDeviceById(string id)
		{
			Device device;
			if (!devices.TryGetValue(id, out device))
			{
				device = null;
			}
			return device;
		}
	}
}

Then we have the actual Device. This class was edited down to the basics, as the real behavior of the Device is not important. For example purposes, it just has an Id that we use to index it in the dictionary.

namespace PSampaio.Refactoring.Example1.Old
{
	public class Device
	{
		public string DeviceId { get; private set ; }

		public Device(string id)
		{
			DeviceId = id;
		}
	}
}

The ActiveDeviceSupport class is the one that uses both the DeviceManager and the Devices. Again, for example purposes, this only sets the first Device we have on the DeviceManager as the ActiveDevice.

namespace PSampaio.Refactoring.Example1.Old
{
	using System.Linq;

	public class ActiveDeviceSupport
	{
		public void SetFirstDeviceAsActive()
		{
			var devices = DeviceManager.Instance.Devices;
			var firstDevice = devices.FirstOrDefault();
			if (firstDevice != null)
			{
				DeviceManager.Instance.ActiveDevice = firstDevice;
			}
		}
	}
}

And finally, we have the set of unit tests run against the DeviceManager. In case you are wondering, yes, these are the actual tests (albeit being slightly edited to protect the innocent) that we have for these classes.

namespace PSampaio.Refactoring.Example1.Old
{
	using System.Collections.Generic;
	using System.Linq;

	using Microsoft.VisualStudio.TestTools.UnitTesting;

	[TestClass]
	public class Tests
	{
		[TestMethod]
		public void AddDevice()
		{
			var devices = new List<Device>();
			for (var i = 1; i < 5; i++)
			{
				var device = new Device(string.Format("Device {0}", i));
				devices.Add(device);
				DeviceManager.Instance.AddDevice(device);
			}
			var thirdDevice = DeviceManager.Instance.GetDeviceById("Device 3");
			Assert.AreSame(devices.First(d => d.DeviceId == "Device 3"), thirdDevice);
		}

		[TestMethod]
		public void ListDevices()
		{
			var devices = DeviceManager.Instance.Devices;
			Assert.AreEqual(4, devices.Count());
		}

		[TestMethod]
		public void RemoveDeviceTest()
		{
			var devices = DeviceManager.Instance.Devices;
			var device = devices.ElementAt(2);
			Assert.IsTrue(DeviceManager.Instance.RemoveDevice(device));
			devices = DeviceManager.Instance.Devices;
			Assert.AreEqual(3, devices.Count());
			Assert.IsFalse(DeviceManager.Instance.RemoveDevice(device));
		}

	}
}

So what’s wrong with the code? The short answer is: a lot!

Fixing the Singleton instance… and removing it

Lets start by analyzing the Singleton instance. The first thing we need to consider when implementing a Singleton instance is whether we really need it, as it’s very likely we don’t. If we do, we should at least implement a double-check lock. See this MSDN page and this Stack Overflow question for more information. Please note that this implementation has to be done just right, so be careful when writing it. I’ve read the block of code below a couple of times, and I think it’s correct, but you never know.

Here’s what we ended up with. Don’t just copy/paste it!

private static volatile DeviceManager instance;
private static readonly object instanceLock = new object();
public static DeviceManager Instance
{
	get
	{
		if (instance == null)
		{
			lock (instanceLock)
			{
				if (instance == null)
				{
					instance = new DeviceManager();
				}
			}
		}

		return instance;
	}
}

In this case, we won’t be needing the Singleton instance so we will just remove this code altogether.

Depend on abstractions

The reason we don’t need the Singleton instance is twofold. First, it limits our ability to test any behavior that uses the DeviceManager (more on this later). Secondly, it forces us to depend on the concrete implementation of the DeviceManager, while what we want is to depend on abstractions. This way, we can promote the cohesiveness of our classes without having a tight coupling between them.

This is better stated in the book Agile Software Development – Principles, Patterns, and Practices by Robert “Uncle Bob” Martin. He calls this the Dependency-Inversion Principle:

A. High-level modules should not depend on low-level modules. Both should depend on abstractions.
B. Abstractions should not depend on details. Details should depend on abstractions.

In order to do this, we need to extract an interface from DeviceManager.

namespace PSampaio.Refactoring.Example1.New
{
	using System.Collections.Generic;
	using System.Linq;

	public interface IDeviceManager
	{
		Device ActiveDevice { get; set; }
		IEnumerable<Device> Devices { get; }
		void AddDevice(Device device);
		bool RemoveDevice(Device device);
		Device GetDeviceById(string id);
	}

	public class DeviceManager : IDeviceManager
	{
		private readonly Dictionary<string, Device> devices;

		public Device ActiveDevice { get; set; }

		public IEnumerable<Device> Devices
		{
			get { return devices.Values.AsEnumerable(); }
		}

		public DeviceManager()
		{
			devices = new Dictionary<string, Device>();
		}

		public void AddDevice(Device device)
		{
			if (!this.devices.ContainsKey(device.DeviceId))
			{
				this.devices.Add(device.DeviceId, device);
			}
		}

		public bool RemoveDevice(Device device)
		{
			var result = false;
			if (this.devices.ContainsKey(device.DeviceId))
			{
				result = this.devices.Remove(device.DeviceId);
			}
			return result;
		}

		public Device GetDeviceById(string id)
		{
			Device device;
			if (!devices.TryGetValue(id, out device))
			{
				device = null;
			}
			return device;
		}
	}
}

We also need to change the ActiveDeviceSupport class. Instead of having a concrete dependency on DeviceManager, we now depend on an abstraction, and that dependency is explicit. We could later evolve this design to use an IOC container to resolve the dependencies automatically.

namespace PSampaio.Refactoring.Example1.New
{
	using System.Linq;

	public class ActiveDeviceSupport
	{
		private readonly IDeviceManager deviceManager;

		public ActiveDeviceSupport(IDeviceManager deviceManager)
		{
			this.deviceManager = deviceManager;
		}

		public void SetFirstDeviceAsActive()
		{
			var devices = deviceManager.Devices;
			var firstDevice = devices.FirstOrDefault();
			if (firstDevice != null)
			{
				deviceManager.ActiveDevice = firstDevice;
			}
		}
	}
}

Correcting the tests

The tests we started with are a perfect example of how not to do unit testing, as they will only pass if they are run all together and in a specific order. While the first (CanAddDevice) test is isolated, the other two depend on the previous ones passing. Obviously, the tests will fail if we run each one separately. The reason for this is that they depend on the Singleton instance of the DeviceManager. So based on the changes we have been making, we can now write the tests differently.

namespace PSampaio.Refactoring.Example1.New
{
	using System.Linq;

	using Microsoft.VisualStudio.TestTools.UnitTesting;

	[TestClass]
	public class Tests
	{
		[TestMethod]
		public void CanAddDevice()
		{
			const string id = "Device 1";
			var deviceManager = new DeviceManager();
			var device = new Device(id);
			deviceManager.AddDevice(device);
			Assert.AreSame(device, deviceManager.GetDeviceById(id));
		}

		[TestMethod]
		public void CanListDevices()
		{
			const int deviceCount = 4;
			var deviceManager = new DeviceManager();
			for (var i = 0; i < deviceCount; i++)
			{
				var device = new Device(string.Format("Device {0}", i));
				deviceManager.AddDevice(device);
			}
			Assert.AreEqual(deviceCount, deviceManager.Devices.Count());
		}

		[TestMethod]
		public void CanRemoveDevice()
		{
			var deviceManager = new DeviceManager();
			var device = new Device("Device 1");
			deviceManager.AddDevice(device);
			Assert.AreEqual(1, deviceManager.Devices.Count());
			deviceManager.RemoveDevice(device);
			Assert.AreEqual(0, deviceManager.Devices.Count());
		}
	}
}

As you can see, each test is now isolated from the others, testing a specific code path in the DeviceManager. There are still changes that we could make, namely the for loop in the ListDevices test and the fact that we call IDeviceManager.AddDevice on the CanRemoveDevice test, but I’m OK with leaving it like that for now.

As a positive side-effect, if we need to test the ActiveDeviceSupport class, we don’t depend on DeviceManager anymore, so we can just mock the IDeviceManager interface and test ActiveDeviceSupport in isolation also.

Hope this helps!