I decided to start a new column gathering all sorts of “what’s wrong with this code?” snippets. Why?

  1. Its fun.
  2. Its good for interview questions.
  3. It starts discussions. More interesting than just talking (or writing) to myself.

So, here’s what I plan:

  1. I post a code snippet (most probably C# but I’m not setting constraints on myself)
  2. You comment on what’s wrong with the code on the snippet
  3. I post my answer, interesting comments, etc. (you can comment on my answer too :-))

* Note that these posts are syndicated on blogs.microsoft.co.il (and maybe some other places) so please leave your comments at the main blog – www.ekampf.com/blog/ ** I know there are many “What’s wrong with this code?” out there already. I’ll try not to recycle (too much ;)) When looking at these snippets note the following “Bad Code” classifications: Confusing code, Overly complex code, Performance Issues, Buggy. So, to start with an easy one (I think), check out the following Singleton implementation:

public sealed class Singleton
{
    private Singleton() { } 

    private static Singleton _insatnce;
    private static object _syncRoot = new Object();

    public static Singleton Instance
    {
        get
        {
            lock (_syncRoot)
            {
                if (Singleton._insatnce == null)
                {
                    Singleton._insatnce = new Singleton();
                }
                return Singleton._insatnce;
            }
        }
    }
}

Update July 16th, 2007: I’ve posted the answer and a summary on the comments at the following post.

kick it on DotNetKicks.com

Similar Posts:

    None Found

Tags: , ,


13 Comments to “What’s wrong with this code? #1”

  1. DeveloperZen » Blog Archive » What’s wrong with this code? #1 - Discussion | August 11th, 2008 at 2:33 am

    [...] Singleton implementation in the snippet I posted works fine as a lazy, thread-safe Singleton as it ensures only one thread can create the instance. [...]

  2. Yoav Zobel | August 11th, 2008 at 2:49 am

    Netz,
    First of all: _insatnce –> _instance :-)
    (I actually prefer s_Instance for static fields)
    Second: No need to lock the entire method. If you want to use locking, better to move the lock inside the if, and double check for null:
    if …
    {
    lock …
    {
    if …
    {
    new …
    }
    }
    }
    Third – This will not work without:
    return _instance;

  3. ekampf | August 11th, 2008 at 2:51 am

    Yoav, you’re almost right…

  4. Omer van Kloeten | August 11th, 2008 at 2:53 am

    Is there a reason this isn’t implemented as pre-loaded? That would remove the multi-threading problems.

  5. ekampf | August 11th, 2008 at 2:54 am

    What do you mean by “pre-loaded”?

  6. Omer van Kloeten | August 11th, 2008 at 2:54 am

    public sealed class Singleton
    {
    private Singleton() { }

    private static Singleton _insatnce = new Singleton();

    public static Singleton Instance
    {
    get
    {
    return Singleton._insatnce;
    }
    }
    }

  7. simon | August 11th, 2008 at 2:55 am

    When something is static, you should lock on it’s type, not on the object, so instead of

    lock (_syncRoot)

    you should write:
    lock (_syncRoot.getType())

  8. Omer van Kloeten | August 11th, 2008 at 2:56 am

    Simon,

    That’s a bad, bad thing to do. Now you would be deadlocked with anything else that decides to lock onto the type System.Object, even if it’s not even in your code…

  9. ekampf | August 11th, 2008 at 2:56 am

    Simon,
    I don’t know where you got this direction that static types should be locked according to their type. In fact, I can imagine anything wose you can be doing.
    SyncRoot are usually static objects. If what you’re saying is true then all the developers writing multi-threading code in your product will lock on their _syncRoot.GetType() which is basically the same as locking on typeof(Object).
    This means everyone lock on the exact same object – typeof(object) – which will pretty much deadlock you’re application right away…

  10. Matt | August 11th, 2008 at 2:59 am

    I agree with Omer, just instantiate the Singleton object when the class is loaded.

    Are there any implications in calling the return from within the lock?

    Correct the spelling on “_instance”.

    And on top of it all, this singleton is worthless, it doesn’t have any methods so why would anyone access it? :)

  11. Brennan Stehling | August 11th, 2008 at 3:01 am

    I am going to reach a bit, but if the comment by Matt suggests that this singleton is useless because it has no methods than maybe this is meant to be a base class for other singletons. The problem then is the fact that it is sealed which is not nice to a developer using this class. It should instead be marked as virtual if the intension is for it to be inherited.

    Otherwise I agree with Omer. You can avoid the locking code if you instantiate the static value in the declaration, although I do not often see that in documented code. Perhaps there is a reason the value is defined in the property accessor.

    Also, you could clean this up a little by calling a private method to create the singleton instance with the accessor. It would look like…

    // requires using System.Runtime.CompilerServices
    [MethodImpl(MethodImplOptions.Synchronized)]
    private void InstantiateSingleton()
    {
    if (_singleton == null)
    {
    _singleton = new Singleton();
    }
    }

    The attribute handles the locking for you and then the property access is much simpler.

    public static Singleton Instance
    {
    get
    {
    if (_instance == null)
    {
    InstantiateSingleton();
    }
    return _instance;
    }
    }

    The code is then also self-documenting with the descriptive method name.

  12. Adi | August 11th, 2008 at 3:01 am

    Here’s the shortest implementation of a singleton:

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

    private Singleton(){}
    }

    Ofcourse, you’d have to adapt it to your own needs as it’s useless in this form.
    In its current implementation, the CLR is taking care not to instantiate a static more than once, so you save yourself of providing your own locking logic.

    Brennan, if I remember correctly, [MethodImpl(MethodImplOptions.Synchronized)] translates to lock(this) which leads to deadlocks.

  13. Phil H | August 11th, 2008 at 3:02 am

    I agree with those who suggested instantiating in the static field declaration.

    However, if you wanted to use a property instead I would start with making _instance volatile and use a double-check something like:

    public static Singleton Instance
    {
    get
    {
    if (Singleton._instance == null)
    {
    lock (_syncRoot)
    {
    if (Singleton._instance == null)
    Singleton._instance = new Singleton();
    }
    }
    return Singleton._instance;
    }
    }