Appropriate Comments
time to read 8 min | 1418 words

Both Analtoly and Oren Ellenbogen  had commented on my commenting post (pun intended). The example we are using right now is this piece of code:

outputs = new EntitySet<IConnection>(
      delegate(IConnection connection) { Raise(OutputAdded, connection); },
      delegate(IConnection connection) { Raise(OutputRemoved, connection); }
      );

I argue that this piece of code should stand on its own, without any comments to explain what is going here. I think at least part of the issue here is that I am focusing entirely on the comments on the code and not XML Comments. Here is the code for the EntitySet, the class start with a big XML comment that start with general explanation on the class and then goes on to some detail with regard to some detailed implementation that has to do with NHibernate.

To quote the first part of the comments:

/// This class handle strong typing collections that you get from
/// NHibernate.
/// It also handles adding/removing items to the other end (by allowing
/// you to specify delegates to do it).

And the constructor that I'm calling about is defined as:

public EntitySet(Action<T> add, Action<T> remove)

Putting this two together, I'm comportable with not commenting what is going on in the output declaration. It should be obvious by reading the xml comment on EntitySet<T> and seeing the constructor overload I'm using.

With regard to commenting on a library code, Anatoly said:

When you call the library function, the comment what the function does should be in the header of the function. When you change implementation you change the comment. When you call the library function (even in several places) need to say what the line of code does (calling x expecting y) here it may be very light. But still need to comment.

If I need to comment on the callsite for a method, that is a sign for bad design on my code. The possible exceptions is I'm doing something non standard there (like expecting spesific result because the conditions are just right) and if I'm doing it more than once, I need to refactor to remove the comments.

The case above is a classic one, I think. If you don't understand what is going on there, you really don't understand what is going here. It's not a question of not understanding some side affects. In this case it's whatever you know what the library does or not.

Ayende: You seem to say that the code should be optimized for someone completely ignorant about the project, just seating down and understanding what is going on.

Anayoly: Something like this. My point is that clear and self-descriptive code is a must but it does not mean that you don't need comments.

This is the part I disagree with most strongly, I think. Clear code doesn't need call site comments. They are a chore to write when you write the code, they are noise when you read the code. On heavily commented code, my eyes gets into 'ads-skipping' mode, and I literally don't see them more than green blurs on the screen.

I tend to think that a big enough project require some rumping up time before you understand what is going on. Anatoly says that it is possible to have a new guy on a project and starting to contribute useful features in a very short times (minutes to hours).

I've seen accessible projects. You feel the difference right away.

Could you explain what made it accessible? Was it that you were familiar with the technology and the development methods that were used? Was it good documentation? How much time did you spend reading the comments in the code in order to learn what is going on there. I said in the comment that I believe that it is possible only on "common ground" projects, where the architecture is mostly the same.

Moving to Oren's comment about this now. The first suggestion that you had (refactor creating the outputs into a method that would have a comment there) is something that I would generally prefer to do if the code needed explaining. In this case, do you think that the comments on the class and the naming of the parameters are enough?

It seems to me (I'm guilty at this as well) that you fall in love with complex code but you forgot that someone else would have to maintain your code somewhere in the future.

Yes, and no. I sometimes does complex code for the fun of it, but I don't intend to take this code to production. There is some elegance for complex code that works. But if I look at it afterward and need more than a minute to understand what i did there, it's useless, in my eyes. This is part of the reason I like peer programming. You can't do stuff too complicated without sounding it out first, and then the other guy will likely offer a better / simpler way to do this.

I just had a case where i wrote a method that passed a test, but when I went back to the method and looked at it, I had no idea what was going there. I ended up breaking the method to several smaller methods, so I got something like:

AssertNoCycles();
List<Node> visited = new List<Node>();
foreach(Node node in graph.Nodes)
{
    DoWork(node, visited);
}
AssertAllNodeWereVisited(visited)

Imagine that each of those methods in inlined, and you'll start to see the problem, I guess. My first instinct is to break the code to manageble pieces rather than explain what I did there.

I liked Oren's second suggestion, which is to have a certral repository of "Required Reading" before you can work on the project. I'm not talking about 1,600 design guides. I'm talking about much simpler stuff like:

This project uses the following libraries, and here are the documentation. You probably need to check the following three links to get up to speed with what they do, and we are using them to do this and that (links to everything).

This allows you to assume a common base for the people that would work on the project.

Tweet Share Share 0 comments
Tags: