Re: [lmi] Help modifying 'rate_table.cpp'

[Vadim Zeitlin]

On Fri, 16 Dec 2016 00:06:35 +0000 Greg Chicares <address@hidden> wrote:

GC> On 2016-12-15 15:39, Vadim Zeitlin wrote:
GC> > On Thu, 15 Dec 2016 06:02:29 +0000 Greg Chicares <address@hidden> wrote:
...
GC> > GC> Doing it there, instead of in writer::write_values() (in the "text"
GC> > GC> namespace IIRC) as in my original sketch, also brought the very
GC> > GC> welcome incidental benefit that I didn't have to deal with objects
GC> > GC> of type 'boost::optional const&'. I realize it's awfully late for
GC> > GC> code review, but what was the rationale for 'optional'?
GC> > 
GC> >  I like it. I'd even go as far as saying that I love it. I think it makes
GC> > the code much clear and robust and I try to never use empty strings or
GC> > integers with special sentinel values of 0 or -1 in any new code I write.
GC> 
GC> It may very well be valuable in other use cases, but the complexity it
GC> adds is not negligible. In an early draft of the above change, I managed
GC> to trigger abort(), presumably by using the wrong assignment operator.

 Sorry, I don't see how could this be possible due to the use of
optional<>. It only throws an exception if you try to dereference an
empty one, but this is not related to the use of the assignment operator.

GC> All I wanted to do was change an integer value from six to eight. Doing
GC> that in the most obvious way with an 'int' would not have failed, and in
GC> at least that sense a POD type is more robust. Using the value of an int
GC> simply by writing its name is clearer than prepending an asterisk, and
GC> more robust because there's no asterisk to forget. This thing has a cost.

 If I understand you correctly you did write

        *opt = 8;

and this resulted in an exception (not an abort) because "opt" was empty. I
guess this is indeed more complex than writing

        num = 8;

where "num" is a plain int, but this is a wrong comparison. The right one
would be with

        num = 8;
        num_valid = true;

and I vastly prefer using "opt" here.

GC> Consider this simplified example (showing only two of the five invariants):
GC> 
GC> void actuarial_table::parse_table()
GC> {
GC>     LMI_ASSERT(-1 == table_type_    );
GC>     LMI_ASSERT(-1 == min_age_       );
GC> ...(assign real values to all those variables)
GC>     LMI_ASSERT(-1 != table_type_    );
GC>     LMI_ASSERT(-1 != min_age_       );
GC> }
GC> 
GC> The invariants are immediately clear: at entry, all those member data have
GC> obviously invalid sentinel values; at exit, all have been assigned. These
GC> invariants are limited to the function that enforces them, which guarantees
GC> that they're safe to use elsewhere without asking each one "are you valid?"
GC> before each use. In this particular case, I like sentinels better than any
GC> other technique.

 You have to keep track of these sentinels by hand and it always starts
simply enough but often ends up much less so. I really think that
optional<> is the right abstraction and it's better exactly because it
abstracts the notion of something being present or not and you don't have
to track it manually. It's very liberating and IME chances of making a
mistake when using optional<> are much, much lower than without it.


GC> I understand this point. However, AFAICT the only fields that are
GC> actually optional are text fields for which there is no meaningful
GC> difference between the value being empty vs. nonexistent.

 Yes, but you have to think about it and ask yourself whether it's really
impossible for some field to have an empty value, maybe in some particular
old table file. Of course you can do everything without optional<>. It's
just not worth it IMO.

GC> >  I'm pretty sure that if you tried to rewrite the code without using
GC> > optional<> but keeping its functionality (i.e. the ability to detect
GC> > missing/duplicated fields in the input), you'd end up with a version that
GC> > would be not only longer but also much less clear.
GC> 
GC> The use of 'optional' stems from a specification defect. I should have
GC> stated up front that missing text fields need not be detected. The
GC> business requirements for this code are fully met if implicitly default-
GC> initialized strings that are never assigned to are treated exactly the
GC> same as strings that are explicitly assigned an empty value.

 Even if you're absolutely certain that this is, and always will be, the
case, I still think the code is much more clear when it uses optional<>.
We're all used to using empty string to indicate absence of a string, but
it doesn't mean it's a good idea and it allows the code to accidentally use
the string without checking whether it's empty or not which is much more
difficult to do accidentally with optional<>.

GC> Okay, there's a distinction between them in that context. Still, if I saw
GC>    *p = 42;
GC>     p = 42;
GC> in a C++ puzzle book that says those two statements both result (in some
GC> unspecified context) in changing the value of 'p' to 42, I'd find it
GC> difficult to see how that could be the product of a good design. Maybe
GC> it becomes clear if you've used 'optional', but otherwise it's quite
GC> unexpected at first blush.

 You have to allow the first variant because optional<> has pointer-like
semantics (and if you removed this, you would make it much less convenient
to use with objects of class types). And if you didn't allow the second
one, you'd have to write "p = optional<int>(42)" or, maybe a bit better,
"p = make_optional(42)" or, if you use C++17, "p = optional(42)" which is
clearly the best, but still not as convenient as the direct assignment. I
think the standard committee made the right choice of keeping this
assignment operator, people using optional<> extensively in their code do
appreciate it.

 I really hope you will appreciate optional<> better with time because it's
something that I find very useful, especially for such a simple class.

 Regards,
VZ