Check for proper inline comment formatting

I'd additionally throw a warning if an inline comment isn't alone on a line, i.e.

  $foo = bar; // wrong

  // This is right.
  $foo = bar;

I know that this is not yet part of Drupal Coding Standards but this basically leads to better comments. Devs tend to leave 'cryptic' comments (like in the example above) squished until column 80 is reached if they are allowed to append a comment on a line.

Some inline comments in multi-line conditional statements in Drupal suffer from this:

if ($foo > $bar
  && $fee != $ber // what
  || $fuu < $bor // ever
  ) {...

// Instead of:

if ($foo > $bar
  // Do not include what.
  && $fee != $ber
  // Omit ever.
  || $fuu < $bor
  ) {...

btw: Coder_format already reformats such comments.

I have to dsiagree on this one. To start with, I've seen several people say "well, such-and-such a module is 4,000 lines long; that's just too big for me." Adding seprate lines for comments is only going to exacerbate that situation. Plus, there are times when a side-by-side comment is more appropriate, for example:

  if (some-condition) {
    ...
  }  /* end some-condition */

I do this all the time to keep nesting straight. As a matter of fact, I usually add the comment before I even put the code in the statement. Further, in this case, I much prefer the "/* */" form. Putting the comment before, IMO makes it less readable and harder to follow, especially in a multi-line IF as you described.

Another case for the side-by-side comment is in an array, for example:

  $_default_settings = array(
                             'drupal_section' => TRUE,
                               'kill_cron' => 0,                /* kill long-running cron */
                             'table_section' => TRUE,
                               'optimize_tables' => FALSE,      /* use OPTIMIZE to release overhead */
                             'sequences_section' => FALSE,   /* not on because not much interest */
                             'node_section' => FALSE,        /* not on because of expense */
                               'include_comment_count' => FALSE,     /* comment count on nodes */
                               'include_node_access' => FALSE,       /* node access summary */
                               'node_show_size' => 99999,       /* show nodes exceeding x KB */
                               'node_max_size' => 50,           /* warn if nodes exceeding x KB */
 ...

Which brings up the indentation comment. I do that here because the ones that are indented are sub-data for the ones that are not indented. This makes it easier to understand and follow, IMHO.

Just my 2 cents worth (you can keep the change).

Good examples. However, regarding your first example, the rule of thumb is to write functions and conditions in a size that do not need explanations for what ends where. If a function gets too big, separate it into multiple.
Also, I often see needless indents due to negated conditions:

function bar() {
  if ($foo) {
    ...
    // have many lines for doing something in here
    ...
  }
}

instead of just negating the condition, and thereby omitting needless indents:

function bar() {
  if (!$foo) {
    return;
  }
  ...
  // have many lines for doing something in here
  ...
}

Your second example would look even cleaner and more readable this way IMHO:

$_default_settings = array(
  'drupal_section' => TRUE,
  // Whether to kill long-running cron. Specified in seconds.
  'kill_cron' => 0,
  'table_section' => TRUE,
  // Whether to use OPTIMIZE to release overhead after invoking foo().
  'optimize_tables' => FALSE,
  // Sequences are disabled by default, because not of much interest.
  'sequences_section' => FALSE,
  // Nodes are disabled by default, because of expense.
  'node_section' => FALSE,
  // Whether to take amount of comments on a node into account.
  'include_comment_count' => FALSE,
  // Whether to generate a node access summary for all nodes.
  'include_node_access' => FALSE,
  // Maximum size limit in KB; nodes exceeding this size won't be included.
  'node_show_size' => 99999,
  // Warning treshold in KB; nodes exceeding this size will be listed.
  'node_max_size' => 50,
);

In this example, I have extended the existing comments to be more helpful for a developer -- which should be the main purpose of inline comments. Please note that all of above comments are smaller than 80 chars, and they could be extended to another inline comment line, if needed.

Continuing this debate from http://drupal.org/node/208767#comment-715951 ...

I think you need to look at the actual code, not just the formatting patch, to see what I'm talking about. For example, consider theme_update_report() from modules/update/update.report.inc:

    $row .= "<div class=\"info\">\n";
    if (!empty($project['extra'])) {
      $row .= '<div class="extra">'."\n";
      foreach ($project['extra'] as $key => $value) {
        $row .= '<div class="'. $value['class'] .'">';
        $row .= check_plain($value['label']) .': ';
        $row .= theme('placeholder', $value['data']);
        $row .= "</div>\n";
      }
      $row .= "</div>\n";  // extra div.
    }

    $row .= '<div class="includes">';
    sort($project['includes']);
    $row .= t('Includes: %includes', array('%includes' => implode(', ', $project['includes'])));
    $row .= "</div>\n";

    $row .= "</div>\n"; // info div.

I think it's entirely clear what // info div. and // extra div. are talking about, but it's helpful to see it inline, to know exactly which div you're referring to when you label what you're closing.

Compare with (just for the // info div part):

    $row .= '<div class="includes">';
    sort($project['includes']);
    $row .= t('Includes: %includes', array('%includes' => implode(', ', $project['includes'])));
    $row .= "</div>\n";
    // Close the info div.
    $row .= "</div>\n";

IMHO, that's *much* less clear...

Which brings me to my point: some of this stuff really is subjective and depends on the context. I don't think it's always automatically better to avoid inline comments, so I'd oppose an automated check that marked any inline comment as an error.

Not that my opinion means anything but I like inline comments on the same line as the statement they're describing, so I'm +1 dww. However, I think we should do a straw poll with the core contributors to see what they want.

@sun: I see your point, but must still disagree on the ending comments. When I am coding and know that there will be several lines inside, for example, a looping structure or if statement, I always write the ending statement immediately after writing the beginning statement - before any of the in between code. That way I always know I have the end there and don't end up with an "unexpected $end..."

I agree that a simple out should be coded first. It's just cleaner.

However, I have to disagree with your "more readable" example. To me it looks unnecessarily cluttered. Readability, like beauty, is in the eye of the beholder. However, I have largely gone to the two space indentation method - even on the example above (I think). I guess I'm just getting used to it, although I have seen examples (like arrays within arrays) where it is not a clear practice. Oh, and despite how it shows here, the comments are lined up in a monospaced font such as I use in development.

Angie originally created this issue because she didn't like some of my coding practices. since then I have not only started using Coder religiously but have evolved some of my practices because of it. Being the original "inspiration" for this request, I would like to just see it closed and we can all agree to disagree.

There are two coding practices mentioned above, that I feel pretty strongly about.

1. In comment #4 above, sun suggests that we use early returns to prevent "needless indents." if there are too many returns, or these returns are nested deeply, it's begging for bugs. Maintainers are more likely to add code to a single execution point, than one with multiple returns, especially if those returns are highly nested.
2. I personally like the short quick comment at the end of a line, and I don't "get" why these need to be full sentences (capitals and periods) or on separate lines. I also disagree in comment #4 above that the example is more readable on separate lines; the separate line example is less readable to me.

I think that we have a good set of coding standards that make our modules look-and-feel similar, so that a new programmer can feel comfortable working on them. I follow many of the coding rules, because there's value in that, but please, let's not get over-anxious in creating rules for rules sake. I also think that we need either some direction from above, or a reasonably strong consensus before adding standards like the ones discussed here.

@douggreen: very nicely said, thank you.

Re: 1. I think we'd all agree with sun when he says "write functions and conditions in a size that do not need explanations for what ends where". However, if it's difficult to split up a single function and it has to be larger than a single screen (say 30-40 lines), I think it's ok to have a simple test at the very beginning of a function (within the first 10 lines at most) to decide if you need to bail out, or immediately return a cached value, etc. And I do think that this can help keep the function more readable by not having the whole rest of the function nested another level deep. So, the specific example in #4 isn't necessarily a bad thing, but something where the return is more deeply nested, or with many return points peppered throughout a large function, would be. Otherwise, douggreen is right that a single return point at the end of the function is usually less error prone.

Re: 2. absolutely.

@dww, agreed on 1 also. I have exceptions to my one return rule, it's not inviolate, and you just outlined one of those exceptions. I also break this rule sometimes at the bottom of the function where there are two returns based on some conditional check.

Back to topic. Just created a patch against Panels that contained inline comments like these previously:

$items[] = array( // TODO Deprecated generalized ajax handler. Remove if at all possible.
  'path' => 'panels/ajax',
  'title' => t('ajax'),
  'callback' => 'panels_ajax_passthru',
  'callback arguments' => array('panels_ajax'),
  'access' => user_access('access content'),
  'type' => MENU_CALLBACK,
);

foreach ($obj as $key => $value) {
  $display->$key = $value;
    if (in_array($key, array('layout_settings', 'panel_settings', 'cache'))) { // unserialize important bits
      $display->$key = empty($display->$key) ? array() : unserialize($display->$key);
    }
  }
}

$f = 'pid, did, panel, type, subtype, configuration, cache, shown, access, visibility, position'; // doin it this way for readability
$q = "%d, %d, '%s', '%s', '%s', '%s', '%s', '%s', '%s', '%s', %d";

Completely misplaced comments like these can only happen if developers have no clue of writing clean inline comments. However, I agree with previous arguments that an inline comment on the same line is better for rather short notes, i.e.

$row .= "</div>\n"; // Close info div.

So, taking Angie's initial list into account - can we agree on these relaxed rules?

Comments should:

- Begin with a capital letter and end with a period.
- Be indented the same amount of space as their surrounding code.
- Only be specified with //, never /* */ or #.
- Be placed on a separate line if they are longer than 30 chars, following an opening parenthesis/curly brace, or the remarked line is already longer than 80 chars.

Those rules are what I follow, except I tend to use the /* ... */ form on the end of lines.

Certainly, TODO'd should always be on a separate line.

Completely misplaced comments like these can only happen if developers have no clue of writing clean inline comments.

Well gee whillikers, there's nothin I like more than being trotted out as an example of what not to do. But I'll take this in a positive light and agree that to an extent, up until sun hit the panels issue queue with that patch, I didn't have a clue as to conventions around in-line comments. I generally tried to keep fully-formed & appropriately punctuated thoughts on the preceding line, and leave same-line comments for short notes only (unless the line is overly long). But, as the excerpts from the old version of panels.module in #11 indicate, I was also pretty lackadaisical about that at times. So the conventions sun proposes in #11 seem generally OK. But that's quite different from my endorsing the idea that it should be in Coder and automatically marked as an error when not adhered to. As dww points out in #5, this is a subjective thing. I'll take that one step further: it seems to me that by limiting same-line comments to 30 characters, we're deciding that it's OK for a complex & subjective cognitive judgment (whether or not a particular string of words is or is not 'quick', 'cryptic', or any of the other adjectives that have been used in this thread) to be governed by a character count mechanism that, to me, seems crudely simple by comparison.

I'm a little peeved here because I already did commit the patch that sun referrs to in #11 under the assumption that this WAS a coding standard (although sun didn't mean to move every single same-line comment to a previous-line comment, so it's kind of moot), and I was happy to submit to the standard if it's something the community had agreed upon. Seeing that it isn't, however, my basic reaction is this: come on, people! They're comments! I'll admit, the idea of a coding standard for comments (when it has nothing to do with doxygen) personally makes me chafe in a way that reminds me of the discussions about instituting school uniforms when I was in high school. More importantly, however, it strikes me that while there are plenty of things that computer parsers are going to be better and more consisting at judging than humans are, we're really barking up the wrong tree if we think that a brute-force character counting mechanism will, on average, prove more reliable than letting developers sort this out on there own.

Sam, I don't recall Sun ever using me as a bad example, but Angie has on several occasions (and, well, I used her once myself). As Derek said, some of these warnings (not errors) are somewhat subjective, and you are free to ignore them. I have a few modules where I ignore things in 5.x simply because they are much easier to fix in 6.x. Yes, a 30 character limit may be a bit arbitrary, but the line needs to be drawn somewhere. If you don't like 30, suggest another number, or just ignore the warning.

@webchick: I usually comment out code with // so that I can put them back in one at a time, and not necessarily in order. But I wasn't suggesting the /* format be accepted. It's just a quirk of mine that I'm working on changing. (For example: $output .= '</div>' /* mymodule-some-class */)

Please note that this is not only about Coder's review rules. It hopefully results in

• a clarification to the Comments section in Coding Standards.
• a simple logic for corresponding new Coder code review rules.
• a simple logic to update the coder_format script (which implements a custom logic currently).

bump.

We currently have some standards for inline comments at
http://drupal.org/node/1354#inline

And the general comment standards above also apply (such as 80-character lines).

Apparently, Coder is not currently checking for 80-character limits in comments. It should.

Drupal code sniffer now has an inline comment check:
http://drupal.org/project/drupalcs

I will look at the latest standards. I thought the 80 character limit only applied to the first line of block comments.

The standards for in-line comments are on http://drupal.org/node/1354, and yes they all are supposed to be under 80 characters.

Coder 7.x is frozen now and will not receive updates. Coder 8.x-2.x can be used to check code for any Drupal version, Coder 8.x-2.x also supports the phpcbf command to automatically fix conding standard errors. Please check if this issue is still relevant and reopen against that version if necessary.