Re: [lmi] Group quotes, part deux

[Greg Chicares]

On 2016-05-05 12:58, Greg Chicares wrote:
[...]
> Where should the results of this "extra processing" be stored?
> Right now, we have
>     global_report_data report_data_;
>     std::vector<row_data> rows_;
>     totals_data totals_;
> aside from which 'group_quote_pdf_generator_wx' has no data members
> other than
>     page_metrics page_;
>     int row_num_;
> My inclination is to add a few new data members to that main class,
> but it seems best to ask first, because I don't want to pollute the
> design through lack of understanding. The global_report_data struct
> might be a better place to store these new data, for encapsulation;
> OTOH, that struct is now just a collection of strings, all directly
> ascertainable from the composite ledger alone, and adding this new
> information (which is not ascertainable that way) would change the
> nature of the struct. Perhaps this question is too abstract, and I
> should just implement something and solicit criticism.

Annotated, excerpted diff of revision 6560 (20160505T1439Z):

Index: group_quote_pdf_gen_wx.cpp

+        // Dynamically-determined fields.
+        std::string plan_type_footnote_;

I added that member to struct global_report_data...

+    std::string plan_type_;

...but added that member to class group_quote_pdf_generator_wx.
This keeps footnotes together in struct global_report_data, but
scatters "plan type" members across different classes. Is this
good, or bad? Can we state a rule that explains which should
go where?

In global_report_data::fill_global_report_data():

     footer_ =
           brbr (invar.GroupQuoteIsNotAnOffer)
         + brbr (invar.GroupQuoteRidersFooter)
+        + brbr (plan_type_footnote_)
         + brbr (invar.GroupQuotePolicyFormId)
         + brbr (invar.GroupQuoteStateVariations)
         + brbr (invar.MarketingNameFootnote)

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

Later, in group_quote_pdf_generator_wx::add_ledger():

     if(is_composite)
         {
+        plan_type_ =
+            (invar.GroupIndivSelection ? invar.GroupQuoteRubricVoluntary
+            :has_suppl_specamt_        ? invar.GroupQuoteRubricFusion
+            :                            invar.GroupQuoteRubricMandatory
+            );
+        report_data_.plan_type_footnote_ =
+            (invar.GroupIndivSelection ? invar.GroupQuoteFooterVoluntary
+            :has_suppl_specamt_        ? invar.GroupQuoteFooterFusion
+            :                            invar.GroupQuoteFooterMandatory
+            );
         report_data_.fill_global_report_data(ledger);
         }

It seems natural to ask whether the added lines should be moved into
global_report_data::fill_global_report_data(). But they depend on
  bool group_quote_pdf_generator_wx::has_suppl_specamt_
which isn't visible there. Of course, we could pass that boolean as
an argument, or we could move its declaration into global_report_data;
but then global_report_data becomes merely a container for all of the
main class's data members, which seems wrong--and 'has_suppl_specamt_'
is not "global_report_data" because it doesn't appear in reports, so
the struct would become a generic "global_data" holder, and that seems
to be a code smell.

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

Later, in output_document_header(), I have a different design question:

-    // Then add any additional fields in left-to-right then top-to-bottom 
order.
-    std::vector<extra_summary_field> const& fields = 
report_data_.extra_fields_;
+    // Add a "plan type" field, then any additional fields,
+    // in left-to-right then top-to-bottom order.
+    std::vector<extra_summary_field> fields;
+    fields.push_back(extra_summary_field({"Plan Type", plan_type_}));
+    std::vector<extra_summary_field> const& f = report_data_.extra_fields_;
+    fields.insert(fields.end(), f.begin(), f.end());

Formerly, "plan type" was user input that parse_extra_report_fields()
processed. Now, it's deduced from context. I might have added it the way
other non-parsed fields are handled, thus:

    {
    open_and_ensure_closing_tag tag_tr(summary_html, "tr");
    append_name_value_to_html_table(summary_html, "Product", );
    append_name_value_to_html_table(summary_html,"Effective 
Date",report_data_.effective_date_);
    }

but there's nothing to pair it with, and it shouldn't appear on a line
of its own if parse_extra_report_fields() supplies other data. It made
sense to treat information from two sources in two ways, but now I'm
thinking "three times and you refactor". Is there good reason not to
treat everything the same way, e.g., replacing the above block by

    fields.push_back(extra_summary_field({"Plan Type", report_data_.product_}));
+   fields.push_back(extra_summary_field({"Product", plan_type_}));
+   fields.push_back(extra_summary_field({"Effective Date", 
report_data_.effective_date_}));

and letting the loop for "parsed" input handle everything? I don't
want to stomp on your design without asking, lest I destroy something
valuable that I just don't happen to notice.

BTW:
+   fields.push_back(extra_summary_field({"Plan Type", plan_type_}));
Is that the "best" syntax in modern C++? I didn't realize I could do
it that way and was slightly surprised that it works. Perhaps it
works in K&R C and I had just forgotten about it.