More on comments and code
time to read 8 min | 1404 words

Anatoly Lubarsky has commented on my previous post about commenting:

Sorry for this but you are totally wrong.
I don't want to search or read documentation or debug to understand your code.
It is totally useless work until you write for yourself only.

I also don't like this code and don't think it is descriptive.

I started to reply, but it turned to be pretty long and pretty important, so I'm posting it here.

Codebases do not exist in isolation. Explaining what is happening in the code because it's not doing the common thing get repetitive and annoying very quickly. To take few examples:

EntitySet<T> and its associated actions are not simple to grasp, but the moment you grasp them, there isn't any point to comments, it is just as useless as the "disable the button" one. Take a look at the code here. This is not commented, and is not likely to be. The moment you understand that EntitySet will call the delegate methods on add and removal, it should be clear what is going here. In this case, the collection will raise events in the parent class with very little coding on our part. Could this use a comment for somebody other than me?

Well, this did deserve its own post when I did it, so I guess that this means that I admit that it is not something trivial (not that I don't post about trivialities), but the other approach means that the code will look like:

// This will raise the OutputAdded event when a connection is added to the outputs
// and OutputRemoved event when a connection is removed from the outputs.
outputs = new EntitySet<IConnection>(
      delegate(IConnection connection) { Raise(OutputAdded, connection); },
      delegate(IConnection connection) { Raise(OutputRemoved, connection); }
      );


// This will raise the InputAdded event when a connection is added to the outputs
// and InputRemoved event when a connection is removed from the inputs.
inputs = new EntitySet<IConnection>(
      delegate(IConnection connection) { Raise(InputAdded, connection); },
      delegate(IConnection connection) { Raise(InputRemoved, connection); }
      );

Take a look at this code, it explains what is going on here, but is it useful to you? The first time, maybe. But the second time? And the third? And the 100th? This code has a comment bug in it, by the way. Can you find it?

Here is another example, this time it is from NHibernate Query Analyzer and it has to do with executing a database query in a background thread. This is complex stuff, I am sure that you would agree:

ICommand execQuery = new ExecuteQueryCommand(view, mainPresenter.CurrentProject, view.HqlQueryText, typedParameters);
mainPresenter.EnqueueCommand(execQuery);

Here is the commented version of this code:

// This command will be executed in a background thread. ExecuteQuery will take care of syncronizing the updates to UI and notifing it 
// it when it was done or it failed.
ICommand execQuery = new ExecuteQueryCommand(view,  mainPresenter.CurrentProject, view.HqlQueryText, typedParameters)

// Enqueues the command on the main presenter work items queue, which will be picked by the background thread 
// as soon as it is done with other stuff. This allows to keep a responsive UI in the face of long-running queries.
mainPresenter.EnqueueCommand(execQuery);                  

Now it also explain what is happening, and you know that the ExecuteQuery class will take care of handling returning the query to the UI. But I'm using code similar to this one for all my multi-threaded communication, do I really need to give basicaly the same comment over and over again? Do I need to do it just once? What if the other guy that is maintaning the code is looking at another location where I did this, but not where I left the comment?

Here is another example. Here Blog is an Active Record class with a lazy loading for Posts:

Blog blog;
using(new SessionScope())
{
   blog = Blog.Find(id);
}
foreach(Post p in blog.Posts)
{
   // do something
}

This is wrong. It will throw an exception when you try to access the Posts collection, but unless you are familiar with the way Active Record works (or you already go this exception), it will make not sense. Do I need to put the foreach inside the using and explain why? What about all the dozens of other places where I access lazy loaded collections (something that is certainly not obvious)?

Like I said, I don't believe that comments should be used to communicate the common stuff (even if it is not obvious). If you try to work with any code base of meaningful size, you either has to avoid doing anything of any complexity (which mean a lot of repetitiveness) or you are going to develop common idions (for this spesific codebase) for dealing with it. Not reading the documentation {best scenario} (or talking to other team members, or reading the tests and sweaping the code) when you are new to a project makes absolutely no sense unless you are confident that you can manage as you go along.

A project has common idioms for dealing with problems. In the context of the project, explaining what they do amounts to commenting "adding 1 to x" when you use: x++. When I work, I assume that the other people working on the project are familiar with the same idioms. Yes, it make it harder to understand the code in the start, but show me one significant project when someone can just come in, have a few minutes to look at the code and can start real work on it.

Tweet Share Share 0 comments
Tags: