[webkit-dev] coding style and comments

Sun Jan 30 16:26:11 PST 2011

I'll go the other way and point out some example of comments that I think are poor.

///////////////////////////////////////////////////////////////////////////////
// BlobResourceSynchronousLoader
<http://trac.webkit.org/browser/trunk/WebCore/platform/network/BlobResourceHandle.cpp?rev=69610#L72>

    This two-line block comment is wasteful. It just states that a class declaration is coming. The class does not need a herald to announce it.

// We cannot handle the size that is more than maximum integer.
<http://trac.webkit.org/browser/trunk/WebCore/platform/network/BlobResourceHandle.cpp?rev=69610#L101>

    If it's not sufficiently clear what the code below is doing it, factor it out into a named function. Also, why use the mystery constant 0x7fffffff and describe it in a comment as "maximum integer" when there's a perfectly good symbolic MAXINT constant?

// Read all the data.
<http://trac.webkit.org/browser/trunk/WebCore/platform/network/BlobResourceHandle.cpp?rev=69610#L110>

    Classic "what not why" comment. If it's not clear enough that the two lines below read all the data, refactor to make the code itself clear.

///////////////////////////////////////////////////////////////////////////////
// BlobResourceHandle
<http://trac.webkit.org/browser/trunk/WebCore/platform/network/BlobResourceHandle.cpp?rev=69610#L130>

    Another "here comes a class declaration!" comment.

// static
<http://trac.webkit.org/browser/trunk/WebCore/platform/network/BlobResourceHandle.cpp?rev=69610#L133>

   Repeats what's in the header and could very easily go out of sync.

// We need to take a ref.
<http://trac.webkit.org/browser/trunk/WebCore/platform/network/BlobResourceHandle.cpp?rev=69610#L162>

    This comment doesn't explain either what the code is doing or why. 

// Do not continue if the request is aborted or an error occurs.
<http://trac.webkit.org/browser/trunk/WebCore/platform/network/BlobResourceHandle.cpp?rev=69610#L194>

    The very next lines of code are "if (m_aborted || m_errorCode) return;" The comment doesn't add anything, just makes the code more verbose.

// If the blob data is not found, fail now.
<http://trac.webkit.org/browser/trunk/WebCore/platform/network/BlobResourceHandle.cpp?rev=69610#L198>

    Redundant with immediately following code.

// Parse the "Range" header we care about.
<http://trac.webkit.org/browser/trunk/WebCore/platform/network/BlobResourceHandle.cpp?rev=69610#L206>

    This comment is not only redundant, it's taunting me! It implies there is a special reason to care about "Range" but doesn't tell me what it is.

Most of the other comments in this file are along these same lines. They reduce code density without actually adding information.

Regards,
Maciej

On Jan 27, 2011, at 4:50 PM, Dirk Pranke wrote:

> Two other ways to potentially answer these questions:
> 
> 1) What are files that you all think *are* well commented? (pointing
> at files that aren't probably isn't that useful, and is certainly
> meaner ;).
> 
> 2) One file over in that same directory, is a "filesystem.py" file
> that I wrote (most of). I think it's pretty well commented, by python
> standards, but I'm curious what others think. I give you permission to
> publicly bash me ahead of time ;)
> 
> I will also note, as a general thought, that comments should not be
> used as documentation where tests can be used instead. Similarly,
> people often under-document tests and make them very hard to maintain.
> 
> Lastly, I volunteer to take whatever wisdom is offered up on this
> thread and aggregate it onto the Wiki ...
> 
> -- Dirk
> 
> On Thu, Jan 27, 2011 at 4:38 PM, Dirk Pranke <dpranke at chromium.org> wrote:
>> I agree with all of the sentiments below, except for possibly one or two cases.
>> 
>> Namely, sometimes you write code where the "what" is not obvious. In
>> that situation, it may not be possible to change the code to make it
>> more obvious, because you are bound by contract. For example, suppose
>> you are implementing an interface with a method called sort(). The
>> fact that you choose a quicksort or a mergesort is a useful one-line
>> comment, and in my opinion is probably better than just wrapping a
>> method called quicksort() and hoping it gets inlined away.
>> 
>> A slight variant of this is that you may be learning or
>> reverse-engineering code where the "what" is not clear (perhaps
>> because it was poorly rewritten). In the longer-term, ideally you'd
>> clean up the code, but in the short term it might be useful to capture
>> the "what" in comments until you can safely refactor things and delete
>> the comments.
>> 
>> For example, evmar at chromium.org commented on IRC that he often checks
>> out files from the tree, adds local comments to the file, to help keep
>> track of what the code does, but then deletes them prior to checking
>> his patches back in. He's a pretty good guy, so I am guessing that his
>> comments would be useful to others as well (a net improvement to the
>> code and an incremental step towards the refactoring described above),
>> and it's a shame that that effort is wasted.
>> 
>> Two other specific examples, from the Python code I alluded to in my
>> first message:
>> 
>> In Tools/Scripts/webkitpy/common/system, there are the "fileset.py",
>> "directoryfileset.py", and "zipfileset.py" files. They are otherwise
>> fine but have no comments in them, probably because the contributor
>> felt that comments were not "webkit style". However, it took me a
>> non-trivial amount of time -- and a brief conversation with Mihai, who
>> reviewed the code -- to understand the structure and rationale of the
>> code (what is a fileset for, etc.). A couple of file-level or
>> class-level comments would've eliminated that concern and may save
>> others the same time in the future.
>> 
>> Secondly, in zipfileset.py, there is an open() method (line 49). What
>> that method does is entirely non-obvious: It extracts a named member
>> of a zip file and saves it into a file named according to the
>> "filename" parameter. I think this routine is an example of something
>> that would benefit from both either a "what" comment (or a renaming of
>> the method, but it's not obvious to me what it should be renamed to,
>> perhaps extract_as or extract_and_save_as) and a "why" comment
>> (because we need to take the "-actual" files from the layout test
>> archives and rename them to the "-expected" versions.
>> 
>> -- Dirk
>> 
>> [ Note that this is not meant to be a mark against that code, which is
>> generally pretty good IMO. I would have similar feedback on virtually
>> any C++ file I could open up, I suspect. This just happened to be the
>> file that prompted the train of thought. ]
>> 
>> On Thu, Jan 27, 2011 at 3:46 PM, Darin Adler <darin at apple.com> wrote:
>>> On the WebKit project we like “why” comments, but not “what” comments.
>>> Comments that say what the next block of code does usually are not a good
>>> idea. Instead by using appropriate names and granularity we want the code
>>> itself to say what the comment would have.
>>> We also frown on “textbook” style comments. Long block comments that read
>>> like a manifesto about what a code or class will do aren’t typical in
>>> WebKit.
>>> It’s critical that comments contain information that the code does not,
>>> rather than repeating information that is already present in the code. And
>>> that comments are brief enough and easy enough to read that they are likely
>>> to be noticed and updated as the code changes.
>>> I don’t think this has much to do with specific programming languages.
>>> A good discussion of comments could revolve around some specific code
>>> sequences and a discussion of whether a particular comment is suitable. I’m
>>> not sure we can get very far in the abstract.
>>>     -- Darin
>>> 
>> 
> _______________________________________________
> webkit-dev mailing list
> webkit-dev at lists.webkit.org
> http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev

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