Re: [lmi] [lmi-commits] [6610] Do not attempt to define an average for "additional" premium

[Vadim Zeitlin]

On Fri, 20 May 2016 12:47:43 +0000 (UTC) address@hidden wrote:

> Revision: 6610
>           http://svn.sv.gnu.org/viewvc/?view=rev&root=lmi&revision=6610
> Author:   chicares
> Date:     2016-05-20 12:47:43 +0000 (Fri, 20 May 2016)
> Log Message:
> -----------
> Do not attempt to define an average for "additional" premium
> 
> Modified Paths:
> --------------
>     lmi/trunk/group_quote_pdf_gen_wx.cpp

 There is nothing really important here, but just a couple of minor
remarks:

> Modified: lmi/trunk/group_quote_pdf_gen_wx.cpp
> ===================================================================
> --- lmi/trunk/group_quote_pdf_gen_wx.cpp      2016-05-19 21:44:14 UTC (rev 
> 6609)
> +++ lmi/trunk/group_quote_pdf_gen_wx.cpp      2016-05-20 12:47:43 UTC (rev 
> 6610)
> @@ -1344,22 +1344,64 @@
>              ,'$' + ledger_format(totals_.total(col), f)
>              );
...
>          std::string average_text;
> -        switch(col)
> +
> +        // The cast is only used to ensure that if any new elements are added
> +        // to the enum, the compiler would warn about their values not being
> +        // present in this switch.
> +        switch(static_cast<enum_group_quote_columns>(col))

 This comment doesn't make sense in conjunction with the "default" label
below, the warning won't be given if there is a "default" label, so either
this comment (and the cast) should be removed or (probably preferably) the
"default" label replaced with all the cases for the columns not handled.

>              {
>              case e_col_basic_premium:
> -            case e_col_additional_premium:
> +                {
> +                double const dividend = totals_.total(e_col_basic_premium);
> +                double const divisor  = 
> totals_.total(e_col_basic_face_amount);
> +                LMI_ASSERT(0.0 != divisor);
> +                double const average = 1000.0 * dividend / divisor;
> +                average_text = '$' + ledger_format(average, f);
> +                }
> +                break;
>              case e_col_total_premium:
> -                // We can rely on the face amount column corresponding to 
> this
> -                // premium just preceding it because the way we display the
> -                // averages wouldn't make sense otherwise.
> -                double const average = 
> 1000*totals_.total(col)/totals_.total(col - 1);
> +                {
> +                double const dividend = totals_.total(e_col_total_premium);
> +                double const divisor  = 
> totals_.total(e_col_total_face_amount);
> +                LMI_ASSERT(0.0 != divisor);
> +                double const average = 1000.0 * dividend / divisor;
>                  average_text = '$' + ledger_format(average, f);
> +                }
>                  break;

 We're still computing the average of the total premiums column
unnecessarily. This doesn't result in NaNs or infinities any more, of
course, but should arguably still be avoided. I.e. why not add a test for
totals_.total(e_col_additional_premium) being non zero before doing this?


 Also, the last and least point: wouldn't it be better to compute and
format the average only once instead of duplicating these lines in both
branches? E.g. something like

        double
            dividend = 0,
            divisor = 0;
        switch(col)
            {
            case e_col_basic_premium:
            case e_col_additional_premium:
                dividend = totals_.total(e_col_basic_premium);
                divisor  = totals_.total(e_col_basic_face_amount);
            case e_col_total_premium:
                dividend = totals_.total(e_col_total_premium);
                divisor  = totals_.total(e_col_total_face_amount);
                break;
            }
            break;

        if(dividend != 0.0)
            {
            LMI_ASSERT(divisor != 0.0);
            double const average = 1000.0 * dividend / divisor;
            average_text = '$' + ledger_format(average, f);
            }

?

 Regards,
VZ