up vote 1 down vote favorite

I need to generate consecutive sentence of int values: 0, 1, 2, ... But I need to access them from different threads. So I wrote such code:

class Counter
{
    private static Counter instance = new Counter();
    private static int i = 0;

    public static Counter Instance
    {
        get
        {
            return instance;
        }
    }

    public int Next()
    {
        lock (this)
        {
            return i++;
        }
    }

    public void Reset()
    {
        lock (this)
        {
            i = 0;
        }
    }

}

May this be implemented simpler?

link|improve this question
Your code looks quite simple, readable and obvious to me (ignoring the unnecessary singleton “noise”). Why do you want to do it even simpler? – svick May 6 at 17:12
@svick also I expect that probably lock can be avoided somehow, probably c# has built-in synchronized int or something? – javapowered May 7 at 5:05
@javapowered, yes, C# has something like that, but it's quite hard to get right. So, unless this code is a performance bottleneck for you, you should probably stick with lock. – svick May 7 at 9:01
feedback

3 Answers

up vote 2 down vote 
private static int i = 0;

Are you sure that i should be static? If it's static there isn't too much sense of the Counter instance = new Counter() instance and the Next and the Reset methods also could be static.

Anyway, I'd use a longer variable name, like nextValue for better readability.

Furthermore, consider the drawbacks of the Singleton pattern:

This pattern makes unit testing far more difficult, as it introduces global state into an application.

More on Wikipedia.

link|improve this answer
feedback
up vote 2 down vote

I think Interlocked.Increment is what you're looking for.

link|improve this answer
@Leonid: To be honest, I wouldn't know what to expand; The MSDN link explains everything :) The biggest design mistake using Interlocked.Increment in the requested scenario, is to do something like Interlocked.Increment(ref someVariable); return someVariable; – Willem van Rumpt May 7 at 19:30
feedback
up vote 0 down vote

Never lock on this as your callers can also do the same and cause deadlocks where you don't expect them. See below where I create and use a private locking variable. And made i an instance variable since you use a singleton instance anyhow.

class Counter
{
    private static readonly Counter instance = new Counter();

    private static readonly object locker = new object();

    private int i;

    public static Counter Instance
    {
        get
        {
            return instance;
        }
    }

    public int Next()
    {
        lock (locker)
        {
            return i++;
        }
    }

    public void Reset()
    {
        lock (locker)
        {
            i = 0;
        }
    }
}
link|improve this answer
this code doesn't look simpler, I know that it is not recomended to lock on this but it's fine for such small simple and rarely used class imho. – javapowered May 7 at 13:52
1  
Never claimed it was simpler -- just making it work properly. That's the first step before trying to simplify. – Jesse C. Slicer May 7 at 13:53
1  
Nitpicking: lock (locker) makes me think of a box that contains a secret that one locks. I would call it something else ... maybe lockHandle, like Bill Wagner does informit.com/articles/article.aspx?p=1231461 – Leonid May 7 at 13:57
Hm.. I can buy that locker isn't a good name, but I really detest things with handle in them when they're not actually handles, either. lockingObject, monitor (since lock expands to Monitor.Enter and Monitor.Exit), sync ?? – Jesse C. Slicer May 7 at 14:01
feedback

Your Answer

 
or
required, but never shown
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.