[Libreoffice] Splitting up source/header files for clarity

[Soeren Moeller]

Hi Thorsten

Thank you very much for your review, I attached a new patch (the three
patches together contain the old patch and the fixes), and below
commented on some of your your remarks.

2011/1/12 Thorsten Behrens <thb at documentfoundation.org>:
> 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.
After moving the implementation of funcdesc.hxx to funcdesc.cxx, now
its only funcdesc.cxx which uses ScResourcePublisher and ScFuncRes,
while they aren't used in global.cxx anymore. So I have now moved both
declaration an implementation of these two classes into funcdesc.cxx.

>
> 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.
Sorry, this was an error, it seems I forgot to remove the line before comitting.

>
>> +/**
>> +  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 ;)
Thank you for your tips, I have now removed the non-informative
comments, and added a clearer description of the intention (as far as
I can see from the code) of this class

>
>> +    /**
>> +      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)
I have now added a more precise comment to Clear.

>
>> +    /**
>> +      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. :)
Now I also have changed the types in the implementation in funcdesc.cxx

Regards
Sören Möller
(LGPLv3+ / MPL)

>
> 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: 0001-Moved-funcdesc-s-implementations-out-of-global.cxx.patch
Type: text/x-patch
Size: 65352 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/libreoffice/attachments/20110112/1308aff6/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Minor-fixes-to-the-previous-patch.patch
Type: text/x-patch
Size: 4487 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/libreoffice/attachments/20110112/1308aff6/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-Minor-fixes-to-the-previous-patches.patch
Type: text/x-patch
Size: 12109 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/libreoffice/attachments/20110112/1308aff6/attachment-0005.bin>