Re: [lmi] [PATCH] Remove unnecessary definitions of pure virtual functions

[Greg Chicares]

On 2017-03-15 21:04, Vadim Zeitlin wrote:
> On Wed, 15 Mar 2017 19:21:31 +0000 Greg Chicares <address@hidden> wrote:
> 
> GC> On 2017-03-15 16:51, Vadim Zeitlin wrote:
> ..
> GC> >  IMO the most preferable alternative by far would be to forbid 
> constructing
> GC> > the object in an invalid state, IME such objects are a huge source of
> GC> > problems (if NULL was Hoare's billion-dollar mistake, then invalid 
> objects
> GC> > are at least a 500-million-dollar one).
> ..
> GC> I would nevertheless maintain that class round_to, as it stands,
> 
>  This is a perfectly acceptable pragmatic solution, there is no urgent need
> to go change all lmi code using this class right now and there is
> definitely no security vulnerability here. But I still think the idea of
> allowing objects to (implicitly) be in an invalid state is a poor one and
> I'd like to recommend avoiding it in any new code.

This sounds like an interesting philosophy, so let's explore it a little
more with this particular example. Reimplementing class InputSequence
took a lot of work, but it was worthwhile; maybe class BasicValues should
be next.

You initially took this stance with respect to class round_to. I'm not
saying that we shouldn't cut that particular head off the Hydra. I'm just
suggesting that we take a step back and consider first whether we this
design principle (or any other in our armory) is the way to defeat the
beast itself, and then the individual heads will fall in due course.

> GC>   class LMI_SO BasicValues{...
> GC>     round_to<double> round_specamt_;
> GC>   ...};
> GC> 
> GC>   void BasicValues::SetRoundingFunctors()
> GC>   {...
> GC>     set_rounding_rule(round_specamt_, 
> RoundingRules_->datum("RoundSpecAmt"));
> GC>   ...}
> 
>  This is a very nice example though because it shows that accepting the
> possibility of invalid state in round_to<> practically inevitably leads to
> also allowing a BasicValues object to be in an invalid state as well,
> whereas not allowing the invalid state of round_to<> would discourage
> allowing BasicValues to be in an invalid state too, while still allowing it
> if really needed.

This is where I have trouble applying your idea. Class BasicValues, as
currently designed, is a holder for perhaps half a dozen objects, all of
which are required for instantiating its derived class AccountValue, and
all of which need to be initialized in the particular context of a given
object of class Input.

Consider only 'ihs_basicval.cpp' and not 'basicvalues.cpp'. (The latter is
for the 'antediluvian' variation, which a really broad refactoring would
eliminate. Only the former is used for production.)

There are two overloads of BasicValues::BasicValues(), and one of them is
plainly marked as a disliked kludge, so let's focus only on the one that
is not so marked. I'll try to write a what I guess XP practitioners would
call a "user story" for it, and ask how you'd implement it.

<background> A "sales illustration" is a document, required by regulation,
that accompanies a life-insurance policy and is generally presented to the
customer before a sale is made. Class Input represents a complete set of
input parameters that distinguish one illustration from another: e.g., it
specifies the customer's name, age, and amount of insurance...and about
two hundred other parameters. </background>

<background> One extremely important parameter in class Input is the
"product", represented as the string Input::ProductName. One product might
provide a death benefit for a term of ten years only, with a premium that
increases every year (because the probability of death increases as people
age). Another might provide a guaranteed death benefit throughout the whole
of life, in return for a stated annual premium that is guaranteed never to
increase. Many products are hybrids of those two basic designs, limited only
by the imagination, and other basic designs exist as well. Each product is
specified by a set of a couple hundred parameters in its "product database".
</background>

<user_story> Given an instance of class Input, construct many objects that
are prerequisites for the task of generating an illustration--respecting an
elaborate set of construction dependencies driven by the logic of the problem
domain--and hold onto them for other classes's use in carrying out that task.
<user_story>

For example, the existing BasicValues::Init() constructs these subobjects in
a fixed order:
 - Input::ProductName determines an xml file that is read into member
   ProductData_, which embodies the "product" as a set of strings, the most
   important of which are the names of other xml files that embody various
   individual facets of the product
 - Seven parameters that are members of class Input are arguments to the
   constructor for member Database_, the "product database" mentioned above.
     <digression> Clearly the name of the product-database xml file is needed
     here. It might have been plucked from the ProductData_ member constructed
     in the last step above, but instead the product database constructs its
     own "product" member akin to ProductData_ above--arguably more general,
     and perhaps affordable, but not obviously ideal. I believe I already
     admitted that this whole hairball might profitably be redesigned, and
     we're only on the second line of BasicValues::Init(). </digression>
Then BasicValues::Init() proceeds to initialize its (many) other shared_ptr
members, in dependency order. Thus:

    RoundingRules_.reset(new rounding_rules(....
    // Mortality and interest rates require database and rounding.
    // Interest rates require tiered data and 7702 spread.
    MortalityRates_.reset(new MortalityRates (*this));
    InterestRates_ .reset(new InterestRates  (*this));
    ...

I guess BasicValues::Init() is more of a mess than it needs to be (and I'm
not really sure why those members are shared pointers anyway--perhaps they
were auto_ptrs back in the 16-bit world where these subobjects were too big
to exist on a 65536-byte stack segment.) But the user story, AFAICT, really
is as simple as explained above: just construct these subobjects and hold
onto them as a data source while the illustration is being generated.

That being said...

> GC> Should we give up the ability to hold a round_to object as a member? or 
> the
> GC> ability to initialize it after the proper context has been set (such as 
> the
> GC> name of the file from which the rules are read)?
> 
>  Again, ideal would be to forbid creating BasicValues in an invalid state
> and provide a factory function taking the name of the file, reading the
> rules from it and then creating BasicValues with the rounding method
> specified by these rules.

The intention of the discussion above is to move our focus from this
particular detail to a much more general level. BasicValues::Init()
 - reads the product xml file that contains the name of the xml file
   that contains the rounding rules
 - using that file name, rounding rules are read into round_to members
but that function does so many other things that, if we turn its logic
inside out, I fear we'll have something nightmarish like the following
(indented to show structure):

  BasicValues
    (Input input
      ,ProductData_ = product_data(input)
        ,RoundingRules_(ProductData_.RoundingFilename)
          ,round_specamt(RoundingRules_.datum("RoundSpecAmt")
          ,...about twenty other rounding rules
        ,...a dozen other subobjects like RoundingRules_
          ,...each with...let's say "numerous" sub-sub-objects

Of course, that's not necessarily more nightmarish than what we have
in production today. The central design issue, I think, is to find the
top-level structure that minimizes total complexity. I don't pretend
to have found it. I think the design pattern present in production is
called "FORTRAN": a procedural hierarchy surrounded by an ocean of data.
Or maybe that's too harsh, because I really did manage to partition that
ocean into logical seas. But even so, this

struct Oceans
{
  shared_ptr<Arctic>   arctic_;
  shared_ptr<Atlantic> atlantic_;
  ...
  std::vector<int>    flotsam();
  std::vector<double> jetsam();
  ...
};

would seem to leave room for improvement.