views:

338

answers:

5

Here is my sample abstract singleton class:

public abstract class A {
    protected static A instance;
    public static A getInstance() {
        return instance;
    }
    //...rest of my abstract methods...
}

And here is the concrete implementation:

public class B extends A {
    private B() { }
    static {
        instance = new B();
    }
    //...implementations of my abstract methods...
}

Unfortunately I can't get the static code in class B to execute, so the instance variable never gets set. I have tried this:

Class c = B.class;
A.getInstance() - returns null;

and this

ClassLoader.getSystemClassLoader().loadClass("B");
A.getInstance() - return null;

Running both these in the eclipse debugger the static code never gets executed. The only way I could find to get the static code executed is to change the accessibility on B's constructor to public, and to call it.

I'm using sun-java6-jre on Ubuntu 32bit to run these tests.

+4  A: 

Abstract Singleton? Doesn't sound viable to me. The Singleton pattern requires a private constructor and this already makes subclassing impossible. You'll need to rethink your design. The Abstract Factory pattern may be more suitable for the particular purpose.

BalusC
The purpose of the private constructor in the Singleton is to prevent anyone else instantiating it. I have achieved this here - you can't instantiate an abstract class, and the subclass has a private constructor.
Simon
It doesn't force the subclass to be abstract-only and have a private constructor only.
BalusC
+2  A: 

A.getInstance() will never call a derived instance since it's statically bound.

I would separate the creation of the object from the actual object itself and create an appropriate factory returning a particular class type. It's not clear how you'd parameterise that, given your example code - is it parameterised via some argument, or is the class selection static ?

You may want to rethink the singleton, btw. It's a common antipattern and makes testing (in particular) a pain, since classes under test will provide their own instance of that class as a singleton. You can't provide a dummy implementation nor (easily) create a new instance for each test.

Brian Agnew
A.getInstance() does not need to call a derived instance, since it returns the instance variable. The derived instance (class B) sets the instance variable. Except that the static code block does not get run.If I can't fix this then a Factory pattern may be my only option.
Simon
+3  A: 

Singletons are kind of yucky. Abstract insists on inheritance which you more often than not want to avoid if possible. Overall I'd rethink if what you are trying to do is the simplest possible way, and if so, then be sure to use a factory and not a singleton (singletons are notoriously hard to substitute in unit tests whereas factories can be told to substitute test instances easily).

Once you start looking into implementing it as a factory the abstract thing will sort itself out (either it will clearly be necessary or it may factor out easily in place of an interface).

Bill K
+1 for pointing out that singletons are an anti-pattern.
Michael Aaron Safyan
+1: Singletons suck and their use should be avoided.
DeadMG
+2  A: 

In addition to problems others have pointed out, having the instance field in A means that you can only have one singleton in the entire VM. If you also have:

public class C extends A {
    private C() { }
    static {
        instance = new C();
    }
    //...implementations of my abstract methods...
}

... then whichever of B or C gets loaded last will win, and the other's singleton instance will be lost.

This is just a bad way to do things.

Matt McHenry
Thanks - and the others who said the same thing. This highlights that my real problem is how I select which one of several implementations of my interface I should expose. Factory or Facade look like good ways to go.
Simon
A: 

Well from your post, even if it's not clearly stated, sounds like you want the abstract class to play two different roles. The roles are :

  • the abstract factory role for a (singleton) service that can have multiple substitutable implementations,
  • the service interface role,

plus you want the service to be singleton enforce 'singletoness' in the entire family of classes, for some reason it's not enough for you to cache the service instance.

This is fine.

Somebody will say it smells very bad because "violates separation of concerns" and "singletons and unit testing don't go well together".

Someone else will say it's ok-ish because you give put responsibility of instantiating the right children in the family itself and also expose more fluent interface overall since you don't need mediation of a factory that does nothing else than exposing a static method.

What is wrong is that after you want the children to be responsible of selecting what implementation should the parent factory method return. It's wrong in terms of design because you are delegating to all children what can be simply pushed up and centralized into the abstract superclass and also it shows you are mixing together patterns that are used in different contexts, Abstract Factory (parent decide what family of classes clients are going to get) and Factory Method (children factories select what the clients will get).

Factory Method is not just not required but also not possible with factory methods since it is centered on implementing or overriding "instance" methods. There is no such thing as override for a static methods, nor for a constructor.

So going back to the initial good or bad idea of an abstract singleton that selects which behaviour to expose there are several ways to solve the initial problem, One could be the following, looks bad but i guess is near to what you were looking for:

public abstract class A{
    public static A getInstance(){
      if (...)
         return B.getInstance();
      return C.getInstance();
    }

    public abstract void doSomething();

    public abstract void doSomethingElse();

}

public class B extends A{
    private static B instance=new B();

    private B(){
    }

    public static B getInstance(){
        return instance;
    }

    public void doSomething(){
        ...
    }
    ...
}

//do similarly for class C

The parent could also use reflection.

More test friendly and extension friendly solution is simply to have children that are not singleton but packaged into some internal package that you will document as "private" and abstract parent that can expose the "singleton mimiking" static getInstance() and will cache the children instances enforcing that clients always get the same service instance.

Benny F