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

[Vadim Zeitlin]

On Sat, 19 May 2018 12:54:40 -0400 (EDT) Greg Chicares <address@hidden> wrote:

GC> branch: master
GC> commit 49ae3018dd8477a7acd793391740e064f885f0e1
GC> Author: Gregory W. Chicares <address@hidden>
GC> Commit: Gregory W. Chicares <address@hidden>
GC> 
GC>     Reverse sense of certain conditionals for readability
GC>     
GC>     Renamed and reversed the sense of a function that indicates whether
GC>     the 'hidden' flag should be set.

 I [can] understand the reversal[*], but what's wrong with "should_"
prefix? IMHO it makes the function meaning much more clear, e.g. this:

GC> @@ -365,7 +365,7 @@ class using_illustration_table
GC>              vc.push_back
GC>                  ({i.header
GC>                   ,i.widest_text
GC> -                 ,!should_show_column(ledger, column++)
GC> +                 ,hide_column(ledger, column++)
GC>                  });
GC>              }

doesn't seem to be clear at all now as it's natural to become confused as
to why should we hide a column here. I think should_hide_column() or just
is_column_hidden() would be a much better name, as hide_column() clearly
means "make this column invisible", at least to me.

 Regards,
VZ

[*] OTOH I'm still not sure if it's a good idea, the function name
    shouldn't be determined by how it's used. Will we rename it back
    if we ever decide that we want to store the "is_shown" flag, rather 
    than "is_hidden" one in column_parameters, I wonder?