Comments on Doc Comments

I was reviewing some VBA code over at Code Review Stack Exchange the other day and it got me to thinking about comments. The code wasn’t too bad, but there were so many comments in the code that, well…

I just read over this again and I think the fact that I mention the comments so often really highlights the issue. I barely even looked at the code because I was too distracted by all the comments.

To be clear, the comments I’m referring to were document style “header” comments and left over cruft from a pseudo-code first style of development. Comments like this were before each and every method in the module.

'/======================================================================================================================================================
'/  Author:  Zak Armstrong
'/  Email:   zak.armstrong@company.co.uk
'/  Date:    12/August/2015
'/  Version: 0.2
'/
'/  Is Called By:   None
'/
'/  Calls:          Open_Workbooks
'/                  Initialise_Worksheets
'/                  Initialise_Collections_And_MetricHeadings
'/                  Initialise_Providers_And_Advisers
'/                  Insert_arrAscentric_LifeCo_Column
'/
'/                  Sheet_Data_To_Array
'/                  Filter_Sheet_Arrays
'/                  Aggregate_Sheet_Data
'/
'/                  Allocate_Business
'/
'/                  Print_Adviser_Report
'/                  Print_Provider_Report
'/
'/  Description:    All Company Wealth Business is contained in the Subsheet. This macro produces adviser totals for business (assets and fees) in the previous year
'/                  (month by month breakdown) by aggregating the subsheet into one giant table and then assigning each piece of business to an adviser, a Month and a business type.
'/                  The report can then be easily configured for any desired outputs (E.G. by adviser, by provider, by type of business)
'/
'/  Change Log:     | Author            | Date          | Description of Changes
'/                  -----------------------------------------------------------------------------------------------------------------------------------
'/                  |Zak armstrong      | 12.08.2015    | Started v0.2 from scratch. Major difference: now aggregating all subsheet data first and
'/                  |                   |               | only then allocating each piece of business.
'/                  |                   |               |
'/                  |                   | 14.08.2015    | "Finished" Writing Macro. Had succesful test run.
'/                  |                   |               |
'/                  |                   | 17.08.2015    | Reviewed all code, replaced a lot of hard-coded references (e.g. specific position numbers) with
'/                  |                   |               | dynamically generated ones. Re-structured main allocation sub.
'/                  |                   |               |
'/                  |                   | 18.08.2015    | Finished Writing Format_Adviser_Report, Format_Provider_Report, restructured macros from 1 module to 3.
'/                  |                   |               |
'/                  |                   | 19.08.2015    | Double-Checked Code, Slight re-structuring/obfuscation before posting to Code Review (Stack Exchange)
'/                  |                   |               |
'/======================================================================================================================================================

The important pieces got lost because there was just too much there to sift through. The second problem is that, because there was one of these at the top of every method, it was hard to see the code from 30,000 feet up and get an overview of what it does. That’s a problem that could just as easily show up in C#, and I see it too often.

    public interface IMergeView
    {
        /// <summary>
        /// Returns a boolean value indicating if the OKButton is enabled.
        /// </summary>
        /// <returns>Boolean</returns>
        /// <example><code>view.OkButtonEnabled = true;</code></example>
        bool OkButtonEnabled { get; set; }

        /// <summary>
        /// List of Strings that populate the source selector.
        /// </summary>
        /// <returns><see>
        ///         <cref>IList{string}</cref>
        ///     </see>
        /// </returns>
        IList<string> SourceSelectorData { get; set; }

        /// <summary>
        /// List of Strings that populate the destination selector.
        /// </summary>
        /// <returns><see>
        ///         <cref>IList{string}</cref>
        ///     </see>
        /// </returns>
        IList<string> DestinationSelectorData { get; set; }

XML doc comments are great. I love the information that they provide to intellisense, but far too often, they just repeat what the code already says. When they do that, they’re just getting in the way, breaking our concentration. The example above provides no additional information. We already know what they do because they have meaningful names. Adding this extra cruft just gets in our way.

I’m not trashing on doc comments though. They really are a great thing to have. We just need to be sure that we’re using them for the right things in the right places. For example, this gem was buried in that VBA header comment.

'/  Description:    All Company Wealth Business is contained in the Subsheet. This macro produces adviser totals for business (assets and fees) in the previous year
'/                  (month by month breakdown) by aggregating the subsheet into one giant table and then assigning each piece of business to an adviser, a Month and a business type.
'/                  The report can then be easily configured for any desired outputs (E.G. by adviser, by provider, by type of business)

This is a good header comment. It says why and how instead of who and what. It provides valuable information to the maintainer of the code. Unfortunately, it was buried inside this monolithic comment block at the top of the method. This should have been the entire doc comment. This and nothing more. Heck, I might have even put this into a Description Attribute.

So what about our C# XML doc comments? I asked some developer friends of mine for their perspective on doc comments and I got some really interesting answers.

This is one of the things I do still like about Objective-C over Swift. Because you only really need doc comments in/on the public stuff, and in Objective-C, the public stuff is in the .h. And your actual implementation is in the .m.

Nick Griffith – author of importBlogKit

[It’s] best is to code against interfaces, and xml-doc the interface, leaving the code uncluttered.

Mathieu Guindon – Co-Owner/Developer of Rubberduck

I think this is wonderful advice. Documenting the interface keeps all of the clutter out of the implementation. We haven’t really talked about what to document yet though. We briefly touched on the fact that I don’t think doc comments should simply repeat the code, so what should they say? Nick had this gem to add.

If there is any weird behavior, you can document it. And if you don’t document any weird behavior, you have documented that there shouldn’t be any weird behavior.

Right! We document anything that isn’t obvious. Just like any other comment in our code. We don’t comment the obvious in our implementation, we shouldn’t comment the obvious in our documentation either. Keep it simple, keep it clean. Here’s an example of what I consider to be good doc comments.

    /// <summary>
    /// Stores user name and password credentials.
    /// </summary>
    /// <remarks>
    /// Do no use internally. For COM Interop only. Use <see cref="SecureCredentials"/> instead./>
    /// </remarks>
    public class Credentials : CredentialsBase<string>
    {
        /// <exception cref="NullReferenceException">If either argument is null, an exception is thrown.</exception>
        public Credentials(string username, string password)
            :base(username, password)
        { }
    }

    /// <summary>
    /// Securely stores user name and password credentials.
    /// </summary>
    public class SecureCredentials : CredentialsBase<SecureString>
    {
        /// <exception cref="NullReferenceException">If either argument is null, an exception is thrown.</exception>
        public SecureCredentials(string username, SecureString password)
            : base(username, password)
        { }
    }

It’s minimal. It gives you a high level overview at the class level and only documents the “weird” parts. In this case, one class isn’t to be used internally, but exists only to expose to COM clients. That’s important information to have when you’re looking at available options in your intellisense.

Another thing to look out for is assumptions about the state of the object when a particular method is called, or the range of it’s parameters. Possible exceptions and runtime errors are also a great thing to see. It lets people consuming your API know what exceptional situations they might need to handle. In .Net, there’s even a comment tag specifically for exceptions.

Before I ramble on too much, let’s sum this up.

  • Be concise. If you ramble too much, nobody listens. (Take this blog post for instance.)
  • Don’t repeat what the code already says.
  • Document the why and how, not the who and what.
  • Document the unordinary and potentially surprising.
  • Use a form of documentation that will show up in intellisense. (i.e. don’t use Doxygen for .Net code. Sure, the documents are pretty, but it doesn’t show up in intellisense.)
  • Stay out of the way of your code. Document the public interface, not the implementation. If your documentation is getting in the way, consider using included document files instead.
  • Document possible exceptional behavior that may need to be handled by the client.
Advertisements

, , , , , ,

  1. Leave a comment

Leave a Reply

Please log in using one of these methods to post your comment:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s

%d bloggers like this: