[webkit-dev] coding style and comments

[Maciej Stachowiak]

On Jan 31, 2011, at 11:01 AM, Peter Kasting wrote:

> On Sun, Jan 30, 2011 at 4:47 PM, Maciej Stachowiak <mjs at apple.com> wrote:
> Well, I didn't mean to pick on the authors of this file. This is the impression I get from a lot of code that some call "well-commented", by which they mean "lots of comments". 
> 
> I agree that the comments you pointed out are pretty unhelpful.  I tried to emphasize already that silly comments that just restate the next line of code are not at all what I mean by "well commented", and that what I am interested in are comments about subtle but crucial details (e.g. complex ownership rules) or comments that sum up a huge swath of source code (e.g. a class-level comment that covers the critical high-level points the class is responsible for).

I think people who favor comments tend to produce a lot of exactly this kind of comment. Except in some cases its verbose multiline comments that exceed the number of actual lines of code. Here's some more files with many comments that are better deleted or replaced with refactoring:

http://trac.webkit.org/browser/trunk/Source/WebCore/platform/image-decoders/ImageDecoder.h
http://trac.webkit.org/browser/trunk/Source/WebCore/platform/image-decoders/ImageDecoder.cpp

To pick just one specific example:

   // ICOs always begin with a 2-byte 0 followed by a 2-byte 1.
   // CURs begin with 2-byte 0 followed by 2-byte 2.
   if (!memcmp(contents, "\x00\x00\x01\x00", 4) || !memcmp(contents, "\x00\x00\x02\x00", 4))

This would be more readable if the comments were deleted and the memcmps were replaced with calls to inline functions named hasICOMagicNumber/hasCURMagicNumber or the like.

> I honestly don't think there is much disagreement about what kinds of comments are unhelpful.  I think the disagreement here comes from past experience, where some people have mostly experienced low-quality and out-of-date comments and are justifiably uninterested in repeating that, and others have been helped by high-quality comments in complex codebases and want to see more of that in WebKit.

Well, I think a "comments are good" attitude can result in lots of low-quality comments, instead of reserving comments only for cases where they are high in value.

> It seems like the best rule of thumb would be that reviewers should look at any added comments and judge whether the comments are really valuable.  I don't think we need to (or should) globally frown on comments -- just on bad comments.

I would go further than that. Even if a comment is valuable, the reviewer (and the patch author) should think about whether what it says could be expressed in source code instead, with some refactoring. Comments should be a last resort for making code clear. I do agree that they have their place, but I think that place is fairly rare.

Regards,
Maciej

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-dev/attachments/20110131/cb36d465/attachment.html>