Grammar and Clarity in Code Comments
-
Suppose I created this function (changed a bit to protect the innocent):
''' <summary>
''' Creates the UL to hold the children of the specified node.
''' </summary>
''' <param name="parentNode">The node for which its children will be placed in a UL.</param>
''' <param name="currentNode">The node of the current page.</param>
''' <returns>The UL that holds the children.</returns>
''' <remarks>
''' If there are no children, null will be returned.
''' All descendants will be handled.
''' </remarks>
Private Shared Function CreateChildrenList(ByVal parentNode As Node, ByVal currentNode As Node) As HtmlGenericControl
' Yada, yada, yada...
End FunctionThe comment I wrote for
parentNodeseems a little off for some reason. It might be because it's overly formal or verbose, but I'm thinking the grammar is wrong in some way. I could just write "The children of this node will be placed in a UL." However, I want to place "the node" at the beginning (I think this is a certain type of sentence, but I'm not good enough with grammar to know what to call it... something relating to the subject/object ordering). How would you change that comment ("The node for which its children will be placed in a UL")? Or would you leave it as is?Somebody in an online forum wrote:
INTJs never really joke. They make a point. The joke is just a gift wrapper.
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 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?
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 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. :)
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.
-
The into which the children will be inserted [placed]. Not sure that conveys your process exactly, but I think it's pretty correct grammatically.
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 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 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.
-
Suppose I created this function (changed a bit to protect the innocent):
''' <summary>
''' Creates the UL to hold the children of the specified node.
''' </summary>
''' <param name="parentNode">The node for which its children will be placed in a UL.</param>
''' <param name="currentNode">The node of the current page.</param>
''' <returns>The UL that holds the children.</returns>
''' <remarks>
''' If there are no children, null will be returned.
''' All descendants will be handled.
''' </remarks>
Private Shared Function CreateChildrenList(ByVal parentNode As Node, ByVal currentNode As Node) As HtmlGenericControl
' Yada, yada, yada...
End FunctionThe comment I wrote for
parentNodeseems a little off for some reason. It might be because it's overly formal or verbose, but I'm thinking the grammar is wrong in some way. I could just write "The children of this node will be placed in a UL." However, I want to place "the node" at the beginning (I think this is a certain type of sentence, but I'm not good enough with grammar to know what to call it... something relating to the subject/object ordering). How would you change that comment ("The node for which its children will be placed in a UL")? Or would you leave it as is?Somebody in an online forum wrote:
INTJs never really joke. They make a point. The joke is just a gift wrapper.
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.
-
Suppose I created this function (changed a bit to protect the innocent):
''' <summary>
''' Creates the UL to hold the children of the specified node.
''' </summary>
''' <param name="parentNode">The node for which its children will be placed in a UL.</param>
''' <param name="currentNode">The node of the current page.</param>
''' <returns>The UL that holds the children.</returns>
''' <remarks>
''' If there are no children, null will be returned.
''' All descendants will be handled.
''' </remarks>
Private Shared Function CreateChildrenList(ByVal parentNode As Node, ByVal currentNode As Node) As HtmlGenericControl
' Yada, yada, yada...
End FunctionThe comment I wrote for
parentNodeseems a little off for some reason. It might be because it's overly formal or verbose, but I'm thinking the grammar is wrong in some way. I could just write "The children of this node will be placed in a UL." However, I want to place "the node" at the beginning (I think this is a certain type of sentence, but I'm not good enough with grammar to know what to call it... something relating to the subject/object ordering). How would you change that comment ("The node for which its children will be placed in a UL")? Or would you leave it as is?Somebody in an online forum wrote:
INTJs never really joke. They make a point. The joke is just a gift wrapper.
-
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.
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.
-
"The one with the flat tire". The one who's tire is flat sounds wrong to me.
-
I think you are right about "whose". According to this, "whose" can be used as the possessive form of "which". Since I was trying to do "which's", it makes sense to use "whose" instead.
_Josh_ wrote:
The entire thing smells a bit to me. The only reference to 'UL' is in the comments and bears no obvious relationship to 'ChildList' which is what the method name suggests it creates or the HtmlGenericControl type it returns. Perhaps it's obvious to someone with some web programming knowledge but it's not to me. If you're creating something derived from HtmlGenericControlthen return the more specific type as you know what it is.
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.
Somebody in an online forum wrote:
INTJs never really joke. They make a point. The joke is just a gift wrapper.
-
Are you guys writing software or competing in a comp for the literacy department :-D
Good advice is always certain to be ignored, but that's no reason not to give it.
As an autodidactic, I don't feel the need to choose. :)
Somebody in an online forum wrote:
INTJs never really joke. They make a point. The joke is just a gift wrapper.