Singleton Pattern with Public Constructor

Question

public class MySingletonClass
{
  public MySingletonClass()
  {
    _mySingletonObj = Instance();
  }

  public static MySingletonClass Instance()
  {
    if (_mySingletonObj  == null)
    {
      lock (typeof(lockObject))
      {
        if (_mySingletonObj  == null)
          _mySingletonObj  = new MySingletonClass();
      }
    }
    return _mySingletonObj ;
  }
}

MySingletonClass _myObj = new MySingletonClass();

This act as singleton with public constructors..?

Thanks

Answer 1

No, it's not a singleton - anyone can create multiple instances of it. (Leaving aside the stack overflow issue that's already been raised, and the fact that you're using double-checked locking unsafely.)

One of the distinguishing features of a singleton type is that it prevents more than one instance of itself from ever being constructed.

From the wikipedia Singleton Pattern article:

In software engineering, the singleton pattern is a design pattern that is used to restrict instantiation of a class to one object.

From Ward Cunningham's pattern repository:

A Singleton is the combination of two essential properties:

• Ensure a class only has one instance
• Provide a global point of access to it

Clearly your singleton fails to meet both these definitions.

See my singleton article for real implementations.

Answer 2

The constructor should be private

Answer 3

The code as posted does not work as a singleton, due to the public constructor, but in addition to that, there's numerous flaws and problems with the code:

• Public constructor calls Instance, which calls public constructor, which calls Instance, which calls..... stack overflow imminent
• Public constructor returns a new object every time it is called, you can not replace the returned result with the same object reference upon subsequent requests. In other words, public constructor on singleton breaks the pattern.
• You should not leak your lock objects, which in your case you lock on the type of an object. Do not do that, instead lock on a private object.

Here's a fixed version of your code:

public class MySingletonClass
{
  private readonly object _mySingletonLock = new object();
  private volatile MySingletonClass _mySingletonObj;

  private MySingletonClass()
  {
    // normal initialization, do not call Instance()
  }

  public static MySingletonClass Instance()
  {
    if (_mySingletonObj == null)
    {
      lock (_mySingletonLock)
      {
        if (_mySingletonObj  == null)
          _mySingletonObj = new MySingletonClass();
      }
    }
    return _mySingletonObj;
  }
}

MySingletonClass _myObj = MySingletonClass.Instance();

Answer 4

Instead of locking it, you could also try to create a static readonly singelton...

public sealed class Singleton
{
    private static readonly Singleton instance = new Singleton();

    static Singleton()
    {
    }

    private Singleton()
    {
    }

    /// <summary>
    /// The public Instance property to use
    /// </summary>
    public static Singleton Instance
    {
        get { return instance; }
    }
}

Answer 5

Check out the .NET Optimized code section at the Dofactory. This has IMO the best implementation. Also check out the site for other design pattern implementations in C#.