advice needed: bug44456 addition of ODF-formula datedif to calc

[Eike Rathke]

Hi Winfried,

On Thursday, 2012-04-26 13:55:14 +0200, Winfried Donkers wrote:

> I have added the formula 'datedif', as defined in ODF 1.2 to calc (see diff).

Hey, great!

> But there are some items I would like to get advise on:
> 1. parameter fmt in datedif( date1; date2; fmt ) defines the output
> (d, m, y, yd, ym, md for days, months etc). In my code, the users
> needs to enter "d" (i.e. use the double quotes around). IMHO, that is
> not user-friendly. Can anyone tell me how to solve that?

Well, this is how the function is defined, the unit/format parameter is
a text string.

> 2. the calculation in DateDif(..) uses mean values for days in year
> (365.2425) and for days in month (30.4369). That will in some
> instances lead to -seemingly?- incorrect results. Should I do
> something about that, i.e make the code date-concious?

Yes please, make it leap year aware and use correct values for days in
months.

> 3. The various defined values for paramter fmt (d, m, y, yd, ym, md)
> are 'hard' in the code ("d", "m", "y", "yd", "ym", "md"). This does
> not look neat to me. is there a preferred way of doing such things?

No, hard coded is fine in this case as the values are not to be
translated. However, string comparison should be case insensitive to
allow also upper case values. IMHO Excel does that. Btw, do you have
access to an Excel version to compare?

> 4. Currently, when an undefined value of parameter fmt is passed, the
> function returns 0 instead of an error message tp the user. Can anyone
> tell me how to implement a correct and user-friendly handling of
> incorrect parameter values?

Throw a lang::IllegalArgumentException, the interpreter handles that.

> 5. ODF 1.2 defines DATEDIF, but the languages I've seen using this
> function use DATEDIFF. Why is ODF different (or isn't it)?

The function originates from Lotus 1-2-3 where it was called DATEDIF,
MS-Excel called it the same, and ODF just adopted that name. The
DATEDIFF function you mention is something different (VBA?).


Now some nitpicks on your code ;)

> +++ b/sc/source/core/tool/addinhelpid.cxx

Is there a specific reason why you implemented this in the
Analysis-AddIn? I don't recall exactly whether this function was
provided in Excel as an AddIn or built-in. If as AddIn it makes sense to
implement it in our AddIn for import/export from/to Excel, if not then
a built-in function may be easier.


> +++ b/sc/source/filter/oox/formulabase.cxx
> +    { "DATEDIF",                "DATEDIF",              485,    NOID,   3,  3,  V, { RR }, FUNCFLAG_EXTERNAL },

How did you determine the BIFF12 function identifier value 485?


> +++ b/scaddins/source/analysis/analysis.cxx
> +long SAL_CALL AnalysisAddIn::getDateDif( sal_Int32 nStartDate, sal_Int32 nEndDate, const STRING& aFormat ) THROWDEF_RTE_IAE
> +{
> +    long lRet = GetDateDif( nStartDate, nEndDate, aFormat );
> +    RETURN_FINITE( lRet );
> +}

Please don't use machine specific type long in API calls, use sal_Int32 instead.


> +++ b/scaddins/source/analysis/analysis.hxx
> +    virtual long SAL_CALL     getDateDif( sal_Int32 nStartDate, sal_Int32 nEndDate, const STRING& aFormat ) THROWDEF_RTE_IAE;

Here as well, use sal_Int32 instead of long.


> +++ b/scaddins/source/analysis/analysis_deffuncnames.src
> +    StringArray ANALYSIS_DEFFUNCNAME_DateDif
> +    {
> +        ItemList =
> +        {
> +            < "DATUMDIFFERENZ"; >;
> +            < "DATEDIF"; >;
> +        };
> +    };

Is DATUMDIFFERENZ the name in a localized German Excel? I didn't find
anything on that. These names should exactly match what Excel uses to be
able to import localized AddIn names (which unfortunately they were in
Excel). This is a reason why I asked if it was necessary to implement
this function as an AddIn.

Btw, when referring freedesktop.org bug numbers in commit summaries or
source code please use our convention fdo#..., so fdo#44456 here.

Thanks
  Eike

-- 
LibreOffice Calc developer. Number formatter stricken i18n transpositionizer.
GnuPG key 0x293C05FD : 997A 4C60 CE41 0149 0DB3  9E96 2F1A D073 293C 05FD
-------------- 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/20120426/3380f253/attachment-0001.pgp>