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

[Greg Chicares]

On 2018-05-19 17:07, Vadim Zeitlin wrote:
> On Sat, 19 May 2018 12:54:40 -0400 (EDT) Greg Chicares <address@hidden> wrote:
[...]
> 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.

Okay, 'should_' prefix restored in a1f29ab6.

> [*] 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?

As for choosing the "hidden" rather than the "shown" sense...
Calling it should_hide_column() because its value is used downstream
as is_hidden() is a weak reason. A strong reason is that it makes the
function's definition easier to read. E.g.:

    bool should_hide_column(Ledger const& ledger, int column) const override
    {
        // Don't show AttainedAge on a composite.
        return ledger.is_composite() && column == column_end_of_year_age;
    }

means "Hide composite attained age". That's how we'd say it in natural language.

Actually, now, the comment is superfluous and could be removed; but it
was necessary earlier:

    bool should_show_column(Ledger const& ledger, int column) const override
    {
        // Don't show AttainedAge on a composite.
        return !(ledger.is_composite() && column == column_end_of_year_age);
    }

"Show this column unless it's composite attained age."

or previously:

        return column != column_end_of_year_age || !ledger.is_composite();

"Show any column except attained age, but show every column if this
is not a composite."