On Tue, 30 Nov 2010 18:50:17 +0100, Lubos Lunak <l.lunak@suse.cz> wrote:
On Tuesday 30 of November 2010, Thorsten Behrens wrote:Additionally, I think most classes don't necessarily need detailed docs for all methods in the first place (which may also hurt later merging from OOo), but would already benefit from a two-line "mission statement" at class level (of course plus some module-level overview of "what's inside").I beg to differ.
This could be a significant factor for new possible contributors. While patches removing dead code or similar certainly help too, the codebase can move forward only by people writing new code, and that requires understanding of the existing code.
I am with Lubos here I have to say. I do see Thorsten's point on that
wrong documentation is worse than no documentation, and fixing wrong
APIs and function names would be preferred. I also see how it makes
merging worse.
BUT, as a newcomers, I was very much overwhelmed with millions of lines
of code, many of which were not documented at all. And as a newcomer you
don't even know what is a core API and what is a seldomly used
thing.
People who are not intimate with but rather intimidated
by the code base, do need these docs to steer around and understand the code.
Being able to look up in doxygen what a "bFull" parameter implies
and which effect it has in a function, would be a real boon. Let me give
a random real world example that I just pulled up by searching the code
base for a bFull parameter :):
In /libs-core/editeng/inc/editeng/unofored.hxx we define:
QuickFormatDoc( BOOL bFull=FALSE );
What does bFull mean? Not so quick? What portions will be formatted if
this is FALSE? Looking at the function it either calls
pImpEditEngine->FormatFullDoc();
or
pImpEditEngine->FormatDoc();
What the heck is the difference between those functions? Now I have to
go another layer deeper to those 2 (both undocumented!) functions.
All that FormatFullDoc does:
for ( sal_uInt16 nPortion = 0; nPortion < GetParaPortions().Count(); nPortion++ )
GetParaPortions()[nPortion]->MarkSelectionInvalid( 0,
GetParaPortions()[nPortion]->GetNode()->Len() );
FormatDoc()
Excuse me, what does the stuff before we end up in FormatDoc() actually
do? It must modify the document somehow as a sideeffect,because we call
FormatDoc without any parameter. And it seems to mark some selections as
invalid. So perhaps bFull=FALSE only works on selected text?
For that I need to dig into what GetParaPortions and ParaPortions
actually are and do, which is an *undocumented class* implemented here:
libs-core/editeng/source/editeng/editdoc2.cxx.
I'd give up at this point, because after reading that much code, I had
forgotten what I wanted to do in the first place :). A simple docstring
in QuickFormatDoc, such as
/**
* param bFull determines whether we need to reflow the whole document
or only the pieces that are visible on the screen.
*/
would have saved me much time, and I could actually have improved some
code rather becoming a frustrated opengrok hunter..
(note this is a bullshit comment, as I *still* don't know what bFull
really does :)).
Sebastian
Attachment:
pgpwnSe8ny6tc.pgp
Description: PGP signature