Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Comments should:
- Begin with a capital letter and end with a period (or a colon, if it's a list, but those are weird and I don't like them :P).
- Be indented the same amount of space as their surrounding code.
- Only be specified with //, never /* */ or # (Coder module might check for this already.)
Comments
Comment #1
sunI'd additionally throw a warning if an inline comment isn't alone on a line, i.e.
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:
btw: Coder_format already reformats such comments.
Comment #2
webchick+1 for sun's suggestion. There are very few instances where I'd consider side-by-side comments more legible than "top-down" and for those instances, I can just ignore Coder's warnings. ;)
Comment #3
NancyDruI 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:
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:
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).
Comment #4
sunGood 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:
instead of just negating the condition, and thereby omitting needless indents:
Your second example would look even cleaner and more readable this way IMHO:
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.
Comment #5
dwwContinuing 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:
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):
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.
Comment #6
ezyang CreditAttribution: ezyang commentedNot 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.
Comment #7
NancyDru@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.
Comment #8
douggreen CreditAttribution: douggreen commentedThere are two coding practices mentioned above, that I feel pretty strongly about.
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.
Comment #9
dww@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.
Comment #10
douggreen CreditAttribution: douggreen commented@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.
Comment #11
sunBack to topic. Just created a patch against Panels that contained inline comments like these previously:
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.
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.
Comment #12
NancyDruThose 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.
Comment #13
sdboyer CreditAttribution: sdboyer commentedWell 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.
Comment #14
webchick-1 to inline comments denoted by block syntax, such as:
Because if I want to comment out your buggy function to see if it's causing my headaches, I can't just do:
I have to do:
...which is a royal pain in the butt. :P
Comment #15
NancyDruSam, 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 */
)Comment #16
sunPlease note that this is not only about Coder's review rules. It hopefully results in
Comment #17
jhodgdonbump.
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.
Comment #18
klausiDrupal code sniffer now has an inline comment check:
http://drupal.org/project/drupalcs
Comment #19
douggreen CreditAttribution: douggreen commentedI will look at the latest standards. I thought the 80 character limit only applied to the first line of block comments.
Comment #20
jhodgdonThe standards for in-line comments are on http://drupal.org/node/1354, and yes they all are supposed to be under 80 characters.
Comment #21
klausiCoder 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.