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

[Vadim Zeitlin]

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.

 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 IMNSHO there should really be readable
constants to allow writing column_parameters{..., column::visible} instead
of the current version of the code.

 Regards,
VZ