Re: [lmi] [lmi-commits] master dd5ca93 2/3: Avoid magic values

[Greg Chicares]

On 2018-05-19 19:59, Vadim Zeitlin wrote:
> On Sat, 19 May 2018 17:46:07 +0000 Greg Chicares <address@hidden> wrote:
[...]
> GC>              vc.push_back
> GC>                  ({i.header
> GC>                   ,i.widest_text
> GC> -                 ,hide_column(ledger, column++)
> GC> +                 ,hide_column(ledger, column++) ? oe_hidden : oe_shown
> GC>                  });
> GC>              }
> GC> 
> GC> I could make 'bool hide_column()' return an enum, but that would only push
> GC> the awkwardness upstream.
> 
>  I think it would be slightly better to do it, although it would definitely
> require renaming the accessor to something like column_visibility().

I've noted that idea in a comment for now. There are one or two similar
"properties" I'd like to add before reconsidering this.

> GC> One other thing troubles me about this: the extensibility argument for
> GC> enums rather than bools. Formerly, we had 'bool hidden_;', which wasn't
> GC> extensible at all. Now, we have an enumeration with two values:
> GC>      {oe_shown
> GC>      ,oe_hidden
> GC>      };
> GC> What happens if we ever add a third, for columns that are "partially 
> hidden",
> GC> or "shown only if there's enough room"? We'd have to revisit every test 
> like
> GC>   if(oe_shown == visibility)
> GC> or
> GC>   if(oe_hidden != visibility)
> GC> which are equivalent for bool, but equivalent for enumerations only as 
> long
> GC> as they have exactly two complementary enumerators.
> 
>  The recommended way is to "switch" over enum value instead of checking it
> with "if". If the "default" case is not used in the switch, adding a new
> enum element would give warnings for all places where the code handling it
> would need to be added.

Yet they're cumbersome and verbose. Consider:

  private:
    bool const is_hidden_;
  public:
    bool is_hidden() const {return is_hidden_;}

If we replace that bool member variable with an enumeration
  enum oenum_visibility {oe_shown, oe_hidden};
thus:
    oenum_visibility const visibility_;
then do we really want to replace the tiny one-line accessor
above with the following (just because a third enumerator
might someday be added, however remote the possibility)?

    bool is_hidden() const
      {
        switch(visibility_)
          {
          case oe_shown:  return false;
          case oe_hidden: return true;
          }
      }

That seems like a high price to pay in this case, where we
don't seriously expect there ever to be a third enumerator,
and our only reason for using an enumeration is to replace

  column_metadata("Header", "999.99", false);

with

  column_metadata("Header", "999.99", oe_hidden);

when we don't actually write it that way--what we're actually
replacing is

  bool should_be_hidden;
  column_metadata(header, widest_text, should_be_hidden);

with

  oe_visibility should_be_hidden; // different type
  column_metadata(header, widest_text, should_be_hidden); // identical!

and I wonder whether this trip has been worth the price.

> GC> >  Except that this would result in some really long names, i.e. we'd 
> have to
> GC> > write column_parameters::enum_show_or_hide::e_hidden, so I'd move this 
> enum
> GC> > to outer scope and maybe abbreviate it to something like "visibility" 
> (why
> GC> > do we need "enum_" prefix for enums? This looks like Hungarian notation
> GC> > coming back with the revenge to me...).
> GC> 
> GC> An 'enum_' prefix does use extra characters, but verbosity is the only
> GC> thing it has in common with hungarian notation, which encodes a variable's
> GC> type into its name:
> 
>  Well, "enum" is (part of) the type... I.e. I don't understand why should
> we care about some constant being an enum versus just a const int. Also, in
> MSW code "E" prefix for enums (which is conceptually the same as "enum_")
> is always used in the same code bases which use "C" prefix for classes and
> Hungarian notation for the variables, so I just can't shake this
> association off.

[...prefixes: 'enum_' for enumerations, 'e_' for enumerators...]

> GC> I like to distinguish these lexically:
> 
>  I'd be curious to know why. I.e. you don't use "class_" prefix for all
> classes, nor "type_" prefix (or "_t" suffix) for all typedefs. Why should
> enums be singled out for special treatment?

To allow different names for types and instance variables, e.g.,
in this hypothetical example:
  void resize(size_t size);
or this actual one:
  AccountValue::IsModalPmtDate(mcenum_mode mode) const;
What are the options?

0. foo(mcenum_mode mode) above: prefixed type name

1. foo(Mode mode): uppercase types, lower case variables

2. foo(mc::mode mode): namespace for 'mc' types--interesting idea

3.a. foo(mode m): use single-letter variable names

3.b. foo(mode mod): use curiously-mangled variable names
or   foo(mode the_mode): curiously-decorated variable names

I'm not sure any of those options is obviously better than the
rest. Taking a fresh look now, I'd rank them in order of
increasing preference thus:
  3.b << 3a << 1 < 0 < 2
and would be inclined to replace (0) with (2) if it were easy.

Note that we generally must define types before we know all the
ways in which we might use them. Therefore, even though (3) may
make perfect sense for one particular function, it might not be
sensible for another. Choosing (3) as a strategy for naming
types commits us to a strategy for naming instances forever.

Then there's the problem of enumerators with similar names but
different contextual meanings. E.g., "monthly" could describe
  mce_monthly: a premium that's paid twelve times a year
  mce_monthly_rate: an interest rate that's compounded monthly
  mce_spread_monthly: a distinct variant of 'mce_monthly_rate'
Similarly, several enumerations include a "none" enumerator.
I named these things in the C++98 era, so they're distinguished
lexically. Scoped enums could solve this problem more cleanly.

And, for lmi's more strongly typed 'mc' enumerations, we have
'mcenum_'-prefixed class templates whose underlying C enumeration
is prefixed 'mce_'...so input uses 'mce_mode' for validation, but
but calculations use the simpler 'mcenum_mode' values that have
already been validated. That's two different naming conventions
for enumerations...and what about their enumerators? Let's see:

  enum mcenum_mode
    {mce_annual     =  1
    ,mce_semiannual =  2
    ,mce_quarterly  =  4
    ,mce_monthly    = 12
    };
  typedef mc_enum<mcenum_mode> mce_mode;

Thus:
  mcenum_- prefix: C enumeration mcenum_mode
  mce_-    prefix: enumerators
  mce_-    prefix: also used for template typedef'd above

I guess the only rule I've consistently applied is that an
'enum_' prefix denotes a C enumeration.

>  Also, what about the C++11 strongly typed (a.k.a. scoped) enums? Should
> they still use "enum_" prefix? If so, surely their enumerators shouldn't
> have any prefix, as otherwise we'd end up with "enum_foo::e_bar" which is
> just clearly over-specified.

The reason I haven't eagerly embraced this is that the names on
the left are so much longer than the names on the right:

mcenum_mode::annual          versus   mce_annual
mcenum_rate_period::annual   versus   mce_annual_rate

That would be less awful if we dropped the prefixes:

mode::annual          versus   mce_annual
rate_period::annual   versus   mce_annual_rate

but maybe that goes too far: the natural name for a variable of
type 'rate_period' is just 'rate_period', so we're back to the
{0,1,2,3a,3b} options above.

> GC>   enumerators : 'e_'-
> GC>   enumerations: 'enum_'-
> GC> Perhaps 'en_' would suffice in the latter case. I wouldn't mind
> GC> substituting that globally, but it's a big change.
> 
>  I don't think making things much less readable for the gain of 2
> characters is worth it. I.e. I'd rather either get rid of the prefixes
> entirely (but this doesn't seem to be on the cards)

...and instead choosing from one of the other {0,1,2,3a,3b} options
above, or something else entirely?

> or keep it as is.

Yeah.

>  Concerning oenum_visibility specifically, I think it would make much more
> sense to use a scoped enum for it, i.e.
> 
>       enum class oenum_visibility
>               {shown
>               ,hidden
>               };
> 
> As I might have hinted above, I'd drop the "oenum_" prefix too, but this is
> a stylistic issue, whereas using a scoped enum instead of unscoped one a 
> clear objective advantage of making them safer to use by removing the
> implicit conversions, so this is more important.

Yet, even supposing we replace 'oenum_visibility' with
'oe::visibility' in accordance with (2) above, we'd still have:

- ,should_hide_column(ledger, column++) ? oe_hidden : oe_shown
+ ,should_hide_column(ledger, column++) ? oe::visibility::hidden : 
oe::visibility::shown

which is rather more verbose. Against that, we must weigh the gain
in type safety. This is already forbidden with unscoped enums:

  enum E {x, y};
  void foo(E e);
  foo(0);     // already forbidden
  E e = x;
  e += 5;     // already forbidden
  int i = e;  // allowed, but what's the problem?

and that's really important. Scoped enums add type safety where it's
less important IMO:

  enum class E {x, y};
  void foo(E e);
  foo(0);     // was already forbidden
  E e = E::x; // now must say 'E::x'
  e += 5;     // was already forbidden
  int i = e;  // no longer allowed, but how big is the benefit?

Looking at it in a different way, suppose compilers offered an option to
warn on implicit conversion from enum. Given the function mentioned above:

  bool AccountValue::IsModalPmtDate(mcenum_mode mode) const
  {
      return 0 == Month % (12 / mode);
  }

would I want to enable such a warning? Probably, at least from time to
time (as I do with -Weffc++). Would I add a static_cast? Maybe; maybe not.
But would I want to replace every
  /mce_annual/mcenum_mode::annual/
  /mce_semiannual/mcenum_mode::semiannual/
  ...
everywhere just so that I could have extra type safety that I don't really
care about, at a cost in readability? I don't think so.

Did the committee give us a balky language change where a compiler warning
would have been much better?