[Libreoffice] Splitting up source/header files for clarity

[Thorsten Behrens]

Soeren Moeller wrote:
> > I have now splitted the implementation of funcdesc.hxx out into a new
> > source file funcdesc.cxx.
>
Hi Soeren,

first off, many thanks for going to clean this up, your work here is
much appreciated! That split so far looks good.

> > In the process I also have extracted two new header files for
> > classes previously defined in global.cxx.
>
That's not necessary - ScResourcePublisher and ScFuncRes are only
used inside global.cxx, thus need no extra header - in fact, this
would not be idiomatic for c++. Simply leave them next to the code
that's using them.

See a few more comments inline of the actual patch:

> diff --git a/sc/inc/funcdesc.hxx b/sc/inc/funcdesc.hxx
> index d94d9b2..5f3ca7e 100644
> --- a/sc/inc/funcdesc.hxx
> +++ b/sc/inc/funcdesc.hxx
> @@ -34,39 +34,161 @@
>   * global.cxx
>   */
>  
> +#include "scfuncs.hrc"
> +
>  #include <tools/list.hxx>
> -#include <tools/string.hxx>
> +//#include <tools/string.hxx>
>
Please remove, don't comment out.

> +/**
> +  Contains description of a function
> +*/
>  class ScFuncDesc : public formula::IFunctionDescription
>  {
>  public:
> +    /**
> +      Constructor
> +    */
> +    ScFuncDesc();
>  
> -    virtual ::rtl::OUString getFunctionName() const ;
> +    /**
> +      Destructor
> +    */
> +    virtual ~ScFuncDesc();
>
Those doc-comments don't provide any value. Destructors and nullary
constructors seldomly need any comment at all, and for the class,
I'd suggest something along the lines of "Serves human-language
function descriptions within the calc formula parser framework" - or
somesuch. Did not look too closely, TBH. ;)

Two related golden rules for good comments/documentation: 

Good comments don't repeat the code or explain it. They clarify its
intent. Comments should explain, at a higher level of abstraction
than the code, what it does.

 or, put bluntly:

Don’t insult the reader’s intelligence ;)

> +    /**
> +      Resets the object in a clean manner
> +    */
> +    void Clear();
>
For one-liners like this, I'd prefer this markup:

/// Resets the object as if it were newly constructed 

(well, does clear() perform that? writing good comments usually
involves some detailed code review, or even debugging)

> +    /**
> +      Returns the category of the function
> +
> +      @return    the category of the function
> +    */
>      virtual const formula::IFunctionCategory* getCategory() const ;
>
Since these methods are inherited from the IFunctionDescription
interface, I'd rather document them there (in the formula module).
That way, all derived classes benefit from your documentation (and
doxygen is smart enough to link that).

> +    /* Public variables */
>
I'd consider this redundant. If you want to comment on the fact that
this class exposes its data structures, try to reconstruct the
reason why this is so - e.g. a bug number, for which it was added,
e.g. in a hurry before a shipment, to keep changes minimal (and
later cleanup was forgotten). Else, the reason would be "programmer
lazyness / sloppy coding", and that's a default assumption that
needs no comment. ;)

> -    USHORT                nFIndex;                // Unique function index
> -    USHORT                nCategory;              // Function category
> -    USHORT                nArgCount;              // All parameter count, suppressed and unsuppressed
> -    USHORT                nHelpId;                // HelpID of function
> +    sal_uInt16            nFIndex;                // Unique function index
> +    sal_uInt16            nCategory;              // Function category
> +    sal_uInt16            nArgCount;              // All parameter count, suppressed and unsuppressed
> +    sal_uInt16            nHelpId;                // HelpId of function
>
Ok to change, but I'd then also adapt the other occurences. And
maybe make that a separate patch - makes review easier, and ensures
that uncontroversial changes get in right away. :)

Could you quickly brush this up a bit & resend? As I said, otherwise
good & useful!

Cheers,

-- Thorsten
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/libreoffice/attachments/20110112/3a1c4a42/attachment.pgp>