Which is more evil: an unnecessary singleton or a God Object?

Question

Here's the situation: I've got a class that is doing too much. It's mainly for accessing configuration information, but it also has the database connection. It's implemented as a singleton, so this also makes unit testing it difficult as most code is pretty tightly coupled to it. This is even more problematic as it creates an import-time dependency (we're doing this in Python), which means that certain modules must be imported in a certain order. Ideally, I'd like to both split this into two classes and make it a non-singleton.

Fortunately, my employer has warmed up to the fact that this kind of testing is good and is willing to permit me to make changes like this if it makes code more testable. However, I doubt that they will be willing to allow me to spend too much time on it. And I'd rather fix this incrementally rather than trying to be too radical.

So, I see three choices here:

1. Break the configuration object into a (singleton) configuration object and a (non-singleton) database object. This would at least allow me to remove the database as an import-time dependency.
2. Make the configuration object a non-singleton and pass it to objects that need it. I feel that this better addresses our short-term needs, but I think it would take significantly more time.
3. Do something I hadn't thought of that you suggest in your answer. :-)

So what do I do?

Answer 1

It's hard to know without seeing your code, but why not do what you say - do it incrementally? First do step 1, split out the database.

If that is quick then go back, and you now have only 1 smaller object to stop being a singleton rather than 2. So step 2 should be quicker. Or at this stage you might see some other code that can be refactored out of the singleton.

Hopefully you can step-by-step reduce what is in the singleton until it vanishes, without ever paying a huge time tax at any one step.

For example could perhaps one part of the configuration be made singleton at a time, if the parts of the configuration are independent. So maybe GUI configuration left singleton while fileconfiguration refactored, or something similar?

Answer 2

Option 1 is what I do in all my apps: configuration object singleton and database objects created on demand or injected.

Having the cofiguration object as a singleton has always been a perfect fit for for me. There is only one configuration file and I always want to read it when the application starts.

Answer 3

I think you are on track to separate into two classes. You might want to consider using a factory to create your database context/connection as needed. This way you can treat the connection as a unit of work entity that gets created/disposed as needed rather than keeping a single connection around for the life of the object. YMMV, though.

As for the configuration, this is one case where I find that a singleton can be the right choice. I wouldn't necessarily dump it just because it is hard to unit test. You might want to consider building it, though, to implement an interface. You can then use dependency injection to provide a mock instance of the interface during testing. Your production code would be built either to use the singleton instance if the injected value is null or to inject the singleton instance. Alternatively, you could construct the class to allow re-initialization via a private method and invoke this in your setup/teardown test methods to ensure that it has the proper configuration for your tests. I prefer the former to the latter implementation, though I've used it as well when I don't have direct control over the interface.

Making the changes incrementally is definitely the way to go. Wrapping the current functionality with tests, if possible, and making sure that those tests still pass after your modifications (though, not the ones dealing directly with your modifications, of course) is a good way to make sure you're not breaking other code.

Answer 4

Excuse the fact that I don't know any Python, so I hope any pseudo code makes sense...

I'd split the object into two parts first so that the responsibilities of your singleton are smaller, then I'd take the remaining configuration singleton and change it into a normal class (as though you are going to perform your second suggestion).

Once I'd got that far I'd create a new, wrapper singleton that exposes the methods of the configuration class:

class ConfigurationWrapper : IConfigurationClass 
{
    public static property ConfigurationWrapper Instance;

    public property IConfigurationClass InnerClass;

    public method GetDefaultWindowWidth()
    {
        return InnerClass.GetDefaultWindowWidth();
    }

    etc...
}

Now the very first thing the application does is inject an instance of the ConfigurationClass into the wrapper.

ConfigurationClass config = new ConfigurationClass()
ConfigurationWrapper.Instance.InnerClass = config;

Finally you can move classes that currently rely on the singleton over to wrapper singleton (which should be a quick find and replace). Now you can convert the classes one at a time to take the config object in through their constructors to complete stage 2. Any that you don't have time to do can use the wrapper singleton. Once you have moved them all over you can get rid of the wrapper.

On the other hand you can ignore the refactoring of the usages and simply inject a mocked configuration class into the singleton wrapper for testing - essentially a poor man's dependency injection.