Challenge: Do You Truly Understand This Code?

35 commentsWritten on February 19th, 2009 by
Categories: Software Development

I think this might be an interesting challenge for all of you. The code that will be listed below contains a non-obvious solution. The reason why i went with the non-obvious solution, or what exactly the non-obvious part is, will not be mentioned in this post. What i want to know is: do you guys truly understand this code without any sort of "why i did this" comment in the code? I think the code is very readable, and pretty communicative. So according to many coding purists, it should not contain any comments. I am particularly interested in finding out if these coding purists think that this code should contain a "why i did it like this" comment or not.

Now, it certainly is possible that there is a more communicative way to write the same code. If so, i would be very interested to hear about any possible improvements you can come up with :)

I will do a follow-up post about this soon. Perhaps tomorrow, or in a few days, depending on how clear or unclear this code turns out to be.

So, this is the code:

    public class Broadcaster : IBroadcaster
    {
        private readonly object monitor = new object();
        private List<IClientProxy> clients;
        private readonly IClientProxyFactory clientProxyFactory;
 
        public Broadcaster(IClientProxyFactory clientProxyFactory)
        {
            this.clientProxyFactory = clientProxyFactory;
            clients = new List<IClientProxy>();
        }
 
        public void Register()
        {
            var client = clientProxyFactory.CreateClientProxyForCurrentContext("MyCoolNamespace/IBroadcastOverWcf/Receive");
            client.Faulted += Client_Faulted;
            AddClientToRegisteredClients(client);
        }
 
        private void AddClientToRegisteredClients(IClientProxy client)
        {
            lock (monitor)
            {
                clients = new List<IClientProxy>(clients) { client };
            }
        }
 
        public void Broadcast(Notification notification)
        {
            foreach (var client in clients)
            {
                client.SendNotificationAsynchronously(notification);
            }
        }
 
        private void Client_Faulted(object sender, System.EventArgs e)
        {
            var client = (IClientProxy)sender;
            client.Faulted -= Client_Faulted;
            RemoveClientFromRegisteredClients(client);
        }
 
        private void RemoveClientFromRegisteredClients(IClientProxy client)
        {
            lock (monitor)
            {
                var clientList = new List<IClientProxy>(clients);
                clientList.Remove(client);
                clients = clientList;
            }
        }
    }

Update: you can find the commented version of this code here

« Book Review: Building Domain Specific Languages in BooThe Commented Version Of The Readable Code Challenge »

This entry was posted on Thursday, February 19th, 2009 at 16:19 and is filed under Software Development. You can subscribe to any comments to this entry via RSS 2.0. You can leave a response, or trackback from your own site.

  • Michael Smith

    Do I truly understand this code? Probably not! :-)

    Is it something to do with creating a new List everytime you add or remove a client, rather than actually removing the client from the list?

    I’m thinking that it’s something to do with adding / removing new clients while iterating over the client list to broadcast the notification. There’s no lock on the clients list while broadcasting but I’m not sure of the effect of making the clients reference point at a new instance while iterating over it.

    Am I even in the ballpark?

  • http://www.juanformoso.com.ar Juan Manuel

    The only strange thing I see is the re-creation of the list when you add/remove items.

    I don’t know *why* you do that, I could search in Google, but I’d assume it’s a performance issue.

    If I was working with you in the same project, I’d probably ask you to verify you didn’t make a mistake and did it on purpose, so I guess a small comment would help in me not asking you in person =)

    —

    After writing that, I re-read the code… I also don’t understand why you don’t have a lock in Broadcast…

    You could definitely say I’d need some comments then =P

  • http://www.kivela.be Stefan V

    Pulling in my head for a possible flame afterwards, but here it goes…

    Recreating the generic list although you put locking around those operations seems to be a fix for a problem in another part of this code to be honest. (else you would probably have created a thread safe encaps. of the list to prevent other devs to forget the locking part)

    I would guess the Faulted event caused you some troubles and you removed the locking around the clients list in the broadcast method? (Just guessing that the client.SendNotificationAsynchronously could cause a Faulted event on a client, thus updating the collection… So locking on this collection read in the broadcast method would cause a complete lock up scenario)

    Since I am guessing here I do think a small comment would have been nicer if it is a performance improvement … Or a unit test that covers the problem in the other case ofcourse.

    In my humble not requested opinion 8) comments are needed if its a “not so obvious” performance improvement, unit tests if this is one of those hard to tackle bugs…

    Tx for the challenge and the explanation later on.

  • http://davybrion.com Davy Brion

    i have to refrain from already responding to the actual comments, but i am going to point out that i would indeed leave comments on code like this, in fact, the actual file in our subversion repository already has those comments in it :)

    still wondering about ways to make it explicitly clear as to what is actually going on though…

  • http://justrudd.org/ Justin Rudd

    It’s a “copy on write” list. You typically use it when you are going to be iterating more than writing so you don’t want to block all the iterating clients. The only thing that makes it “confusing” is the the foreach loop in Broadcast. People probably look at it and assuming that the client reference is going to change out from underneath them if someone comes along and calls {Add|Remove}ClientToRegisteredClients. But foreach will create an IEnumerator which would live for the life of the function regardless of the memory address of clients.

    as for documentation, I’d probably have a tag on the class itself that indicates that it is using a copy on write list. Or I’d write a class called “CopyOnWriteList” that got rid of the lock statements in Broadcaster. That way just looking at the fields, I’d know that you are doing a copy on write implementation.

  • http://justrudd.org/ Justin Rudd

    Ugh…stripped out all my < (less than signs) :)

  • Michael Smith

    Looks like Justin has nailed it to me.

    I’d never come across the pattern before, so once you get the term “copy on write”, 5 minutes googling and reading and you’ve got the concept.

    If I was coming across this as a maintenance programmer, I would hope that the original developer had some kind of comment along the lines of…

    “There will be much more reading that writing, so use the “copy on write” pattern for the list…”

  • http://www.noctovis.net/blog Laila

    I think the clue of all this should be in the tests, not in additional comments in the code :)

  • http://davybrion.com Davy Brion

    it’s not because there will be more reads than writes :)

    i actually hadn’t heard of the “copy on write” pattern before his comment :D

  • http://davybrion.com Davy Brion

    @Laila,

    what if the test would be so complex that it too would require a comment to explain the issue?

  • http://www.noctovis.net/blog Laila

    Then the code should be simplified… :)

  • http://davybrion.com Davy Brion

    that’s not always possible :)

  • http://stijnguillemyn.wordpress.com Stijn Guillemyn

    My best & quick guess would be the fact that for every add and remove, you create a new list, based on the current list and add or remove the given item. But this looks pretty obvious, given the fact a broadcast uses a foreach, meaning the collection itself cannot change during this operation.

    Since you don’t lock the broadcast (probably not something you want giving the fact you want the broadcast to act instantly) you should ensure the collection doesn’t change by always taking a copy.

    Am I close? Or far off?

    I’m just curious now!

  • http://davybrion.com Davy Brion

    @Stijn,

    i’m having too much fun with this post so i’m not giving anything away just yet :p

  • Pingback: Should you add comments on readable, communicative code? « Stijn Guillemyn

  • http://ayende.com/Blog/ Ayende Rahien

    I would say that the actual reason is performance.
    You aren’t likely to changing the broadcast list all that often.
    By having an immutable value, you can avoid locking whenever you need to broadcast.

  • http://stijnguillemyn.wordpress.com Stijn Guillemyn

    @Ayende

    Indeed. As always, your explanation is more clear, but that’s what I meant with:
    probably not something you want giving the fact you want the broadcast to act instantly

    You need a way to ensure your code doesn’t break when broadcasting, due to a changing list.
    You might lock every access, or use this approach that gives better broadcast performance (which is the main purpose of your class after all).

    To me, comments would be advisable. What’s your take on this?

  • http://davybrion.com Davy Brion

    OK… one hint: in the worst possible scenario (from a load perspective), i’m expecting maybe a couple of broadcasts _per minute_

  • http://bensdeveloperblog.blogspot.com Ben

    To me, the code is very readable

    I agree with Laila that seeing the tests fort his code would show more than comments.

  • http://davybrion.com Davy Brion

    i will post a full explanation of the problem, including the comments i added to the code and the tests that protect against the issue and then you can all decide what is more communicative: the comments or the tests

    but seeing as how i’m enjoying leaving you all in suspense (i’m rather evil that way), i will do that on either saturday or sunday depending on when i have the time to write the post :)

    in the meantime, feel free to keep guessing :p

  • Ian Chamberlain

    I think this is to manage concurrency. You cannot support enumeration (as in Broadcast) with write accesses (Add and Remove) at the same time as List instance members are not thread safe. All you would need to get a problem would be *one* add/remove happening at the same time as a broadcast to generate an exception. You either have to lock the List whilst enumerating or not modify it whilst a broadcast is in progress.
    If the issue is thread safety I like to indicate that in the method name as in ThreadSafeAddxxx or ThreadSafeRemovexxx.

  • http://www.codethinked.com Justin Etheredge

    You are treating your Lists as immutable and concatenating the new item or removing the old item from a new list. You are doing this because if you are in the middle of a broadcast when a client is added or removed it will cause the list to be modified which will throw a “collection was modified” exception from the “foreach” loop since you modified the iterator. Wow that was one heck of a run-on sentence.

  • bob

    Creating a copy on write LIst is OK i guess, if your willing to put up with that kind of overhead.

    My confusion rests in your question about comments. This class does almost nothing.
    Why would comments be required?

    You really dont want a pat on the back for writing 50 lines of clear but essentially minimal functionality do you.

  • http://davybrion.com Davy Brion

    @Bob

    i believe comments are at least helpful in this case because the problem isn’t obvious to everyone nor is the solution

    and no, i don’t want a pat on the back for this… it’s funny you think of that. How much functionality would a class need before one deserves a pat on the back from you? ;)

  • Ben

    I’m not familiar enough with C#, but I would have expected the list variable to be marked as volatile. Otherwise, it can be cached and won’t be seen by all threads until the CPU caches are flushed. At that point, its a standard CopyOnWriteList, and I’d have just made a reusable data structure. This is pretty standard and straight-forward code, imho, and I’ve written similar.

  • http://blogs.ipona.com/james James Hart

    One thing I would say that is overly obscure is the use of a collection initialiser:

    clients = new List(clients) { client };

    you’re using the collection initialiser here to shorten the equivalent code:

    clients = new List(clients);
    clients.Add(client);

    In this case, I’d explcitly expand that out. The problem is that idiomatically, collection initialisers are typically used to populate a collection object after it has been constructed -empty-. Using it to append an item to a prepopulated collection is non-idiomatic, and distracts from noticing that the collection is prepopulated at all.

    Also, looking at your removal code, you might consider, instead of creating a list then removing an item, switching to:

    clients = clients.Where(c => c != client).ToList();

    though that maybe plays down the list creation; perhaps:

    clients = new List(clients.Where(c => c != client));

  • Terry

    Without a doubt, I would say that with no “why” style comments the context and the purpose of this code is unclear to say the least. It’s obvious “what” the code does but “why”? Sadly mean code purists don’t understand this fact.

  • http://theblackboardspoet.blogspot.com/ vnaranjo

    Sure. You have a List of Clients, and to control the access to the list, you use a Monitor (dummy object, just to ensure threat Safety).

  • JM

    This reminds me of a “look how smart I am” blog. Not sure what you were trying to get across in your three blogs on this code. The use of comments? The way foreach works? The I can code neat stuff, can you figure it out (I am in this camp)?

  • http://davybrion.com Davy Brion

    @JM

    the importance of comments in certain situations, no matter what the purists might say. did you really read the 3 posts including the (post) comments? i kinda figured it’d be clear (the point)

  • sl

    There’s no such thing as self-documenting code. Sure, you can always roll up your sleeves and study the code line by line. But when you have hundreds or quite commonly thousands of lines of code, it’s impractical to go through all of them. Regardless of how clean your code is written, nothing beats a few lines of plain English sentences describing what your code does at a high-level (unless you’re writing a “hello world” application).

  • Pingback: Using Copy-On-Write In Multithreaded Code To Reduce Locking Overhead | The Inquisitive Coder – Davy Brion's Blog

  • Pingback: Once Again: Comments In Code Aren’t Necessarily Bad | The Inquisitive Coder – Davy Brion's Blog

  • Pingback: The Commented Version Of The Readable Code Challenge

  • Pingback: The Tests For The Readable Code Challenge

Meta

Recent Posts

Categories