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

[Greg Chicares]

On 2018-05-19 12:51, Vadim Zeitlin wrote:
> On Fri, 18 May 2018 23:10:06 +0000 Greg Chicares <address@hidden> wrote:
> 
> GC> On 2018-05-18 17:45, Vadim Zeitlin wrote:
[...]
> GC> > but IMNSHO there should really be readable constants to allow writing
> GC> > column_parameters{..., column::visible} instead of the current
> GC> > version of the code.
> GC> 
> GC> Do you suggest something more or less like the following?
> GC> 
> GC>  struct column_parameters
> GC>  {
> GC> +    enum enum_show_or_hide {e_hidden, e_visible};
> GC>      std::string const header;
> GC>      std::string const widest_text;
> GC> -    bool        const hidden;
> GC> +    enum_hidden const hidden;
> GC>  };
> 
>  Yes.

Okay, like commit 13f94244 then?

Formerly, we determined hidden-ness by calling 'bool hide_column()', e.g.;
we passed its result into a bool variable, and used it via 'bool is_hidden()'.
Thus, the bool variable wasn't really used directly. Is it really better now?
E.g., this has become harder to read:

             vc.push_back
                 ({i.header
                  ,i.widest_text
-                 ,hide_column(ledger, column++)
+                 ,hide_column(ledger, column++) ? oe_hidden : oe_shown
                 });
             }

I could make 'bool hide_column()' return an enum, but that would only push
the awkwardness upstream.

One other thing troubles me about this: the extensibility argument for
enums rather than bools. Formerly, we had 'bool hidden_;', which wasn't
extensible at all. Now, we have an enumeration with two values:
     {oe_shown
     ,oe_hidden
     };
What happens if we ever add a third, for columns that are "partially hidden",
or "shown only if there's enough room"? We'd have to revisit every test like
  if(oe_shown == visibility)
or
  if(oe_hidden != visibility)
which are equivalent for bool, but equivalent for enumerations only as long
as they have exactly two complementary enumerators.

>  Except that this would result in some really long names, i.e. we'd have to
> write column_parameters::enum_show_or_hide::e_hidden, so I'd move this enum
> to outer scope and maybe abbreviate it to something like "visibility" (why
> do we need "enum_" prefix for enums? This looks like Hungarian notation
> coming back with the revenge to me...).

An 'enum_' prefix does use extra characters, but verbosity is the only
thing it has in common with hungarian notation, which encodes a variable's
type into its name:

  enum_foo x[N] = {e_bar};  // array of some enumerative type
  uint8_t arru8y[N] = {1u}; // array of unsigned eight-bit integers

'enum_'- is like the -'_t' suffix that distinguishes 'size' from 'size_t'.

I like to distinguish these lexically:
  enumerators : 'e_'-
  enumerations: 'enum_'-
Perhaps 'en_' would suffice in the latter case. I wouldn't mind
substituting that globally, but it's a big change.