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

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 2

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?