Grammar and Clarity in Code Comments

[Lost User]

AspDotNetDev wrote:

Only some HTML controls have a specific representation in .Net. One of them that does not, UL, requires use of the general purpose HtmlGenericControl. I would use the term "HTML list" (or something similar) in the code comments rather than "UL", but I felt it more appropriate to indicate the more concrete version of what is actually happening to make it easier to understand.

You could call the method CreateULGenericControl() perhaps Also, who do you specify one possible return value in the remarks section? Wouldn't you specify the return value as "The UL holding the children or NULL if there are no children"?

[Lost User]

Wellllllll,,, Firstly I see confusion with the descriptions - parentNode and currentNode parentNode is the node containing the children to be added to the UL currentNode is what? The node into which the UL is to be inserted? Or is it actually the page? It probably doesn't matter, but I don't see that the current node is necessary, and parentNode confuses me a bit, as I think of it as being the parent to the UL, not the data for the UL. As for the comment - I'd probably go with something like The node containing the items to be placed in the UL and the returns comment I would then change to The UL containing the items from the parentNode

MVVM# - See how I did MVVM my way ___________________________________________ Man, you're a god. - walterhevedeich 26/05/2011 .\\axxx (That's an 'M')

[AspDotNetDev]

_Josh_ wrote:

You could call the method CreateULGenericControl() perhaps

Nah, there are many functions in the class that can return UL's. I named it more towards what it's creating the UL for (it's part of a class that is essentially converting a very specific in-memory hierarchy to HTML).

_Josh_ wrote:

Also, who do you specify one possible return value in the remarks section?

I always assume that null may be returned from a function that has a return type that is nullable. Though if it is intended to direct functionality (i.e., the null is expected in some cases), I will either put it in the return comment or the remarks comment. I just happened to side on putting it in the remarks comment this time (I see remarks as not only a place to put things that don't fit anywhere else, but also a place to put additional information that would make the other sections excessively long). Basically, I like to keep the most important information where it will most likely be used.

Somebody in an online forum wrote:

INTJs never really joke. They make a point. The joke is just a gift wrapper.

[AspDotNetDev]

Luc Pattyn wrote:

Maybe: Creates the UL and populates it with the children of the parent node

Good idea. This might be more appropriate for my specific use: "Creates a UL that contains the children of the specified node."

Luc Pattyn wrote:

And currentNode is a mystery to me

It would make more sense if you were familiar with the class this function is part of and the types of nodes that are being passed around (that was one of the things I obscured in my OP).

Somebody in an online forum wrote:

INTJs never really joke. They make a point. The joke is just a gift wrapper.

[Steve Mayfield]

parentNode - The node that contains the children to be placed in a UL.

Steve _________________ I C(++) therefore I am

[AspDotNetDev]

We have a website that is hosted in a CMS. Each page is represented in-memory as a "node". It is a hierarchical data structure. The currentNode is the node that represents the page currently being viewed by the user. It may be used by this class to, for example, change the classes on various HTML elements, such as UL's and LI's. The parentNode is the node that the function happens to be working with.

_Maxxx_ wrote:

The node containing the items to be placed in the UL

I like that. :thumbsup:

Somebody in an online forum wrote:

INTJs never really joke. They make a point. The joke is just a gift wrapper.

[Michael Bergman]

AspDotNetDev wrote:

The node for which its children will be placed in a UL.

More proper, but still clunky:

The node in which its children will be placed in a UL.

Slightly more clear:

The node that holds children nodes which will be placed in a UL.

Slightly more obscure:

The node for unsorted children.

Slightly more irritated:

Why don't you read the #*(%!$& documentation.

m.bergman

-- For Bruce Schneier, quanta only have one state : afraid.

[Rob Grainger]

"The one with the flat tire". The one who's tire is flat sounds wrong to me.

[jsc42]

I'd go for a starter which defines the type of entity and then a description. This means you can do ...

''' <param name="parentNode">(Node) This node contains the child nodes that are to be copied to the UL.</param>
''' <param name="currentNode">(Node) The node that corresponds to the current page.</param>
''' <returns>(Node) The UL node that holds the children after they have been copied.</returns>

[agolddog]

The into which the children will be inserted [placed]. Not sure that conveys your process exactly, but I think it's pretty correct grammatically.

[Harley L Pebley]

AspDotNetDev wrote:

The currentNode is the node that represents the page currently being viewed by the user. ... The parentNode is the node that the function happens to be working with.

Maybe it makes more sense in context of the whole API, but to me with just this snippet, the names seem to be almost exactly opposite the definitions. Is that why it's so confusing to write the documentation? Perhaps it's not a problem with the comments so much as the method signature. A couple questions, looking at just the parameters: - Why have "Node" in the name at all? We know it's type based on the function signature. It smacks of post-fix Hungarian notation. - Neither of these are HTML nodes, correct? If I understand properly, they're nodes in the domain structure. If so, then perhaps replace "Node" with "Source" (as in the source of the data). The returned result would be the destination. - Rather than "current" and "parent", perhaps "page" and "current" (where what's now parent becomes current). Backing up a bit to look at the whole signature:

Private Shared Function CreateChildrenList(ByVal parentNode As Node, ByVal currentNode As Node) As HtmlGenericControl

There are three actors: children in the function name, parent in the first parameter and current in the second parameter. And, if I understand correctly, the children that are being returned are the items from parent converted to some HTML representation. The parent parameter is a container in the domain object hierarchy and the current is another container in the domain hierarchy. This seems pretty confusing. Parent and children aren't the same types, nor do the children belong to the parent. Current is more of a global context, not the current thing this function is interested in. Assuming I understand all this correctly, I'd suggest something more along these lines:

Private Shared Function ConvertItems(ByVal itemContainer As Node, ByVal page As Node) As HtmlGenericControl

"Convert" emphasizes the type differences between what goes in and what comes out. By having "Items" in both the function name and first parameter, it shows these as related. By renaming the last parameter, it emphasizes its global nature. Depending on how the items are referenced in the Node type, I might even name the first parameter just "items" instead of "itemContainer". I know it's not what you asked, but hopefully it helps anyway. :)

[KP Lee]

AspDotNetDev wrote:

<param name="parentNode">The node for which its children will be placed in a UL.</param>

"Children of this node will be placed in a UL."

AspDotNetDev wrote:

The node of the current page

"The current page's node." I know it sounds possesive, but it seems right

AspDotNetDev wrote:

The UL that holds the children

"null or UL holding existing children" Removes the need for one of the remarks One of the greatest tools implemented in VS is intellisense. I remember when I was learning VB.NET, typing in "'''" and expecting an autocomplete of the XML format used for intellisense. Didn't happen. A little disappointing, but OK, I'll look up how to do that in VB. ARE YOU KIDDING ME!!!! VB doesn't support intellisense! So, do you use that formatting because you like it or did MS get off their duff and fix one of their biggest goofs in VS?

[AspDotNetDev]

KP Lee wrote:

null or UL holding existing children

That doesn't say under what conditions null will be returned.

KP Lee wrote:

did MS get off their duff and fix one of their biggest goofs in VS

I think VB.Net intellisense for XML comments has been around since about VS2005.

Somebody in an online forum wrote:

INTJs never really joke. They make a point. The joke is just a gift wrapper.

[AspDotNetDev]

Thanks for the analysis. I wouldn't go with "page" as the name of the second parameter. For one, this is inside of a user control and the property "Page" is already defined (I don't like to name things the same as something in a greater scope). Also, the name "page" just indicates that the parameter is a page, but not which page. Maybe "currentPage" would be more appropriate. The funny thing about the CMS I'm using, though, is that a node need not be a page. It could also represent just a piece of data under a page that a page uses. Still, in this instance, the item being passed would always be a page, so "currentPage" does seem like a more appropriate name. I might also choose something like "requestPage" or "activePage", though I'm ok with "currentNode". I suppose I could go with just "current" or "request" (though I think "Request" is another property on user controls), but that makes the code more confusing (e.g., you must refer back to the type to know what the "current" thing is). Not to mention "currentNode" is a standard name used in tons of places in our codebase, so when people see it they probably know what to expect from it. Also, I would not go with "ConvertItems". For one, that's extremely generic and I have several conversion type functions in the same class that could go by that same name. Naming the function "CreateChildrenList" and the parameter "parentNode" gives the programmer and idea of how the parameter relates to the function. When you name the function "ConvertItems", it is not clear what type of conversion is taking place (e.g., is the "itemContainer" part of the result?). Also, I usually reserve "Convert" for when I'm converting between types, and I would probably use that as a class name rather than part of a property (classes help to give context to the methods, which is something that my OP doesn't mention). What I'm doing here is dynamically construction a partial representation of the domain object as HTML. For example, properties on some nodes (aka, pages, in most cases) may prevent them from being rendered as LI's. Other names I might have used include "BuildChildrenItems" and "ConstructSubItems" (both initial words convey some idea that more than a plain isomorphic conversion is taking place). That all being said, I don't spend too much time thinking about how I name my variables in private functions. I'm happy so long as they're pretty obvious and sensibly short. The comments serve to offer further explanation of what exactly the parameters are for.

[AspDotNetDev]

It would be grammatically correct if you insert "node" after "The". However, it's not an accurate description of the parameter.

Somebody in an online forum wrote:

INTJs never really joke. They make a point. The joke is just a gift wrapper.

[KP Lee]

AspDotNetDev wrote:

That doesn't say under what conditions null will be returned.

It's not implied? If it is not null it will return a UL holding existing children. Usually when you define a return type, you want to return a non-null value. So if you didn't get a UL returned, doesn't that imply there aren't any existing children to be returned? Also, doesn't it imply you will not get a non-null UL returned with no children in it? Thanks for the intellisense info.

[Florin Jurcovici 0]

Unless this code sits in a general purpose, reusable library, and you plan to generate documentation for client programmers from code, I'd remove comments altogether. If you need comments to make your code understandable, there's a problem with the code, one for which comments are a solution the same way a pain killer is a cure for a tooth ache. Another issue with comments is that there is no compiler checking for them, so they usually are the first thing to become buggy, without any unit tests being able to detect this. If you do want to generate documentation from comments, put yourself in the position of the reader of the comments. What would you understand from the summary? It very much depends on the context where it appears in the documentation. But I very much doubt that "the specified node" is being better specified by your context. "Creates an UL from the children of a node." seems better to me. The parameter descriptions then properly specify which argument is the node which's children wil be used. And it's a tiny little bit more concise (unlike my response here). "All descendants will be handled." - IMO a tiny bit ambiguous, because in normal language use people often distinguish between direct and indirect descendants. "Descendants will (not) be handled recursively." is IMO more precise, although more verbose. At first glance, what I'd change is the naming of the parameters. If currentNode is a page node, call it pageNode. If it isn't, give it a name stating what it is - just node might be enough. It's obvious that parentNode is a Node, so using a name stating that it is the source of the children, like for instance childrenSource or childrenContainer, seems more fit, IMO. The same for the function name: it creates an UL from a node's children. Then way not BuildULFromChildren, or ULFromNodeChildren, or just ULFromChildren? CreateChildrenList might create an UL, an OL, a DL, or simply append a new paragraph for each child - the name doesn't tell us anything in this regard. Such aspects are IMO more important than whether to use which's or whose.

[Neal8900]

parentNode - The parent node, the children of which will be placed in a UL.

[AspDotNetDev]

Florin Jurcovici wrote:

Unless this code sits in a general purpose, reusable library, and you plan to generate documentation for client programmers from code, I'd remove comments altogether. If you need comments to make your code understandable, there's a problem with the code, one for which comments are a solution the same way a pain killer is a cure for a tooth ache.

I disagree. Comments are of real practical use. That is one reason they are included in general purpose, reusable libraries. I include comments in all of my code. They assist with understanding the code, even if that code isn't a general purpose library. If a control needs to be modified (which we do all the time to add features), the comments help guide the programmer faster through the existing code.

Florin Jurcovici wrote:

All descendants will be handled

Florin Jurcovici wrote:

Descendants will (not) be handled recursively

When dealing with tree structures, "children" and "descendants" are common terms that refer to precise concepts. Children are direct descendants and descendants are children of children, recursively. No need to repeat what is is part of the definition of the word. Of course, the "recursively" could refer to the fact that the function is recursive (which it is) rather than iterative. However, that leads to some ambiguity and I might just add that as a full sentence in the remarks ("This is a recursive function") or as part of the summary.

Florin Jurcovici wrote:

At first glance, what I'd change is the naming of the parameters. If currentNode is a page node, call it pageNode.

The "current" is the most important part of the name. It is really the current request page node, but currentRequestPageNode seems a tad bit verbose, so I shortened it to currentNode. Seems like a fine enough name, though as I mentioned to somebody else above, currentPage or requestPage might have been more appropriate and less ambiguous.

Florin Jurcovici wrote:

BuildULFromChildren

I like that name. :thumbsup:

Somebody in an online forum wrote:

INTJs never really joke. They make a point. The joke is just a gift wrapper.

[trempnvt]

That's because it is wrong: "who's"="who is" ;P