“Singleton” factories, ok or bad?

Question

I've a lot of (abstract) factories and they're usually implemented as singletons.

Usually for the convenience of not having to pass them through layers who really have no business with using or knowing these factories.

Most of the times I only need to make a decision at startup of which factory implementation the rest of the code program, maybe through some configuration

it looks e.g. like

abstract class ColumnCalculationFactory { 
  private static ColumnCalculationFactory factory;

 public static void SetFactory(ColumnCalculationFactory f) {
         factory = f;
  }

  public static void Factory() {
         return factory;
  }

 public IPercentCalculation CreatePercentCalculation();
 public IAverageCalculation CreateAverageCalculation();
    ....

}

Someting do smell about this, I'm just not sure what - it's maybe more a disuised global than a singleton. It's not like there really really have to be only one factory ever creating ColumnCalculations - although my programs don't need more.

Is this considered best practuce ? Should I rather stuff these in some (semi) global AppContext class ? Something else(I'm not quite ready to switch to some larger IoC container, or spring.net quite yet btw) ?

Answer 1

No, because what you're doing here is creating global state. There are all sorts of problems with global state - chief among them that one function then depends in rather invisible way on the behavior of other functions. If a function calls another function that forgets to store and restore the factory before it finishes, then you have a problem because you can't even get the old value back unless you stored it somewhere. And you have to add code to do that (judging by your code, I'd guess you're in a language with finally, which leaves even more room for mistakes). What's more, if you end up with code that needs to switch between two factories quickly for two subobjects, you have to write a method call at each point - you can't store the state in each subobject (well, you can, but then you defeat the purpose of global state [which admittedly isn't much]).

It probably makes the most sense to store the factory type as a member, and pass it to the constructor of new objects that need it (or create a new one as needed, etc.). It also gives you better control - you can guarantee that all objects constructed by object A went through the same factory, or you can offer methods to swap factories out.

Answer 2

Having a few singletons is pretty typical and not usually problematic--a lot of singletons leads to some irritating code.

We just went through a situation where we had to test our heavily singleton-laden classes. The problem is that when you are testing class b, and it gets class c (a singleton), you have no way to mock out class c (At least EasyMock wouldn't allow us to replace the static factory method of a singleton class.

One simple fix is to have "Setters" for all your singletons for testing purposes. Not really recommended.

One other thing we tried was to have a single class that held all the singletons--a registry. Doing this is getting pretty close to dependency injection which is what you should almost certainly be using.

Aside from testing, I learned a long time ago that when there is never ever ever going to be more than one instance of a given object; in the next rev they often want two, which makes singletons MUCH better than static classes--at least you can add a parameter to the getter of a singleton and return a second one without too much refactoring (which is again just doing what DI does).

At any rate, look into DI, you might be really happy.

Answer 3

I would advise against it, because it makes it very hard to write decent unit tests for the code that calls these factories.

Misko Hevery has a nice article about this on his blog.

Answer 4

The bad form of singleton is the one that implements the equivalent of your Factory method with:

return new CalculationFactory ();

That's much harder to stub, mock or wrap.

The version that returns an object created elsewhere is much better, although like most anything, can be misused or overused.

Answer 5

It really depends on what you're doing and the scope of your application. If it's just a fairly small app and it's never going to grow beyond this, then your current approach may well be fine. There is no universal "best" practice for these things. While I wouldn't recommend using singletons for anything other than stateless leaf methods and/or one-way calls (e.g. logging), dismissing it out of hand 'just because' it's a singleton isn't necessarily the right thing to do.

For anything other than trivial or prototype code, I personally like to explicitly use inversion of control with constructor injection, as it means all dependencies are accounted for and you don't get any 'surprises'. The compiler won't let you instantiate A without B and B without C. Singletons immediately bury these relationships -- you can instantiate A without B and B without C. When the call from A to B happens, you'll get a null reference exception.

This is especially annoying when testing, as you have to iteratively work backwards via runtime failures. When you test code, you're using the API just as a fellow coder would do, so it is indicative of the design problems with this approach. Constructor injection ensures this can never happen -- all of the dependencies are stated up front. The downside with constructor injection is that the configuration of your object graph is more complicated. This is mitigated through the use of an IoC container.

I guess what I'm trying to say is that if you've got to the point where you're considering using some kind of context object and a registry pattern, you may as well have a look at IoC containers. Going to the effort of rolling your own mutt version is probably a waste of time when you can use an established, free product like Autofac.

Answer 6

Is this considered best practice?

I see no problem: you said "my programs don't need more", therefore your implementing anything more flexible/abstract might be a case of You Ain't Gonna Need It.