Wednesday, January 25, 2012

Coding Good Comments: Section Titles

There are many kinds of comments inside program code, one of them being Section Titles: Comments that introduce the reader to a block of code with the primary purpose being to answer the question "Is this the code we're looking for?"

Folks who hate comments might say: "Different sections of code should appear in separate functions or procedures, with the section title becoming the procedure name."

In the real world, however,

  • not every section becomes a procedure,

  • section title comments tend to be longer and more descriptive than procedure names, and

  • applications with extremely high ratios of call-return-interfaces-to-lines-of-worker-code may become hard to understand and follow.
In other words, every real-world program can use section title comments, and this article is about writing good ones.

Example 1

Here's an example of a not-very-good section title; it's not good because it is vague, telling us only that the section involves sample sets rather than some other commonly-used data objects used in the application:
------------------------------------------------------------------------
-- Perform some standard sample set updates as required.

That section title is not good because the section itself violates the rule for high cohesion as the following nested subsection titles show:
   ------------------------------------------------------------------------------------------------------------------
   -- Update 1: Fix rroad_sampling_options.rroad_monitor_sample_loop_connection_number if necessary.

   ------------------------------------------------------------------------------------------------------------------
   -- Update 2: Update the last rroad_sample_set.sample_finished_at if necessary.

Each of the subsection titles is pretty good, but neither has much to do with the other except the fact they both mention sample sets... hence the vague section title. The two subsections appear together because they can share one FOR loop; perhaps this is an example of coincidental cohesion (the worst), perhaps it is communicational cohesion (the second best, when parts of a module are grouped because they operate on the same data), or somewhere in between, but it is certainly not functional cohesion (the best): when parts of a module are grouped because they all contribute to a single well-defined task of the module.

In this case, the vague outer section title is as good as it's going to get without changes to the actual code... but maybe it's not so bad after all: the vagueness itself can be taken as a warning that "coincidental cohesion lurks within".

In other words, the code ain't perfect so neither are the section titles.

Example 2

Here's a section title that's pretty specific, but not exactly helpful:
------------------------------------------------------------------------
-- Loop through each database that is trying to connect past the connection timeout interval.

Instead of describing what the code's doing, it's concentrating on the how: "loop through each database". The first line of code is a FOR loop so nobody needs to know the "how", it's the "what" of the section that should be described in the title.

Here's how the section title was revised:
------------------------------------------------------------------------
-- Stop sampling for connection timeouts.

Much better: "stop sampling" is a big deal in the world of database monitoring, and it's the whole reason this code section exists yet it wasn't even mentioned in the original title.

Example 3

Here's a section title which confuses the "what" and "how" for a long section of code:
------------------------------------------------------------------------------------
-- Process Alert #1: Loop through each database that...
--     should be sampled,
--     or has timed out, 
--     and has not had a recent successful sample.

The word "process" is unforgivably vague since it could mean "issue", "clear" or "cancel", or even "edit", or some combination of all of them; in this case the action is "issue". Also, "Alert #1" is probably appropriate only for someone already familiar with Foxhound, someone who knows that Alert #1 is "Database Unresponsive".

Here's the revision:
------------------------------------------------------------------------------------
-- Issue Alert #1: Database Unresponsive for each database that...
--     should be sampled,
--     or has timed out, 
--     and has not had a recent successful sample.

Example 4

Here is yet another vague "Loop through" section title on a FOR loop exhibiting lower-than-functional cohesion, together with the subsection titles:
------------------------------------------------------------------------
-- Loop through each database that...
--     should be sampled but isn't,  
--     has timed out and should be retried, or
--     still has user_request = 'Stop Sampling' even though sampling has stopped.

   ------------------------------------------------------------------------
   -- Exit immediately if Foxhound is shutting down.

   ------------------------------------------------------------------------
   -- Get the Sample Schedule settings for this moment in time.
   -- This information will be used in later sections.

   ------------------------------------------------------------------------
   -- Start sampling again after sample loop connection number was dropped.

   ------------------------------------------------------------------------
   -- Start sampling for timeout retry.

   ------------------------------------------------------------------------
   -- Check and reset user_request if sampling has already been stopped.

   ------------------------------------------------------------------------
   -- Stop sampling if required to by the sample schedule.

In this case the associated code was changed and enhanced to provide new features, and the section and subsection titles were changed as well.

The biggest change, however, was to the outer section title to make it clear that the subsections exhibit lower-than-functional cohesion:
------------------------------------------------------------------------
-- Start or stop sample sessions as required.

   ------------------------------------------------------------------------
   -- Exit immediately if Foxhound is shutting down.

   ------------------------------------------------------------------------
   -- Get the Sample Schedule settings for this moment in time.
   -- This information will be used in later sections.

   ------------------------------------------------------------------------
   -- Start sampling each database that should be sampled but isn't.

   ------------------------------------------------------------------------
   -- Start sampling if the sample schedule requires it.

   ------------------------------------------------------------------------
   -- Start sampling for timeout retry.

   ------------------------------------------------------------------------
   -- Check and reset user_request and schedule_request if sampling has already been stopped.

   ------------------------------------------------------------------------
   -- Stop sampling if required to by the sample schedule.

Also, the outer section title was stripped of its point-form overview of subsection descriptions: sometimes repetition helps but not here; the subsection titles are all that's needed.

Note that one of the lines above isn't really a section title, it's a warning to the reader:
-- This information will be used in later sections.

The Bottom Line

The Number One Reason, perhaps the only reason for Section Title Comments is to answer the question: "Is this the code we're looking for?"


No comments: