What’s wrong with this code? #1
.NET, Development, What's wrong with this code? July 9th, 2007
I decided to start a new column gathering all sorts of “what’s wrong with this code?” snippets. Why?
- Its fun.
- Its good for interview questions.
- It starts discussions. More interesting than just talking (or writing) to myself.
So, here’s what I plan:
- I post a code snippet (most probably C# but I’m not setting constraints on myself)
- You comment on what’s wrong with the code on the snippet
- 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.
Similar Posts:
- What’s wrong with this code? #1 - Discussion
- What’s wrong with this code? #2
- Getting the Full Name of the Current User
- [Code snippet] Obtaining Active Directory User Information Using System.DirectoryServices
- Apology to My Readers (Buggy dasBlog Contact Form)
[...] 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. [...]
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;
Yoav, you’re almost right…
Is there a reason this isn’t implemented as pre-loaded? That would remove the multi-threading problems.
What do you mean by “pre-loaded”?
public sealed class Singleton
{
private Singleton() { }
private static Singleton _insatnce = new Singleton();
public static Singleton Instance
{
get
{
return Singleton._insatnce;
}
}
}
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())
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…
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…
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?
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.
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.
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;
}
}