Continuing in what's turned into my series on locking advice, I have a new example for you of a typical problem that happens in class design.  As usual I've simplified this almost to the point of being silly to illustrate the problem.  Hopefully the crux of what is going on still shows through.

The theme for today is that often synchronization is inserted into the wrong level of programming code.  Low level classes, like the Counter in this example, have very little idea what is going on in the world around them and so they can't make especially good synchronization decisions. 

Often synchronization at the low level ends up doing nothing useful, but instead only creates complexity and wastes cycles.  My "favorite" example of this is the availablity of synchronized collection classes which, like the Counter example below, have difficulty meeting the synchronization needs of their clients because they have no understanding of the overall unit of work and are therefore unhelpful when composed with other resources.

    public class Counter
    {
        private string name;
        private int reads;
        private int writes;

        public Counter(string _name)
        {
            name = _name;
            reads = writes = 0;
        }

        public void AddRead()
        {
            lock(this) // point #1 -- do we like the three lines marked point #1 ?
            {
                reads++;
            }
        }

        public void AddWrite()
        {
            lock(this) // point #1 -- do we like the three lines marked point #1 ?
            {
                writes++;
            }
        }

        public void WriteCounts(TextWriter tw)
        {
            lock(this) // point #1 -- do we like the three lines marked point #1 ?
            {
                // point #4 -- do we even like this method for this class?
                tw.WriteLine("{0} {1} {2}", name, reads, writes);
            }
        }
    }

    public class Updater
    {
        private static Counter c1 = new Counter("Category 1");
        private static Counter c2 = new Counter("Category 2");
        private static Object myLock = new Object();

        // this method is called by many threads
        public static void DoWork(bool br1, bool bw1, bool br2, bool bw2)
        {
            lock (myLock)  //  point #2, do we need this? does it even belong?
            {
                if (br1) c1.AddRead();
                if (br2) c2.AddRead();
                if (bw1) c1.AddWrite();
                if (bw2) c2.AddWrite();

                // point #3 could we change the lock so that it didn't include these two slowish statements?

                c1.WriteCounts(Console.Out);
                c2.WriteCounts(Console.Out);
            }
        }
    }

What are your thoughts on the marked points (1-4)?  What would a better Counter class look like to meet the needs of DoWork?

(my "solution" is now available here)