0

I am reviewing following code where I am confused with blank constructor for the FlowSpaceImpl class. Since constructor is private and FlowSpaceImpl instance is set to be static and private its obvious developer wants only one instance for this class. But When new FlowSpaceImpl() is call how the object for FlowSpaceImpl class will be initialized at first place. For code review you can look at FlowSpaceImpl implementation

 public class FlowSpaceImpl implements FlowSpace {

     private static FlowSpaceImpl instance =  null;

        private FlowSpaceImpl() {}

            private static FlowSpaceImpl getInstance() {
            if (instance == null)
            instance = new FlowSpaceImpl();
            return instance;
            }



}
jruizaranguren
  • 12,679
  • 7
  • 55
  • 73
Ali Ahmad
  • 1,055
  • 5
  • 27
  • 47
  • 2
    `getInstance()` should be `public`, or else it doesn't make sense. – jlordo Jun 02 '13 at 14:00
  • 2
    A blank constructor means that when initializing the object there's nothing to do. Note that is marked as `private` in order that no other class could create instances of this class. – Luiggi Mendoza Jun 02 '13 at 14:01
  • By the way, this example of singleton shows what it can just be a helper class, no need of a singleton. – Luiggi Mendoza Jun 02 '13 at 14:06
  • @LuiggiMendoza: except that it might be one of several implementations of FlowSpace, and thus be passed as argument to methods expecting a FlowSpace (if what you meant by helper class is a class with only static methods). – JB Nizet Jun 02 '13 at 14:08
  • @JBNizet agreed. But having an implementation like this just shows that something is wrong with the design to begin with. – Luiggi Mendoza Jun 02 '13 at 14:10

1 Answers1

2

Your instance variable is static, the instance constructor has no use. This is the code that initialises the variable

private static FlowSpaceImpl getInstance() {
    if (instance == null) {
        instance = new FlowSpaceImpl();
    }
    return instance;
}

So when getInstance is called, if it's null, it is initialised before being returned.

Note, this code is not threadsafe and as such is very, very bad.

The generally accepted way of writing a threadsafe singleton is :

public class FlowSpaceImpl implements FlowSpace {

    public static FlowSpaceImpl getInstance() {
        return InstanceHolder.INSTANCE;
    }

    private static class InstanceHolder {

        private static final FlowSpaceImpl INSTANCE = new FlowSpaceImpl();
    }

    private FlowSpaceImpl() {
    }
}

This code leverages atomicity guarantees in the Java language specification to ensure thread safety. More information here.

Boris the Spider
  • 59,842
  • 6
  • 106
  • 166
  • It's only very very bad if it's supposed to be used from multiple threads. It's not necessary the case. – JB Nizet Jun 02 '13 at 14:05
  • 1
    @JBNizet agreed. But I think singletons are commonly accepted as bad news. Ones the break when you happen to have multiple threads are even worse news. Better to at least make it thread safe. – Boris the Spider Jun 02 '13 at 14:07
  • 4
    I agree that a singleton is generally an anti-pattern, especially these days where DI frameworks are so common. – JB Nizet Jun 02 '13 at 14:10
  • @JBNizet However, in Java SE, there are many [examples](http://stackoverflow.com/a/2707195/870248) of the use of the Singleton pattern. – Paul Vargas Jun 02 '13 at 14:17
  • +0 The generally accepted way to create a singleton it to use an `enum` with on instance, it's a lot simpler ;) – Peter Lawrey Jun 02 '13 at 15:03