My little team is taking over development for one of our other products. Today is the first day of the typical "I'm getting the app up on my box and the NAnt build doesn't work on my box" hell. One of the things I'm seeing in trying to debug the broken unit tests is a bag of Singleton's that perform data access. Just to make it more fun, one singleton uses another singleton to get its configuration. My real problem is that the database connection string isn't correct yet, but the test failures exposed some larger structural issues to address later.
Look at the code below for a second. It's nothing special really, just a generic Singleton that serves as a gateway to some kind of resource, but this little bit of innocuous code can thoroughly hose the testability of any application.
public class Singleton
{
private static Singleton _instance;
// Lazy initialization because that's more *efficient*
public static Singleton GetInstance()
{
if (_instance == null)
{
lock (typeof(Singleton))
{
if (_instance == null)
{
_instance = new Singleton();
}
}
}
return _instance;
}
private object _something;
private Singleton()
{
_something = getSomethingFromDatabase();
}
private object getSomethingFromDatabase()
{
// go fetch some kind of configuration information from the database
return null;
}
}
So what's so wrong with this? Plenty. The instance of Singleton touches the database in its constructor function. Unit testing any class that depends on the Singleton class automatically includes some sort of live data access -- or does it? One of the problems with singleton's in unit testing is the lack of control over when the singleton was created. To the best of my knowledge, NUnit does not guarantee the order of unit test execution, so the singleton instance will be instantiated by whichever unit test happens to run first. Why is this such a bad thing? Because your tests aren't starting from a known state and therefore the results of the test are not reliable.
For another thing your unit test isn't a unit test, it's an integration test. Integration tests are cool too, but unit tests might be more important from the standpoint of creating working code. One of the qualifications and benefits of a unit test is that it pinpoints the exact trouble spot in the code when it fails. If a coarse-grained test fails, you've got to look in a lot more code to find the exact problem. If there's a database involved, then you're troubleshooting search widens outside of your code. It's possible to test against a live database if you can control the database state, but why do all that extra work if you don't need to? Solve one problem at a time and data access is often a separate issue. One obvious way to tell whether code is adequately unit tested at a fine-grained level is the amount of time spent with the debugger to fix problems. If you're using the debugger a lot, you really need to reconsider your unit testing approach.
Forget TDD for a second. Using a stateful singleton opens yourself up to all kinds of threading issues. Not many developers that write business software are going to be threading experts. When you screw up threading safety you can create really wacky bugs that are devilishly hard to reproduce. You do love bugs that can't be reproduced don't you? The code I'm looking at today seems to handle the thread safety issues quite well, but all the "lock(this, that, or the other)" code blows up the lines of code count. It adds complexity to the code and makes the code harder to understand and read. Add in a bunch of tracing code and you end up with a whole lot of noise code surrounding a little piece of code that actually does something useful. My biggest issue with the code is whether or not it was really necessary in the first place. One of the truisms in software development you bump into occasionally is that "Premature Optimization is the root of all evil" in software development. I'm guessing that the singletons were put in place due to performance concerns. Do it a simple way first and forget caching out of the gate. The performance bottlenecks are almost never where you think they'll be anyway. By doing things the simple way upfront you can leave yourself with more time later when the real performance problems become evident. Don't do anything that makes the code harder to understand unless you absolutely have to.
My best advice is to not use caching via static members until you absolutely have to for performance or resource limitation reasons. If you do need singleton-like functionality, you might try some of the techniques from this article. You can also get around the singleton issues by just being able to replace the single instance with a mock or stub from a static member. It works, but it's not my preference.