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

[Greg Chicares]

On 2018-05-18 17:45, Vadim Zeitlin wrote:
> On Fri, 18 May 2018 12:37:12 -0400 (EDT) Greg Chicares <address@hidden> wrote:
> 
> GC> branch: master
> GC> commit dd5ca9324155c2ed988b8c540e9a9cbe89860606
> GC> Author: Gregory W. Chicares <address@hidden>
> GC> Commit: Gregory W. Chicares <address@hidden>
> GC> 
> GC>     Avoid magic values
> GC>     
> GC>     PDF-generator columns may be hidden or not. Expressing this boolean
> GC>     property as a boolean variable is clean and direct; expressing it by
> GC>     changing the header to an empty string was obscure.
> 
>  I'm of the opinion that using bool parameters (with the possible exception
> of the only parameter of functions named using verbs, e.g. enable(bool)) is
> a bad idea. I wrote
> 
>    https://www.wxwidgets.org/develop/coding-guidelines/#no_bool_params
> 
> 20 years ago and still could sign under every word.

Here, of course, the issue is not extensibility (either a column is
hidden, or it is not), but readability.

>  In this particular case, the existing code is not too bad because of the
> existence of a "bool hidden" variable. But calling the new ctor directly,
> e.g. writing column_parameters{"Name", GetTextExtent(...).x, false}, would
> be completely unreadable.
> 
>  I don't necessarily disagree that using 0 width as meaning that the column
> is hidden was obscure (although it seems like a reasonable extrapolation of
> the variable value to me...),

But "0 == width" means "variable width", which also means "clipping".
It's "header.empty()" that means "hidden". Well, "0 == width" must
also be forced later for hidden columns, but if it's zero at the time
of construction, that signifies something else. We have three conditions:
  0 == header.size()
  0 == width at time of construction
  0 == width later
and, while each of the three zeros is individually plausible, I find it
difficult to deduce the intended meaning from the state.

> but IMNSHO there should really be readable
> constants to allow writing column_parameters{..., column::visible} instead
> of the current version of the code.

Do you suggest something more or less like the following?

 struct column_parameters
 {
+    enum enum_show_or_hide {e_hidden, e_visible};
     std::string const header;
     std::string const widest_text;
-    bool        const hidden;
+    enum_hidden const hidden;
 };

That would call for corresponding changes in client classes, of course,
which might make them more readable, too. And I'm not sure whether such
an enum should be a member of that struct, or an independent entity at
namespace scope, but that's a minor detail.

If that's what you have in mind, then sure, I'll do it. I want to add
an "alignment" enum anyway, to replace
    bool is_centered()       const {return !is_variable_width_;}
which means "centered" unless align_right_ is true, in which case
is_centered() means "right aligned". I see how this all came about
historically, but I don't deal well with cognitive dissonance.