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

[Greg Chicares]

On 2017-03-03 14:37, Vadim Zeitlin wrote:
> On Tue, 28 Feb 2017 22:02:59 -0500 (EST) Greg Chicares <address@hidden> wrote:
> 
> GC> branch: master
> GC> commit 0d3c75aaf160ee3a437685e5e02da255a6ac21e5
[...trimmed to a small but representative sample...]
> GC> +        auto endpoint = duration_num_field(i).GetValue();
> GC> +        std::string z = value_cast<std::string>(endpoint);

[...'z' set once and used repeatedly, e.g.:...]

> GC> -                s.append(" @");
> GC> -                
> s.append(value_cast<std::string>(duration_num_field(i).GetValue()));
> GC> +                s.append(" @").append(z);
> 
>  Wouldn't this code be even more readable if the names of "endpoint" and
> "z" variables were exchanged? The current "endpoint" could well be made an
> "unnamed" variable as it's only used once, and very close to its
> definition,

Yes, its sole purpose is to break up a line which would otherwise
be too long to fit on a single card:

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

Arguably the name 'duration_num_field' is too long.

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

I think your "relatively far" criterion is measured in physical lines,
whereas I look at the code as though this patch had been applied:

         switch(duration_mode_field(i).value())
             {
+            case e_retirement:      s.append(" retirement"); break;
+            case e_attained_age:    s.append(" @").append(z); break;
+            case e_duration:        s.append(" ") .append(z); break;
+            case e_number_of_years: s.append(" #").append(z); break;
-            case e_retirement:
-                {
-                s.append(" retirement");
-                }
-                break;
-            case e_attained_age:
-                {
-                s.append(" @").append(z);
-                }
-                break;
-            case e_duration:
-                {
-                s.append(" ").append(z);
-                }
-                break;
-            case e_number_of_years:
-                {
-                s.append(" #").append(z);
-                }
-                break;
            case e_maturity:

so I see 'z' as being used only in the immediate vicinity of its definition.

But there's another consideration. Compare InputSequence::canonical_form()
to this switch. When I was working on this code, I didn't see any easy way
to merge the two into a single function that would be simpler than the two
present functions. But I'd like to think that someday I'll see a way to do
that. Meanwhile, I don't want to change either one in a way that would make
it more different than the other--and canonical_form() uses 'z' in each
'case'.

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

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

Sure: commit da44805837cc91992c40d7019890453ba3e779d1
unless (to my amazement) that causes a regression when I prepare to push
it. This change is extremely safe and doesn't call for anything beyond
routine automated tests.

I do like your rule. It's just too bad that 'const ' takes six characters
including the space. There are people who have become rich and famous by
reinventing C++, badly, and if I ever do that, all variables will be
const by default:
  int immutable;         // Same type as C++ 'int const'.
  int mutable nonconst;  // Not const.
  int mutable$;          // Not const (alternative space-saving sigil).
Or I could wait twenty days and propose this idea for C++ standardization
as "overloading elision of const".