Re: [lmi] Group quotes, part deux

[Vadim Zeitlin]

On Thu, 5 May 2016 12:58:41 +0000 Greg Chicares <address@hidden> wrote:

GC> The columns to be shown in the new report depend on context, yet in a
GC> fairly simple way: the set of columns now in HEAD are always generated,
GC> and some (or none) are suppressed. Here, "suppressed" means that a
GC> column doesn't appear on the report whatsoever--printing its title and
GC> blank or all-zero contents below is not enough--and presumably I could
GC> figure out how to accomplish that, but it's more sensible to ask for
GC> advice.

 As long as there is no problem with still using all the columns
internally, i.e. we don't care about the tiny overhead of maintaining them
(and I don't think we do), this looks pretty simple as we could just modify
wx_table_generator class to support hidden columns and wouldn't need to
touch anything else except for the add_column() call in
group_quote_pdf_gen_wx.cpp.

GC> I can easily identify which columns to suppress; I'm just not
GC> sure how to prevent them from appearing in the PDF.

 For me, of course, it's the former part which is unclear while the latter
seems pretty trivial.


GC> Here are some concrete questions:
GC> 
GC> Where should the results of this "extra processing" be stored?
GC> Right now, we have
GC>     global_report_data report_data_;
GC>     std::vector<row_data> rows_;
GC>     totals_data totals_;

 I'm afraid there is no deep grand design here, the data are just grouped
according to the following simple logic:

1. report_data_ contains global data appearing outside of the table
2. rows_ contains per-row data
3. totals_ contains the data needed for rendering the final table row

GC> aside from which 'group_quote_pdf_generator_wx' has no data members
GC> other than
GC>     page_metrics page_;

 This basically contains the two constants defining the page width.

GC>     int row_num_;

 And this one one a simple counter of the current row, we could use
rows_.size() instead as "row_num_ == rows_.size()" is an invariant of this
class anyhow, but using a separate variable seemed better from the clarity
point of view.


GC> Does it seem beneficial to change the way totals are calculated,
GC> now that we'll need "averages" as well?

 IIRC you didn't want to calculate these totals in the PDF generation code
as you wanted to avoid any possibility of rounding errors in it. So from
this point of view, the best solution would actually be to compute averages
outside of it too, in a separate non-GUI piece of code which could be
independently unit-tested. But OTOH this could well be an overkill for
something as simple as this and I'm perfectly fine with computing the
averages here on the fly (as you probably know, we could easily compute not
only it but also the standard deviation on the fly too, I always use the
same code, coming from 4.2.2 of TAoCP volume 2, to do it).

 To return to the actual question, I don't see any real benefits to
computing the totals here, we already have them and even if the change
would be simple, why change something that works fine? OTOH I don't have
any real objections to doing it neither, it just doesn't seem like a very
important question to me.


On Thu, 5 May 2016 15:20:12 +0000 Greg Chicares <address@hidden> wrote:

GC> Annotated, excerpted diff of revision 6560 (20160505T1439Z):
GC> 
GC> Index: group_quote_pdf_gen_wx.cpp
GC> 
GC> +        // Dynamically-determined fields.
GC> +        std::string plan_type_footnote_;
GC> 
GC> I added that member to struct global_report_data...
GC> 
GC> +    std::string plan_type_;
GC> 
GC> ...but added that member to class group_quote_pdf_generator_wx.
GC> This keeps footnotes together in struct global_report_data, but
GC> scatters "plan type" members across different classes. Is this
GC> good, or bad?

 To be honest, it does seem rather bad to me as I don't understand the
difference between it and the existing global_report_data fields, in
particular extra_fields_. It would be more logical to me to put plan_type_
into global_report_data as well.

GC> we find a strong reason to keep footnotes together, so that's
GC> apparently where 'plan_type_footnote_' belongs. Does 'plan_type_'
GC> belong there too, because they both express a "plan type" concept?

 Yes, why not?

GC> Later, in group_quote_pdf_generator_wx::add_ledger():
GC> 
GC>      if(is_composite)
GC>          {
GC> +        plan_type_ =
GC> +            (invar.GroupIndivSelection ? invar.GroupQuoteRubricVoluntary
GC> +            :has_suppl_specamt_        ? invar.GroupQuoteRubricFusion
GC> +            :                            invar.GroupQuoteRubricMandatory
GC> +            );
GC> +        report_data_.plan_type_footnote_ =
GC> +            (invar.GroupIndivSelection ? invar.GroupQuoteFooterVoluntary
GC> +            :has_suppl_specamt_        ? invar.GroupQuoteFooterFusion
GC> +            :                            invar.GroupQuoteFooterMandatory
GC> +            );
GC>          report_data_.fill_global_report_data(ledger);
GC>          }
GC> 
GC> It seems natural to ask whether the added lines should be moved into
GC> global_report_data::fill_global_report_data(). But they depend on
GC>   bool group_quote_pdf_generator_wx::has_suppl_specamt_
GC> which isn't visible there. Of course, we could pass that boolean as
GC> an argument,

 This looks like the best idea.

GC> or we could move its declaration into global_report_data;

 No, I agree it doesn't belong there as it's not really a "report data".

GC> I'm sure struct global_report_data has a crisply-definable purpose;
GC> I just don't know the definition, which would enable me to answer
GC> questions like these as I add more new members.

 I hope the definition given above answers this, i.e. global_report_data is
just a plain old container for the data appearing in the report outside of
the table.


 To finish with has_suppl_specamt_, I think it could be put into another
struct, maybe something like aggregate_data, which would also contain the
current running averages if we compute them here (and, of course, also the
running totals if we change the code to compute them here too). The logic
here would be to gather all the fields that change -- or at least can
change with each new row -- in a separate struct. This is very far from
critical, of course, and has_suppl_specamt_ can well continue to stay in
the main class itself too.

 Finally, the last thing I want to say on the subject of this field is that
personally I dislike the following line:

                has_suppl_specamt_ = has_suppl_specamt_ || 0.0 != z;

and would rather write it as

                if (z != 0.0)
                    {
                    has_suppl_specamt_ = true;
                    }

instead. It's a pity that C++ (unlike Perl) doesn't have a "||=" operator
as it could have been used here, but without it the expression form
requires repeating "has_suppl_specamt_" twice which makes it unnecessarily
difficult to parse.


GC> Later, in output_document_header(), I have a different design question:
...
GC> Is there good reason not to treat everything the same way, e.g.,
GC> replacing the above block by
GC> 
GC>     fields.push_back(extra_summary_field({"Plan Type", 
report_data_.product_}));
GC> +   fields.push_back(extra_summary_field({"Product", plan_type_}));
GC> +   fields.push_back(extra_summary_field({"Effective Date", 
report_data_.effective_date_}));
GC> 
GC> and letting the loop for "parsed" input handle everything? I don't
GC> want to stomp on your design without asking, lest I destroy something
GC> valuable that I just don't happen to notice.

 No, I don't think this was done intentionally at all, AFAIR the extra
summary fields were added later and I just didn't realize I could refactor
the existing code to handle both the built-in and the extra fields in the
same way. Looking at it right now, this definitely looks like a good idea.
Should I do it or leave it to you as you're already in this code?

 
GC> BTW:
GC> +   fields.push_back(extra_summary_field({"Plan Type", plan_type_}));
GC> Is that the "best" syntax in modern C++?

 In modern C++ you would actually write

        fields.emplace_back(extra_summary_field{"Plan Type", plan_type_});

and I'm not sure why I hadn't done this here.

GC> I didn't realize I could do it that way and was slightly surprised that
GC> it works. Perhaps it works in K&R C and I had just forgotten about it.

 No, the initializer lists, like the one used to initialize the struct
extra_summary_field here, are a C++11 feature. But, of course, in C++11 you
also don't need the extra parentheses and could use emplace_back() to avoid
an extra copy (even though it's surely unnoticeable in this case). So,
again, it's a mistake (even though a quite harmless one) -- should I fix it
or should I avoid touching this code while you're working on it?

 Please let me know, thanks in advance,
VZ