Re: [lmi] [lmi-commits] master 0d3c75a 5/6: Improve readability

[Vadim Zeitlin]

On Sun, 12 Mar 2017 23:03:44 +0000 Greg Chicares <address@hidden> wrote:

GC> On 2017-03-03 14:37, Vadim Zeitlin wrote:
GC> > On Tue, 28 Feb 2017 22:02:59 -0500 (EST) Greg Chicares <address@hidden> 
wrote:
GC> > 
GC> > GC> branch: master
GC> > GC> commit 0d3c75aaf160ee3a437685e5e02da255a6ac21e5
GC> [...trimmed to a small but representative sample...]
GC> > GC> +        auto endpoint = duration_num_field(i).GetValue();
GC> > GC> +        std::string z = value_cast<std::string>(endpoint);
GC> 
GC> [...'z' set once and used repeatedly, e.g.:...]
GC> 
GC> > GC> -                s.append(" @");
GC> > GC> -                
s.append(value_cast<std::string>(duration_num_field(i).GetValue()));
GC> > GC> +                s.append(" @").append(z);
GC> > 
GC> >  Wouldn't this code be even more readable if the names of "endpoint" and
GC> > "z" variables were exchanged? The current "endpoint" could well be made an
GC> > "unnamed" variable as it's only used once, and very close to its
GC> > definition,
GC> 
GC> Yes, its sole purpose is to break up a line which would otherwise
GC> be too long to fit on a single card:
GC> 
GC> 
00000000011111111112222222222333333333344444444445555555555666666666677777777778
GC> 
12345678901234567890123456789012345678901234567890123456789012345678901234567890
GC>         std::string z = 
value_cast<std::string>(duration_num_field(i).GetValue());

 What's wrong with

            std::string const z = value_cast<std::string>
                                  (duration_num_field(i).GetValue()
                                  );

?

GC> > but using "z" for the string value doesn't seem very helpful to
GC> > me as it's used in several places below, relatively far from the place
GC> > where it is defined, and its meaning is not immediately clear there.
GC> 
GC> I think your "relatively far" criterion is measured in physical lines,

 Yes.

GC> whereas I look at the code as though this patch had been applied:
[...patch compactifying the switch snipped...]

 OK, this does make things better.

GC> so I see 'z' as being used only in the immediate vicinity of its definition.
GC> 
GC> But there's another consideration. Compare InputSequence::canonical_form()
GC> to this switch. When I was working on this code, I didn't see any easy way
GC> to merge the two into a single function that would be simpler than the two
GC> present functions.

 Yes, they don't seem to be especially redundant...

GC> I'm tempted to replace std::string::append() with operator+=(), but if I
GC> did that I'd have to retest this thing, and I am sooooo sick of testing it
GC> that, having at long last put it aside, I really don't want to pick it up
GC> again.

 We really ought to have automatic tests for this code...

GC> >  I would also make both of these variables "const", but I'm not sure what
GC> > do you think of the "make all immutable variables const" rule that I
GC> > advocate.
GC> 
GC> Sure: commit da44805837cc91992c40d7019890453ba3e779d1

 Thanks (in advance, as I don't see it here yet)!

GC> I do like your rule. It's just too bad that 'const ' takes six characters
GC> including the space. There are people who have become rich and famous by
GC> reinventing C++, badly, and if I ever do that, all variables will be
GC> const by default:
GC>   int immutable;         // Same type as C++ 'int const'.
GC>   int mutable nonconst;  // Not const.
GC>   int mutable$;          // Not const (alternative space-saving sigil).

 I don't think anybody has become rich because of it yet, but the fact that
variables in Rust are immutable by default is one of these small things
that shows to me that people who designed it knew what they were doing.

GC> Or I could wait twenty days and propose this idea for C++ standardization
GC> as "overloading elision of const".

 I have an idea, as introducing a new keyword such as "let" or "var" will
never be possible in C++, what about making "static" mean "const auto"?
After all, "static" already means 13 different things in C++, why not give
it one more...

VZ