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
Attachment:
pgpAmxQZpnY7F.pgp
Description: PGP signature