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

[Vadim Zeitlin]

On Sat, 19 May 2018 17:46:07 +0000 Greg Chicares <address@hidden> wrote:

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

 Yes, thanks.

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

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.


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.

GC>   enum_foo x[N] = {e_bar};  // array of some enumerative type
GC>   uint8_t arru8y[N] = {1u}; // array of unsigned eight-bit integers
GC> 
GC> 'enum_'- is like the -'_t' suffix that distinguishes 'size' from 'size_t'.

 The "_t" is a POSIX convention which makes sense for C, but not really for
C++.

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?

 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.

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) or keep it as is.

 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.

 Regards,
VZ