Reputation: 5887

Singleton Pattern with Public Constructor

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

Upvotes: 6

Answers (6)

ZuTa

Reputation: 1444

The essence of Singleton is to provide :

exactly one instance of class across the system;
simple access to it.

The implementation of Singleton based on creation a class with a method(or property in .NET) that creates an instance of this class if one doesn't exists yet. The constructor of class must be private to prevent other ways of initialization. Also Singleton must be carefully used in multi-threaded applications because in one point of time, the two threads may create two different instances (which violates singleton pattern).

More details and examples you can find here.

Upvotes: 1

Lasse V. Karlsen

Reputation: 391336

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();

Upvotes: 4

SwDevMan81

Reputation: 49978

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#.

Upvotes: 3

Heiko Hatzfeld

Reputation: 3197

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; }
    }
}

Upvotes: 1

Jon Skeet

Reputation: 1500515

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: