Challenge: Do You Truly Understand This Code?
Posted by Davy Brion on February 19th, 2009
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
February 19th, 2009 at 5:06 pm
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?
February 19th, 2009 at 5:47 pm
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
February 19th, 2009 at 6:21 pm
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
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.
February 19th, 2009 at 8:05 pm
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…
February 19th, 2009 at 8:38 pm
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.
February 19th, 2009 at 8:40 pm
Ugh…stripped out all my < (less than signs)
February 19th, 2009 at 9:24 pm
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…”
February 19th, 2009 at 9:27 pm
I think the clue of all this should be in the tests, not in additional comments in the code
February 19th, 2009 at 9:27 pm
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
February 19th, 2009 at 9:30 pm
@Laila,
what if the test would be so complex that it too would require a comment to explain the issue?
February 19th, 2009 at 9:40 pm
Then the code should be simplified…
February 19th, 2009 at 9:42 pm
that’s not always possible
February 19th, 2009 at 10:17 pm
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!
February 19th, 2009 at 10:27 pm
@Stijn,
i’m having too much fun with this post so i’m not giving anything away just yet :p
February 19th, 2009 at 10:42 pm
[...] you add comments on readable, communicative code? Davy Brion has put some code online to challenge his readers. And guess what… I’m a reader and I like challenges! ”Do you truly [...]
February 19th, 2009 at 10:50 pm
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.
February 19th, 2009 at 11:04 pm
@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?
February 19th, 2009 at 11:34 pm
OK… one hint: in the worst possible scenario (from a load perspective), i’m expecting maybe a couple of broadcasts _per minute_
February 19th, 2009 at 11:35 pm
To me, the code is very readable
I agree with Laila that seeing the tests fort his code would show more than comments.
February 19th, 2009 at 11:47 pm
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
February 20th, 2009 at 1:05 am
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.
February 20th, 2009 at 8:00 pm
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.
February 23rd, 2009 at 7:11 am
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.
February 23rd, 2009 at 8:17 am
@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?
February 23rd, 2009 at 10:26 am
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.
February 23rd, 2009 at 11:41 am
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));
February 23rd, 2009 at 4:17 pm
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.
February 23rd, 2009 at 9:08 pm
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).
February 23rd, 2009 at 9:32 pm
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)?
February 23rd, 2009 at 9:44 pm
@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)
February 24th, 2009 at 12:21 am
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).
March 8th, 2010 at 4:36 pm
[...] leaves the copy-on-write pattern. I’ve used it before with success (though at the time, i didn’t know it was a known pattern) so this approach has [...]