What’s wrong with that code?

Question

My college professor gave us this code and was asked what’s wrong:

public class Teste {
    private static Teste INSTANCE = null;

    public static Teste getInstance()
    {
        if ( INSTANCE == null )
        {
            INSTANCE = new Teste();
        }
        return INSTANCE;
    }

    private Teste() {
    }
}

I came to the conclusion that:

1. Test will never be created.
2. getInstance will return "Garbage".
3. As the builder is private he will never be called.
4. More than one test instance can be created

Would that be?

Answer 1

The problem with the code is that if there is more than one concurrent call on first access to the method getInstance() it can create two Test instances.

The basic solution for this is to synchronize the method:

public static synchronized Teste getInstance()
{
    if ( INSTANCE == null )
    {
        INSTANCE = new Teste();
    }
    return INSTANCE;
}

The problem with this approach is that all calls will be blocked, slowing the overall execution of the program. Imagine such a method on an application server with multiple users accessing the system? Terrible.

A better solution would be a block synchronized within the IF:

public static Teste getInstance()
{
    if ( INSTANCE == null ) {
      synchronized (Teste.class) {
        INSTANCE = new Teste();
      }
    }
    return INSTANCE;
}

This solves the problem of synchronization in all accesses, but it is a "naive" solution, because we actually returned to the initial problem. Like the IF is not synchronized, two threads can enter the IF at the same time and even with synchronization, they will return different instances when INSTANCE for null.

So the most "pure" solution for Singleton Pattern would be to add a double check, thus:

public static Teste getInstance()
{
    if ( INSTANCE == null ) {
      synchronized (Teste.class) {
        if ( INSTANCE == null ) {
          INSTANCE = new Teste();
        }
      }
    }
    return INSTANCE;
}

With this last approach, we guarantee that there will be no loss of performance because of unnecessary synchronization of the entire method.

In addition, we ensure that even if two competing calls to the method come within the first IF, then we have a new synchronized check that guarantees a single Test instance.

So in the worst case, if there are two or more competing calls on first access to getInstance (when INSTANCE is still null), only these first calls will be synchronized, and after the first assignment of INSTANCE, no subsequent call will be synchronized.

A full read on this you find on

Head First Design Patterns.

I recommend you research on Singleton

Note: the constructor private prevents any other class from inadvertently or arbitrarily creating an unwanted Test instance.