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

[Greg Chicares]

On 2017-03-12 23:33, Vadim Zeitlin wrote:
> 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> 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()
>                                 );

That's three whole lines: one-eighth of my screen, which only displays
twenty-four card images at a time. The original is only two lines:

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

> 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.

Would you actually recommend applying a patch such as that, especially
given that the unchanged lines would be irregular? (I really like
condensed, table-style code; maybe there's a way to force regularity
upon the other cases.)

> 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...

Yes, but refactoring the original unit test (2002 was not a vintage year)
was sooooo enervating that I don't even want to think of this now.

> 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)!

I run every automated test I know before each push. This was too tiny to
push alone, when I had other things pending. I've popped enough off the
bottom of my email queue that your 64-bit patch is next....