What’s wrong with this code? #1

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

You may also like...

13 Responses

  1. Yoav Zobel says:

    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;

  2. ekampf says:

    Yoav, you’re almost right…

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

  4. ekampf says:

    What do you mean by “pre-loaded”?

  5. public sealed class Singleton
    {
    private Singleton() { }

    private static Singleton _insatnce = new Singleton();

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

  6. simon says:

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

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

  8. ekampf says:

    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…

  9. Matt says:

    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? :)

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

  11. Adi says:

    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.

  12. Phil H says:

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

  1. August 11, 2008

    […] 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. […]