Re: [lmi] [lmi-commits] master 49ae301 3/7: Reverse sense of certain conditionals for readability

[Vadim Zeitlin]

On Sat, 19 May 2018 21:14:59 +0000 Greg Chicares <address@hidden> wrote:

GC> On 2018-05-19 17:07, Vadim Zeitlin wrote:
GC> > On Sat, 19 May 2018 12:54:40 -0400 (EDT) Greg Chicares <address@hidden> 
wrote:
GC> [...]
GC> > GC>     Reverse sense of certain conditionals for readability
GC> > GC>     
GC> > GC>     Renamed and reversed the sense of a function that indicates 
whether
GC> > GC>     the 'hidden' flag should be set.
GC> > 
GC> >  I [can] understand the reversal[*], but what's wrong with "should_"
GC> > prefix? IMHO it makes the function meaning much more clear, e.g. this:
GC> > 
GC> > GC> @@ -365,7 +365,7 @@ class using_illustration_table
GC> > GC>              vc.push_back
GC> > GC>                  ({i.header
GC> > GC>                   ,i.widest_text
GC> > GC> -                 ,!should_show_column(ledger, column++)
GC> > GC> +                 ,hide_column(ledger, column++)
GC> > GC>                  });
GC> > GC>              }
GC> > 
GC> > doesn't seem to be clear at all now as it's natural to become confused as
GC> > to why should we hide a column here. I think should_hide_column() or just
GC> > is_column_hidden() would be a much better name, as hide_column() clearly
GC> > means "make this column invisible", at least to me.
GC> 
GC> Okay, 'should_' prefix restored in a1f29ab6.

 Thanks!

GC> > [*] OTOH I'm still not sure if it's a good idea, the function name
GC> >     shouldn't be determined by how it's used. Will we rename it back
GC> >     if we ever decide that we want to store the "is_shown" flag, rather 
GC> >     than "is_hidden" one in column_parameters, I wonder?
GC> 
GC> As for choosing the "hidden" rather than the "shown" sense...
GC> Calling it should_hide_column() because its value is used downstream
GC> as is_hidden() is a weak reason. A strong reason is that it makes the
GC> function's definition easier to read.

 OK, I can see this. I'm not sure if this can be easily generalized to
become a rule suitable for the inclusion into the style guide (as I
originally wanted to), but in this particular case it does make sense.

 Thanks for explaining it,
VZ