Re: [Libreoffice] Script to find undocumented classes

On Wed, 2010-12-01 at 13:37 +0100, Lubos Lunak wrote:
> On Wednesday 01 of December 2010, Thorsten Behrens wrote:
> > see? That's what I meant, documentation for most of the higher-level
> > methods is either
> >
> > a) superficial
> > b) so much prose that you're better off debugging the code in the
> >    first place
> >
> > (bFull has *a lot* of side effects, and no, I did not bother to
> > research all of them for the while)
>
>  See? So nobody actually knows what the code really does, and everytime 
> somebody changes something there, they possibly have a slightly different 
> understanding of the purpose, resulting in even bigger mess. That's yet 
> another advantage of documentation - even you yourself are reminded of what 
> the code is supposed to do, and everybody touching such code is less likely 
> to run into a different direction.

I'm with Lubos here, but I believe we are slightly talking past each
other.  What Thorsten is saying (have idutils, ctags etc to power your
editor) is pretty much required to battle this code base efficiently.
But that doesn't take away the fact that the code base is massively
under-documented or have cryptic structure in some places.  And I've
seen many instances of what Lubos is saying (multiple people adding
their own stuff to the same method to bloat it into a kitchen sink that
takes so many conditional bool parameters) happening in many areas of
the code base.  I would like to have some sort of tool to analyze the
cyclomatic complexity of our code base to see if there are any hot spots
in terms of code complexity.

Now, I also agree with Thorsten that not all methods need to be
documented; some methods are quite easy to figure out from their method
names, and others are just methods extracted from other methods as part
of refactoring, whose purpose are still unknown (they were just
identical blocks of code that could be extracted into a common
function).  In the latter case I sometimes don't even know the intent of
such common block of code.

The practice I follow roughly is that, name a method something
descriptive but not to make it too long, and if there are any gotchas or
intent that can not easily be deciphered from the method name or the
content of the code, I try to put that into its comment in the header.

I think the common idiom is that, the code tells you what the method
does, and the comment tells you what the intention of the method is.
So, I personally think it's a good practice to spell out the intention
of a method in its comment if it's not obvious.  But I don't think we
need to comment a method just to re-iterate what it does.

Examples

This is IMO excessive commenting.

/**
 * Translate the passed string into English. 
 * 
 * @param str original string
 * 
 * @return English translation.
 */
string translateToEnglish(const string& str);

This is IMO good commenting.

/**
 * Store new'ed instance of English translation into internal foo 
 *  container. The container manages its life-cycle, so don't delete
 *  stored string instances!
 * 
 * @return true if the string instance is successfully stored, false 
 *         if the string was not stored.  The caller must delete the
 *         string instance when this method returns false.
 */
bool storeEnglishTranslation(const string* pStr);

Having said all this, I'm a bit concerned that if we start having a
code-comment campaign, and end up having methods with incorrect comments
because only the original authors of the methods know what the intents
of their methods were.  So, I'm not sure if we should go that far,
especially with the existing methods whose authors were not any of us.

Kohei

-- 
Kohei Yoshida, LibreOffice hacker, Calc
<kyoshida@novell.com>