The Commented Version Of The Readable Code Challenge
Posted by Davy Brion on February 21st, 2009
Before i post the tests and a further explanation of the code i listed in the readable code challenge, i wanted to post only the commented version of this code:
public class Broadcaster : IBroadcaster
{
private readonly object monitor = new object();
// This reference will be overwritten with a new instance of a clients List whenever
// we need to add or remove a client from the list. The reason for this is because
// we need to be able to loop through the clients list, but during this loop clients
// might try to register or might be removed from the clients list.
// Using the same instance of the clients List and synchronizing all access with the
// monitor object would not be sufficient because if both the loop and the removal
// would lock on monitor, and the loop and removal would happen on the same
// thread then one of the operations won't block, because that thread already has
// acquired the lock. At that point we get a concurrency exception because clients'
// Enumerator would have been modified while looping through it.
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("Nokeos/IBroadcastOverWcf/Receive");
client.Faulted += Client_Faulted;
AddClientToRegisteredClients(client);
}
private void AddClientToRegisteredClients(IClientProxy client)
{
lock (monitor)
{
// we create a new List instance based on the previous list plus the new client
// and then we assign the new list to the clients reference that the rest of
// this class uses. This happens behind a lock to make sure that the clients
// reference isn't overwritten by the RemoveClientFromRegisteredClients method
// simultaneously
clients = new List<IClientProxy>(clients) { client };
}
}
public void Broadcast(Notification notification)
{
// When we enter the foreach loop, we get a reference to the enumerator of
// the _current_ clients reference (the reference might be overwritten while
// we loop, but our enumerator will never be modified) which means we don't
// need to use a lock here
foreach (var client in clients)
{
// if the send operation fails, the client's Faulted event will be
// triggered which causes removal of the client from our clients list.
// this can happen either while we are in this loop, or afterwards in
// a background thread but in both cases, we don't need to worry about it
// here
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)
{
// we create a new List instance based on the previous list minus the client
// that needs to be removed and then we assign the new list to the clients
// reference. This happens behind a lock to avoid a simultaneuos overwrite
// of the clients reference by the AddClientToRegisteredClients method
var clientList = new List<IClientProxy>(clients);
clientList.Remove(client);
clients = clientList;
}
}
}
I hope the comments clarify the problem sufficiently. If not, tell me what’s not clear to you because it might mean i need to clarify the comments more. Also, suggestions on how to make this code more readable and reducing the need for comments would be very welcome 
I’ll post the tests tomorrow, and then you can all decide what you think is more communicative: the comments, or the tests.
February 21st, 2009 at 7:18 pm
I get the feeling that most of your comments serve to explain the principle of a copy-on-write but they do a good job of explaining why you need this kind of pattern here. Others seem to make up for the poor naming of your ‘monitor’ variable. If you rename it to ‘clientsWriteLock’, it will be obvious from the name and ‘Find Usages’ result what is going on.
The comments “we create a new List instance based on the previous list plus the new client and then we assign the new list to the clients reference that the rest of this class uses” and “we create a new List instance based on the previous list minus the client that needs to be removed and then we assign the new list to the clients reference” pretty much just states what is obvious from the code itself.
One thing that deserves a comment but was left out is why you opted for a copy-on-write instead of a copy-on-read when broadcasting — I assume it is for performance reasons?
February 21st, 2009 at 8:18 pm
@Filip Duyck:
How would a copy-on-read work? Doesn’t a copy imply a read, so wouldn’t it give you the concurrency problem described in the comments?
I do agree on the fact that some parts of the comments are already obvious from the code itself.
They even remind me of Davy’s post Are women better developers than men?, more specifically the part about the female ‘roadmap-comments’ within code…
February 21st, 2009 at 8:20 pm
I would say that you have a lot more comments there than you need.
February 21st, 2009 at 8:52 pm
When I read the code in your original post it was 100% clear what the code was up to and why. Unless your goal is to try to explain threading issues to a less-experienced developer via comments (which I would argue is a pretty bad way to teach someone
) then the amount of comments you have here are just creating noise that make it harder to pay attention to the actual code. I wouldn’t be surprised if this would result in a maintainer simply skimming the comments without stopping to gain an understanding of the code, which would help no one.
My personal recommendation would be to save commenting like this for those rare-but-inevitable occasions where code has to do something very specific for a reason that is very obscure and strongly tied to something entirely outside the control of the developer, with your example not counting as such a situation given that it is a general solution to a common problem, not, for example, a seemingly-incorrect-but-in-reality-utterly-unavoidable work-around at a integration point with a problematice external system. In other words, you should never have to comment on who, what, when, or where, and should only comment on why when why _only_ makes sense in this _single_ situation and no other.
February 22nd, 2009 at 12:51 am
@TomC:
You would copy the list before iterating it. No concurrency problems if you lock during the copying, and no problems with changes to the list while iterating it, because changes are done to the original rather than to your copy.
February 22nd, 2009 at 10:40 am
@Filip
i’ve followed your suggestion… renamed the monitor and gotten rid of the comments in the AddClientToRegisteredClients and RemoveClientFromRegisteredClients methods. So now, i only have the comment block on top of the clients list field and the comments in the Broadcast method. Kinda like the result, thx
As for why i used copy-on-write instead of copy-on-read… i have no idea, this was just the first solution i thought of using. Haven’t tried the copy-on-read yet, but it actually might make it more explicit as to what is going on. You’d have to put locking in all 3 ‘tricky’ places (addition, removal, pre-looping) so to the reader, it might be more clear as to what is going on without requiring comments.
@Jeremy
For you, this code might have been entirely clear from the beginning, but i’m pretty sure that that wasn’t the case for most people who read it. When i posted the original code, a lot of people were asking me for hints in the comments, at the office, on IM, email, etc… So i think the commenting on the ‘why’ does make sense in this example. Oh and i’d hate to see code that you _don’t_ find clear
February 22nd, 2009 at 10:57 pm
If you decide to leave in comments, I’d really try to shorten them up, and that’s very feasible in this example
And why not put them as xml-remarks to the class or to variables as xml-summary?
February 23rd, 2009 at 11:15 pm
@Davy – Believe me, it happens.
I suppose my background (and working environment) is a bit more ruthless than average when it comes to factoring code for understandability that avoids getting comment-happy. In many ways, I suppose I’m more inclined to being ruthless in that manner to help counterbalance the comment-happiness that is prevalent in a lot of codebases. Codebases where comments are used to shore up crap code that itself could easily be made much clearer and avoid the risks of comments where possible (e.g. getting out of sync with the code, what have you.)
To be clear: complicated code should be commented. But only once as many as possible of those comments have been refactored into the structure of the code itself (in the names of members, etc.)
Keep up the great series of posts! I’m really enjoying them so far.
February 24th, 2009 at 6:57 am
@Jeremy
well, as far as this piece of code is concerned, the ’series’ is pretty much over… i think i’ve already milked it for all it’s worth