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

[Greg Chicares]

On 2016-05-20 15:44, Vadim Zeitlin wrote:
> 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
[...]
>> Do not attempt to define an average for "additional" premium
[...]
>  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.

Isn't this the same logic as is used in the places I copied it from, i.e.,
the 'switch' statements on lines 787 and 971?

In the past, I've often written 'default:' in every switch, not to specify
a default action, but to catch omitted cases: a runtime error that is shown
only on actual violations is better than no diagnostic. I adopted this habit
in ancient times when compilers didn't offer an unmentioned-case warning.
A compiler warning is preferable because it is shown at compile time and
presumably whenever a violation is possible, whether or not it occurs.

Now, we have two available warnings, '-Wswitch' and '-Wswitch-enum':
  
https://gcc.gnu.org/onlinedocs/gcc-4.8.5/gcc/Warning-Options.html#Warning-Options
We must already be using '-Wswitch' because '-Wall' turns it on. We aren't
using '-Wswitch-enum' (it gives this warning even if a 'default:' case is
specified).

Probably we should try both globally, choose which to use depending on what
we find, and remove "default: throw something" wherever it occurs. But we
can't do that before the pending release, and I don't think we should touch
this particular special case now. Perhaps you'd like to open an issue for
this in your tracker so that we don't forget it, because this does seem to
be a desirable change.

>>              {
>>              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?

There are two reasons for checking whether the divisor is zero in that
case: to avoid dividing by zero, and to avoid a calculation that won't
be used. We agree that the first is important, and I think we can agree
that the assertion serves that particular purpose. (BTW, the total for
the "Total Face Amount" column is unconditionally set equal to the sum
of the "basic" and "supplemental" (aka "term") face amounts, so neither
can be zero, and the assertion cannot possibly fire.)

As for the second reason, we might save a few machine cycles if we
test whether the numerator is zero before performing the division; I
don't think that's an important savings. Against that slight benefit,
I see two valuable advantages to the alternative now in HEAD:
 - the code is simpler as it stands; and
 - if the average prints as "$0.00", we've discovered a logic error
   that someone will mention; if we print a blank string, the logic
   error might go unnoticed.

>  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);
>           }
> 
> ?

That's the way I initially wrote it. I refactored it because the code
in HEAD now is shorter and simpler.