OK, I used that entry title to get your attention. What it really should say is "Before you add a comment to your code, stop and ask yourself why you think you need a comment here." Clint Willard has an interesting blog entry today on code comments, and I wanted to expand a bit on what he had to say. I agree with Clint that comments are mostly unnecessary in a clearly coded application.

In the vast majority of cases, a comment indicates that something is wrong. If you have to add a comment to explain what a block of code is doing, that means that the code is too complicated or is using poor variable names or function calls.

Consider this code:

<cfoutput>
<!--- Replace the dots with slashes, then replace the root mapping name with the absolute path from the web root to the application. --->	
#ReplaceNoCase(ListChangeDelims('sitemapping.components.services', '/', '.'), 'sitemapping/', '/mysite/')#/
</cfoutput>
		

Without the comment, someone looking at this code would probably be rather confused, or at least require them to take the time to read over it several times to dissect it and figure out what it is doing. The comment helps to explain what the code does. But this is still an undesirable outcome. In cases like this, one should consider moving the code into its own method and using good naming to convey what the code does. Something like:

<cffunction name="convertPackageToPath" access="public" returntype="string" output="false" hint="I convert a package name into an absolute path from the web root.">
	<cfargument name="package" type="string" required="true" />
	<cfset var path = ListChangeDelims(arguments.package, '/', '.') />
	<cfset path = ReplaceNoCase(path, 'sitemapping/', '/mysite/') />
	<cfreturn '#path#/' />
</cffunction>

<cfoutput>
#convertPackageToPath('sitemapping.components.services')#
</cfoutput>
		

Now we have much more self-documenting code. Someone coming to this line of code can understand what is going on immediately. And if we spend a little more time to make the values in the method more dynamic, such as the mapping name or the site root, we gain even more benefit. The method becomes a nice, generic option for a wide range of use cases where we want to convert package names into web-root-relative paths.

That said, of course comments have their place. They can be a good place to explain why something is happening. And they are fine for header information, documentation, or TODO entries that will be picked up by the IDE and added to your task list. But the main point is that self-documenting code should be the goal. So the next time you write or run into code that you think needs comments to explain it, consider refactoring to remove the complexity and make life a bit easier for yourself and anyone looking at the code in the future.

Comments Comments (19) | del.ico.us del.icio.us | Digg It! Digg It! | Linking Blogs Linking Blogs | 13310 Views

Comments

  • # Posted By James Allen | 6/5/08 8:54 AM

    I certainly agree that code should be as self documenting and clear as possible, but surely in your example the function 'hint' is really a comment in many ways. So it's still important to have these kind of descriptions along with clear, well structured code?

  • # Posted By Brian Kotek | 6/5/08 8:59 AM

    That's true, but I tend to use the hint more for API documentation generation than for explaining what something does to someone looking at the code. Though it does indeed have that additional benefit. However, the real point is that the person looking at the code *calling* the method doesn't need much more information to understand what it is doing. If the method itself were in some other CFC (i.e. utility.convertPackageToPath()) the coder looking at this page would never even see the hint but would still know what was going on.

  • # Posted By Matt Williams | 6/5/08 9:31 AM

    This reminded me of a comment from one of Ray's recent blog posts:

    "I'm not a big fan of making things shorter just to save typing. That's one of my complaints about jQuery. I like the power of it - but sometimes I almost wish it had a bit more code to it to make it readable. (Blasphemy, I know.)"

    Making code more readable is a great goal. Especially when you're going back to 1 and 2 year old code to make changes. When things are readable, it is all the easier to do those updates.

  • # Posted By Ben Nadel | 6/5/08 9:35 AM

    I think posts like this can be dangerous. It's one thing when master programmers read this and realize that their "self-documenting" code is good enough; but, I fear, that many lazy programmers will see this as an excuse to not comment, while at the same time writing really bad code.

  • # Posted By Todd Rafferty | 6/5/08 9:53 AM

    @James: Brian isn't saying NOT to comment your code. He's not even saying that hints are the end-all, be all. He's just saying if you have a confusing block of code you might be better off wrapping it up in a UDF or CFC Function to break it down a little to make it easier to follow and being able able to hint what that function is all about is a benefit.

  • # Posted By James Allen | 6/5/08 9:56 AM

    Yes that is a very good point.. I've started looking out for over-commenting and found a couple of examples in the code I'm writing today.
    The problem I found (probably from habit) is that I kind of like comments to break up blocks of code. I.E in one of my gateway methods I set two composed objects into a Transfer object. setUser() and setThread(). The comment is simply ("Set the user and thread objects").. So it's pretty useless really as the code does speak for itself.. However, without the comment the code kind of looks abit bare to me.. Those statements just sitting there on their own.. lol.

    I think I need to get used to not only structuring my code to make it clearer but to cut down on obvious un-needed comments.. Might take some time to break that habit I think.. :)

  • # Posted By Dan G. Switzer, II | 6/5/08 9:59 AM

    @Brian:

    IMO, both comments miss the mark a bit because neither tell you *why* you're doing what your doing--which I think is most important point to convey.

    Your UDF hint is much better than the inline comment, but what's most helpful is saying "why" you're doing something. That's much more helpful in the long run, as it helps you determine whether blocks of code are still necessary or even what the intent of the code is for.

    I can't tell you how many times I've looked at comments from other developers that just spelled out the logic in an if statement. While that can be useful for really complex IF statements, it's much more useful to tell state *why* you're performing the conditional check in the first place.

  • # Posted By Brian Kotek | 6/5/08 10:05 AM

    I suppose that is a possible danger, Ben. But I'm not sure how much more straightforward the advice can be. On the flip side, there are actually dangers with lots of explanatory code comments because unless the developer is vigilant, changes to the code may not be reflected in the many associated comments. This is actually even worse than no comments, because now the comment is actually describing the code incorrectly. Any time there is duplication of knowledge (as there are when comments must explain what code is doing), it becomes one more thing that must be maintained.

    Either way, the developer has to thoughtfully consider what they are doing. And if they're already thinking hard about what is going on, I'd say go ahead and refactor to make things more clear (and often reusable) instead of keeping track of comment updates along with every code change. Just my view though! However we try to explain things, there is always a danger than someone may misinterpret or misapply the idea. Hopefully between the blog entry and comments, we can minimize any problems. ;-)

  • # Posted By Brian Kotek | 6/5/08 10:12 AM

    Dan, not sure I follow. If a developer is looking at the code and sees the method call utility.convertPackageToPath(), are you saying you'd also want a comment there explaining *why* you want to convert the package to the path? To me this would seem to be going too far. Given the method name, as well as the context in which it is called (what happens with the converted path after this method call) I would expect that to suffice in most cases. For more complex cases, I did say that comments explaining the "why" can be very helpful. I just don't think this simple case would require that.

    With regard to the hint, again, I use hints primarily for API documentation generation. So it's somewhat difficult to give a "why" value to the hint since it wouldn't make much sense to someone looking at a PDF of the components and their methods and arguments. Maybe I'm missing what you're trying to say?

    I also hate the comments like "loop over the array" in front of a cfloop tag. It's completely pointless. And again, for complex if statements, once I go past two or three conditions, I pull it out into a method that explains what it is actually doing (i.e. isPackageReadyToSend()), which often covers the "why" as well. If the method name is clear, there's often no need to further explain why it is being used.

    Thanks.

  • # Posted By Ben Nadel | 6/5/08 10:15 AM

    @Brian,

    That is also a very valid point. I think this is such a sticky topic because really the best advice is "be a thorough programmer", and that's not advice that can be followed with ease :)

  • # Posted By Brian Kotek | 6/5/08 10:25 AM

    Very true, Ben. As with most things related to programming, concepts and best practices are unsettlingly nebulous. It's somewhat akin to saying "just do it elegantly and efficiently" hehe. Oh sure, that clears it up!

    Cheers.

  • # Posted By Ben Nadel | 6/5/08 10:34 AM

    @Brian,

    I also really like the idea of breaking out logic-based code into its own function with a descriptive name. Even on small things, I could see that being really useful.

  • # Posted By Jim Priest | 6/5/08 12:21 PM

    I was thinking a few days ago it would be cool if the CF tags accepted "hint=" as an attribute (or something similar).

    I'm doing a lot more 508 stuff and find as my tags become more verbose - tables with 'summary=', etc, I need to comment things less.

  • # Posted By Dan G. Switzer, II | 6/5/08 12:23 PM

    @Brian:

    Obviously you can't really use a "why" with a UDF hint. However, you can still use it on the line that calls UDF:

    // convert the package to directory path so we can find all the children components in that folder
    sPackageFolder = convertPackageToPath('sitemapping.components.services');

    While this example isn't the best, it shows off what I'm talking about.

    When I'm looking at code written by another developer or even code written by myself in the past, it's more helpful to know why the code was written in the first place, not just what the particular line is doing.

    I mean down the road, maybe there's no longer the need to read in all the components to find their children. When you're comments tell you *why* you're doing something, it makes it easier to update.

    The why's are extremely important in some cases, such as checking for dependencies that may or may not exist. Defining how they might exist and why we need to check for them is extremely important.

  • # Posted By Brian Kotek | 6/5/08 12:35 PM

    Dan I do understand what you're talking about and I agree in principle. As I had mentioned in the entry, using comments to specify why certain sections of code are being used is perfectly valid. I suppose I personally apply that at a more general level than you do though. So for this example of converting a package path, I wouldn't choose to add a comment to that since I think the name of the method call itself is sufficient to explain why it is being used. i.e., in my opinion, something like this doesn't really need a comment:

    targetFolder = fileUtils.convertPackageToPath(path);
    targetFolderFiles = fileUtils.getFilesInFolder(targetFolder);
    remoteService.sendFileCollection(targetFolderFiles);

    But something like this probably would:

    targetFolder = fileUtils.convertPackageToPath(path);
    targetFolderFiles = fileUtils.getFilesInFolder(targetFolder);

    // we have to alphabetize the files because the
    // remote service requires them to be in alphabetical order
    fileUtils.alphabetizeFileCollection(targetFolderFiles);

    remoteService.sendFileCollection(targetFolderFiles);

    So I'd add comments not only to specify why, but to denote information that is not (and really could not) be readily apparent from the code itself. But everyone has their preference of course! As long as the code is clear and easy to maintain, the comments are mainly just helpful extras. :-)

  • # Posted By charlie griefer | 6/5/08 3:30 PM

    heh... brian's just all about starting sh!tstorms this week :)

    i think what it comes down to is "comment when necessary". i know brian's point goes beyond that into refactoring code into more understandable (and reusable) components, and i agree with that.

    but for the most part... i think it's just apply comments when comments are justified. sean's 'coding standards' doc from a few years back addresses this as well. really no need for the following:

    <!--- loop from 1 to 10 --->
    <cfloop from="1" to="10" index="i">

    coincidentally enough, i came across a CFC yesterday that had about 12 cffunctions in it. each function had the following over it:

       <!---
       *************************************************************************************************************************************
          Plan Conflicts
       *************************************************************************************************************************************
       --->

    (that one was over the 'getPlanConflicts' function).

    while it's arguably pretty, i don't particularly need ascii art in my code. to be fair, a couple of them do include one liners that indicate what the function does... but yeah, i'd have put that into the "hint" attribute of the <cffunction> tag.

    comes down to this for me... use consistent and descriptive naming conventions. use the "hint" attribute. when the situation dictates, use type checking (if you're on CF8, turn this off in production). all of these things should really make the code fairly self-explanatory.

    when necessary, add a line or two to indicate what the code is meant to do. if you need more than 2 lines for a comment (as a general rule), it may be time to refactor.

  • # Posted By Patrick McElhaney | 6/9/08 10:17 PM

    I couldn't agree more. Comments in code are mostly unnecessary.

    I used to belong to the "use comments to tell the *why*" school. But lately I've realized *why* comments should go in version control commits.

  • # Posted By Tim Daly | 7/7/08 4:39 PM

    If you read the Mythical Man-Month you'll find an interesting story about
    a dinosaur stuck in a tarpit. Despite the ability to lift any single leg the
    dinosaur is hopelessly trapped. It's not the small things that matter, its
    the overall perspective.

    I believe you're confusing the ability to understand code "in the small"
    with understanding code "in the large". Just because I deeply understand
    the driver for my NIC card, it contributes almost nothing to my ability to
    understand the TCP/IP performance and functionality in Linux, even
    though the NIC card code plays a vital role.

    I agree that well written code blocks need little, if any, documentation.
    What IS needed is pervasive documentation. That is, Knuth's idea of
    literate programming. Documentation should be the point and the code
    should be a smaller, integral part of the literate document. I don't care
    if you've got clever, efficient code to do text substitution. I DO care
    why you need to do text substitution and how that fits into the overall
    understanding of the project.

    So, code doesn't need to be documented.
    Documentation should include code to implement the ideas.
    Literate programming is needed.

  • # Posted By Brian Kotek | 7/7/08 9:55 PM

    I certainly would not debate the need for good system documentation, be they database schemas, UML diagrams, or written documents. That is at the level where the question of "why" something is done or some architectural decision was made is critical.