Larry Osterman's WebLog

Confessions of an Old Fogey
Blog - Title

What does style look like, part 4

What does style look like, part 4

Rate This
  • Comments 33
Continuing the discussion of "style"..

Yesterday, I showed the code reformatted into "BSD" style format, which looks like:

#include "list.h"
main(C cArg, SZ rgszArg[])
{
    I iNode;
    I cNodes = atoi(rgszArg[1]);
    I cNodesToSkip = atoi(rgszArg[2]);
    PNODE pnodeT;
    PNODE pnodeCur;
    InitNodes(cNodes);
    for (iNode = 2, pnodeCur = PnodeNew(1); iNode <= cNodes ; iNode++)
    {
        pnodeT = PnodeNew(iNode);
        InsertNext(pnodeCur, pnodeT);
        pnodeCur = pnodeT;
    }
    while (pnodeCur != PnodeNext(x))
    {
        for (iNode = 1; iNode < cNodesToSkip ; iNode++)
        {
            pnodeCur = PnodeNext(pnodeCur);
        }
        FreeNode(PnodeDeleteNext(pnodeCur));
    }
    printf("%d\n", Item(nodeCur));
}

I chose one of the variants that I personally find most attractive, any one of them could work. The thing about this example is that there are no comments.  That's reasonable given that it's an example from a textbook, and as such the words in the textbook that accompany the example suffice as the documentation.  But all code begs to be documented, so lets see what happens.

When you look at documenting a function, there are a couple of things to keep in mind when considering style (there are other important things, like the relevance of the comments, etc, but this post's about style).

The first question that comes to mind, almost immediately is "What commenting form should you use?"  For C/C++, there are a number of options.

First off, C and C++ support two different styles of comment.  The first is the traditional  /* */ style of comments.  And there's the C++ style // comment.  Each of the two comments have different flavors.

When I was in college, I loved using some of the options that /* */ comments enabled.  My code was just full of things like:

         :
         :
    }

    /*************************************************************/
    /*                                                           */
    /*                                                           */
    /*   Loop through the nodes looking for the current node.    */
    /*                                                           */
    /*                                                           */
    /*************************************************************/
    while (pnodeCur != PnodeNext(x))
    {
        for (iNode = 1; iNode < cNodesToSkip ; iNode++)
         :
         :
 

I also had this thing about putting a comment on every line, especially for assignments.  And every comment lined up on column 40 if possible.

         :
         :
    }

    /*************************************************************/
    /*                                                           */
    /*                                                           */
    /*   Loop through the nodes looking for the current node.    */
    /*                                                           */
    /*                                                           */
    /*************************************************************/
    while (pnodeCur != PnodeNext(x))    // Keep looping until PnodeNext == pnodeCur
    {
        for (iNode = 1; iNode < cNodesToSkip ; iNode++) // Skip through cNodesToSkip nodes
         :                             // A comment for this line.
         :                             // A comment for that line.
 

When I look back at my old code, the code screams "1980's", just like the hairdos of "A Flock of Seagulls" does.  But there are times when big honking block comments like that are appropriate, like when you're pointing out something that's REALLY important - like when you're about to do something that would cause Raymond Chen to swear like a sailor when he runs into your application.

Also, while I'm on the subject of comments on every line.  There ARE situations where you want a comment on every line.  For example, if you're ever forced to write assembly language (for any assembly language), you really MUST comment every line.  It's far too easy to forget which registers are supposed to contain what contents, so adding comments on every line is essential to understanding the code.

/* */ comments have the disadvantage that they take up a fair amount of space.  This means that they increase the effective line length.  // comments don't have this problem.  On the other hand, /* */ comments stand out more.

Personally I feel that both comment styles should be used.  One fairly effective form that I've seen is:

         :
         :
    }
    /*
     * Loop through the nodes until you hit the current node.
     */
    while (pnodeCur != PnodeNext(x))
    {
        for (iNode = 1; iNode < cNodesToSkip ; iNode++)
        {
            pnodeCur = PnodeNext(pnodeCur);  // Skip to the next node.
        }
         :
         :
 

Of course that's not very different from:

         :
         :
    }

    // Loop through the nodes until you hit the current node.
    while (pnodeCur != PnodeNext(x))
    {
        for (iNode = 1; iNode < cNodesToSkip ; iNode++)
        {
            pnodeCur = PnodeNext(pnodeCur);  // Skip to the next node.
        }
         :
         :
 

On the other hand, the first form (/**/) stands out more, simply because it has more white space.  You can achieve the same whitespace effect with // comments:

         :
         :
    }
    //
    // Loop through the nodes until you hit the current node.
    //
    while (pnodeCur != PnodeNext(x))
    {
        for (iNode = 1; iNode < cNodesToSkip ; iNode++)
        {
            pnodeCur = PnodeNext(pnodeCur);  // Skip to the next node.
        }
         :
         :
Another aspect of commenting style is the effective line length.  When I learned to program, I learned on 80 character wide terminals, which imposed a fairly strict limit on the number of columns in the code - anything longer than 80 characters was not OK.  Nowadays, that isn't as important, but it's still important to limit source code lines to a reasonable number, somewhere around 100 works for me, but of course your experience might be different.

The other major thing about comments is the content of the comments.  Comments can be the bane of a maintainers existence or their savior, depending on the comment.

I don't know how many times I've come across the following and cringed:

    //
    // Increment i.
    //
    i = i + 1;
 

Yes, the line's commented, but how useful is the comment?  Especially since it's a block comment (block comments have more importance than in-line comments by virtue of the amount of real estate they occupy - the very size of the comment gives it weight.  In general, you want to use in-line comments for little things and block comments for the big stuff.  I almost never use in-line comments, I prefer to use block comments at the appropriate locations and let the code speak for itself.

I've mentioned white space as an aspect of style before, but this is the time to bring it up.  Consider the example routine with some white space added to stretch the code out a bit:

#include "list.h"
main(C cArg, SZ rgszArg[])
{
    I iNode;
    I cNodes = atoi(rgszArg[1]);
    I cNodesToSkip = atoi(rgszArg[2]);
    PNODE pnodeT;
    PNODE pnodeCur;

    InitNodes(cNodes);

    for (iNode = 2, pnodeCur = PnodeNew(1); iNode <= cNodes ; iNode++)
    {
        pnodeT = PnodeNew(iNode);
        InsertNext(pnodeCur, pnodeT);
        pnodeCur = pnodeT;
    }

    while (pnodeCur != PnodeNext(x))
    {
        for (iNode = 1; iNode < cNodesToSkip ; iNode++)
        {
            pnodeCur = PnodeNext(pnodeCur);
        }
        FreeNode(PnodeDeleteNext(pnodeCur));
    }

    printf("%d\n", Item(nodeCur));
}

It's not a major change, but to me, by just inserting 4 empty lines, the code's been made clearer. 

The other aspect of white space is intra-expression white space.  This is one of the aspects of style that tends to get religious.  I've seen people who do:

    for ( iNode = 2 , pnodeCur = PnodeNew( 1 ) ; iNode <= cNodes ; iNode ++ )
And others who prefer:

    for (iNode=2,pnodeCur=PnodeNew(1);iNode<=cNodes;iNode++)
Or:

    for (iNode=2, pnodeCur=PnodeNew(1); iNode<=cNodes; iNode++)
Or:

    for ( iNode=2, pnodeCur=PnodeNew(1) ; iNode<=cNodes ; iNode++ )
The reality is that it doesn't really matter which form you use, as long as you're consistent.

Lets see what happens to the sample routine if you insert some block comments...

#include "list.h"
main(C cArg, SZ rgszArg[])
{
    I iNode;
    I cNodes = atoi(rgszArg[1]);
    I cNodesToSkip = atoi(rgszArg[2]);
    PNODE pnodeT;
    PNODE pnodeCur;

    InitNodes(cNodes);

    //
    // Create a list of cNodes nodes. 
    // 
    for (iNode = 2, pnodeCur = PnodeNew(1); iNode <= cNodes ; iNode++)
    {
        pnodeT = PnodeNew(iNode);
        InsertNext(pnodeCur, pnodeT);
        pnodeCur = pnodeT;
    }

    //
    // Walk the list of nodes, freeing the node that occurs at every cNodesToSkip nodes in the list.
    //
    while (pnodeCur != PnodeNext(x))
    {
        for (iNode = 1; iNode < cNodesToSkip ; iNode++)
        {
            pnodeCur = PnodeNext(pnodeCur);
        }
        FreeNode(PnodeDeleteNext(pnodeCur));
    }

    //
    // Print out the value of the current node.
    //
    printf("%d\n", Item(nodeCur));
}

To me, that's a huge improvement.  By stretching out the code and adding some comments, it's already starting to look better.

Again, just for grins, here's how it would look in my "1980's style":

#include "list.h"
main(C cArg, SZ rgszArg[])
{
    I iNode;                              // Node index.
    I cNodes = atoi(rgszArg[1]);          // Set the number of nodes
    I cNodesToSkip = atoi(rgszArg[2]);    // and the nodes to skip.
    PNODE pnodeT;                         // Declare a temporary node.
    PNODE pnodeCur;                       // And the current node.

    InitNodes(cNodes);                    // Initialize the nodes database
                                          // with cNodes elements

    /*************************************************************/
    /*                                                           */
    /*                                                           */
    /*               Create a list of cNodes nodes.              */
    /*                                                           */
    /*                                                           */
    /*************************************************************/
    for (iNode = 2, pnodeCur = PnodeNew(1); iNode <= cNodes ; iNode++)
    {
        pnodeT = PnodeNew(iNode);        // Allocate a new node.
        InsertNext(pnodeCur, pnodeT);    // Insert it after the current node
        pnodeCur = pnodeT;               // Current = New.
    }

    /*************************************************************/
    /*                                                           */
    /*                                                           */
    /*   Walk the list of nodes, freeing the node that occurs    */
    /*   at every cNodesToSkip nodes in the list.                */
    /*                                                           */
    /*                                                           */
    /*************************************************************/
    while (pnodeCur != PnodeNext(x))    // While not at the current node
    {
        for (iNode = 1; iNode < cNodesToSkip ; iNode++)
        {
            pnodeCur = PnodeNext(pnodeCur); // Current = current->Next
        }
        FreeNode(PnodeDeleteNext(pnodeCur)); // Free current->Next
    }

    /*************************************************************/
    /*                                                           */
    /*                                                           */
    /*          Print out the value of the current node.         */
    /*                                                           */
    /*                                                           */
    /*************************************************************/
    printf("%d\n", Item(nodeCur));
}

Yech.

Tomorrow: function and file headers.