CODEWITHSUNDEEP
c#javadesign-patterns
user40587
user40587

Reputation: 11

Implementting Singleton Design Pattern [Please Suggest]

Can anyone identify the problem within this snippet of Java/C# code implementing Singleton Design Pattern.

Can someone find me the flaw in this implementation of this snippet?

class Singleton{
public static Singleton Instance() {
if (_instance == null)
_instance = new Singleton();
return _instance;
}
protected Singleton() {}
private static Singleton _instance = null;
}

Upvotes: 1

Views: 412

Answers (7)

Snicolas
Snicolas

Reputation: 38168

enum A { A; }

Upvotes: 1

Swagatika
Swagatika

Reputation: 3436

Its not thread-safe.

You can implement in a different way with pre-initialization:

private static Singleton _instance = new Singleton();

public static Singleton Instance()
{
    return _instance;
}

This method is thread-safe as you are only returning the instance in the method.

Upvotes: 0

Krishna Moorthy Seetha Raman
Krishna Moorthy Seetha Raman

Reputation: 293

All the above answers are correct.

In singleton design pattern you should not allow others to create instance for singleton class. You have to own your own rights. The other instance can request you for singleton's instance. So constructor should/must be private.

In your example you made it as protected, In this case if you extends the singleton class you have a possibility of creating an instance from other instance. This possibility should not be provided.

Make constructor as private in your code snippet, then its singleton class.

Upvotes: 0

nanda
nanda

Reputation: 24798

A better way to implement singleton (as Joshua Bloch's Effective Java) is to use enum:

enum Singleton {
    INSTANCE();
    private Singleton() {
       ....
    }
}

If you insist with your approach, you need to do three more things:

  • make the constructor private (as Jon Skeet's suggestion)
  • make _instance volatile
  • double lock the Instance()

Upvotes: 2

Jon Skeet
Jon Skeet

Reputation: 1503449

As Kieren says, it's not thread-safe... and also you've allowed classes to derive from it, which means it's not really a singleton:

public class SingletonBasher : Singleton
{
}

Singleton x = Singleton.Instance();
Singleton y = new SingletonBasher();

It should be invalid for x and y to be different references and both of them to be non-null - it violates the concept of a singleton.

(And yes, I'd recommend my article on singleton implementations too :)

Upvotes: 3

Petar Ivanov
Petar Ivanov

Reputation: 93090

If you are in a multithreaded environment two different threads might enter the if block and then both of them will create a new instance. Add locking:

class Singleton{
    public static Singleton Instance() {
        if (_instance == null) {
            lock(typeof(Singleton)) {
                if (_instance == null) {
                    _instance = new Singleton();
                }
            }
        }
        return _instance;
    }
    protected Singleton() {}
    private static Singleton _instance = null;
}

Upvotes: 0

Kieren Johnstone
Kieren Johnstone

Reputation: 42013

It's not thread-safe

http://csharpindepth.com/Articles/General/Singleton.aspx

Upvotes: 9

Related Questions