current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 2 down vote favorite
1

I have several utility classes who should only ever have one instance in memory, such as LogHelper, CacheHelper, etc. One instance to rule them all.

I've never implemented Singleton in Java before, so I wanted to post my first pass and see if anybody had major issues with this, or just helpful critiques. I want to make sure that my design will have the intended affect when I go to use these singleton classes throughout my codebase.

Singleton:

public class Singleton
{
    private static Singleton instance;

    protected Singleton()
    {
        super();
    }

    public static synchronized Singleton getInstance()
    {
        if(instance == null)
            instance = instantiate();

        return instance;
    }

    // Wanted to make this abstract and let subclasses @Override it,
    // but combining 'abstract' and 'static' is illegal; makes sense!
    protected static Singleton instantiate()
    {
        return new Singleton();
    }
}

LogHelper (a singleton since all my classes will use it to write messages to the same destinations):

public class LogHelper extends Singleton
{
    protected static LogHelper instantiate()
    {
        return new LogHelper(...blah...);
    }

    public LogHelper(...blah...)
    {
        // Initialize the singleton LogHelper
    }

    // More methods for logging, etc.
}

Here's how I'd like to use it throughout my codebase:

LogHelper logHelper = LogHelper.getInstance();
logHelper.logInfo("...");

What I'm worried about is that I may have inadvertently designed this wrong and heading down the wrong avenue. Can anybody weigh-in on my design?!?

Thanks!

share|improve this question

migrated from stackoverflow.com Aug 6 '11 at 18:30

This question came from our site for professional and enthusiast programmers.
    
Make the constructor private. –  Sagar V Aug 6 '11 at 2:50
    
In most cases Singletons are simply a bad idea. They are hard to test, and you are in trouble if you need multiple instances later. The factory pattern is better, but clearly the best solution is Dependency Injection (check out Guice or Spring). –  Landei Aug 9 '11 at 7:21
    
What's wrong with using existing logging frameworks?? See: slf4j.org, logging.apache.org/log4j/1.2, and logback.qos.ch –  Gevorg Aug 22 '11 at 2:08

5 Answers 5

up vote 4 down vote accepted

Unfortunately, there is no way (that I know of) to prevent a subclass from exposing a public constructor - as a result, creating a Singleton base-class may not be in your best interest. If your goal is to enforce Singleton-ness, it is probably best to just implement something like the following on a class-by-class basis:

public class SomeSingletonClass {
  public static final SomeSingletonClass INSTANCE = new SomeSingletonClass();

  private SomeSingletonClass(){
  }

  // ...
}

Many IDEs (such as IntelliJ) will create the boiler-plate Singleton code for you with the click of a button.

If you prefer to access the singleton via a method, you can always make INSTANCE private and add a public static getInstance() method.

I also noticed you have a lazy/synchronized initializer for your example - this is probably not necessary in most cases.

share|improve this answer
    
Perhaps you can say how this is better than using an enum with one instance. ;) –  Peter Lawrey Aug 6 '11 at 12:37
    
Why have you accepted your own answer when there is a better alternative. –  Loki Astari Aug 7 '11 at 20:22
    
Bohemian's answer is the way to go if you want lazy loading. –  nerdytenor Aug 23 '11 at 22:46
up vote 7 down vote

The best pattern for singleton that is guaranteed to be thread safe and minimizes lock overhead (because it has none) is this:

public class Singleton {
    private Singleton() {}

    private static class InstanceHolder {
        static Singleton INSTANCE = new Singleton();
    }

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

This is lazy initialized but carries no locking overhead. It is guaranteed safe by the Java Memory Model: The inner static class is not loaded until the first call to getIinstance() and all static initializtion is guaranteed to be completed before the class is used, therefore INSTANCE is fully constructed when first accessed.

Note: The double checked locking pattern is BROKEN - see this article that explains exactly why.

share|improve this answer
    
I knew it was broken in C++ (unless you know the compiler details (which means its generally broken)), interesting to note it is also broken in Java. Thanks for the reference. –  Loki Astari Aug 7 '11 at 20:25
    
Hmmm, starting from Java 5 you can use the double checked locking by making sure to declare the holder as volatile. See my answer on stackoverflow.com/questions/6445310/… –  Gevorg Aug 22 '11 at 2:05
up vote 7 down vote

The simplest Singleton since Java 5.0 (2004) is to use an enum.

public enum SomeSingletonClass {
    INSTANCE;
}

Classes are loaded lazily and if the only purpose of the class is to be a singleton, you don't need additional lazy loading.

However in your cause you only need a Utility class.

enum Log {;
    public static void info(String text) { }
}

Log.info("...");
share|improve this answer
    
@Dale Wijnand, Thank you. –  Peter Lawrey Sep 24 '11 at 6:32
up vote 1 down vote

The "original" Singleton design pattern relied on a class not exposing public constructors so that it could control instantiations of itself, by encapsulating all usages of new within the class:

public class SomeSingletonClass
{
    private static SomeSingletonClass instance;

    //disallow other classes from instantiating new instances
    private SingletonClass()
    {
    }

    public synchronized SomeSingletonClass getInstance()
    {
        if(instance == null)
            instance = new SomeSingletonClass(); // instantiate here
        return instance;
    }
}

By subclassing the singleton, you would allow subclasses to create multiple instances of the singleton, there by losing the singleton property for the parent. So, the following line will likely result in multiple instances of the Singleton being created in your case:

public LogHelper(...blah...)
{
    // Initialize the singleton LogHelper
}

But it is much worse than you can predict. If a client of LogHelper invokes the following:

LogHelper logHelper = new LogHelper(...blah...);
LogHelper anotherHelper = new LogHelper(...blah...);

you will have two LogHelper instances no matter what - the culprit is the public visibility of the LogHelper constructor. So your subclass is not a singleton at all.

If you want to implement the singleton pattern across multiple classes, it is better to not treat it as a property than can be inherited. Rather, you must recreate the behavior in every individual class.

share|improve this answer
up vote 1 down vote

i've always found this singleton code snippet robust . the synchronize on 'LOCK' object make the singleton creation thread safe 

public class Singleton{

    private static final Object LOCK = new Object();
    private static Singleton singleton;

    private Singleton(){}
/**
     * Returns the singleton instance of <CODE>Singleton</CODE>, creating it if
     * necessary.
     * <p/>
     *
     * @return the singleton instance of <Code>Singloton</CODE>
     */
    public static Singleton getInstance() {

        // Synchronize on LOCK to ensure that we don't end up creating
        // two singletons.
        synchronized (LOCK) {
            if (null == singleton) {


                singleton = new Singleton();


            }
        }

        return singleton;
    }
}
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.