Re: [lmi] [lmi-commits] master 0d5b38d 4/4: Prefer single-character iterator names

[Greg Chicares]

On 2018-05-26 15:25, Vadim Zeitlin wrote:
> On Sat, 26 May 2018 09:54:17 -0400 (EDT) Greg Chicares <address@hidden> wrote:
[...]
> GC>     Prefer single-character iterator names
> GC>     
> GC>     The natural name for the [dereferenced] iterator in [ranged] for(...) 
> is
> GC>     i in an outer loop and j in an inner loop.
> 
>  Sorry, but I just have to ask again: are you sure about this? IMO this
> hurts clarity for a very tiny gain in brevity and so is absolutely not
> worth it.

For me, this is mainly an improvement in clarity; brevity is just a nice
incidental benefit. Consider (capitalization added for emphasis):

@@ -388,9 +387,9 @@ void wx_table_generator::output_horz_separator

-    for(std::size_t COL = begin_COLumn; COL < end_COLumn; ++COL)
         {
-        x2 += all_COLumns().at(COL).COL_width();
         }

Six unique words, excluding C++ keywords and std:: names...
  begin_COLumn
    end_COLumn
    all_COLumns
        COL_width
        COL
        x2
Five of them contain the syllable COL. Four already did; therefore, of all
the names we could use for the fifth, the worst possible choices are COL
and COLumn.

+    for(std::size_t i = begin_COLumn; i < end_COLumn; ++i)
         {
+        x2 += all_COLumns().at(i).COL_width();
         }

And the best of all possible choices is 'i'. When we see 'i', we think
'iterator', which is what this is. It stands out clearly from everything
else; and it's different in kind and in function from everything else.

> But if there is a clearly better name, why not use it? Doesn't it help to
> see in the loop body that the variable contains a column or a line index

IMO, no. It helps to be able to pick out the iterator, and 'i' stands out.
To read a for-loop, I must first read and understand the controlling
statement at the top. Until I understand that, I can't read anything.
Once I understand that, I don't need to be reminded what the iterand is.
But I do need to identify each occurrence of the iterator.

> GC> @@ -357,9 +357,9 @@ class group_quote_pdf_generator_wx
> GC>        public:
> GC>          totals_data()
> GC>              {
> GC> -            for(int col = e_first_totalled_column; col < e_col_max; 
> ++col)
> GC> +            for(int i = e_first_totalled_column; i < e_col_max; ++i)
> GC>                  {
> GC> -                value(col) = 0.0;
> GC> +                value(i) = 0.0;
> GC>                  }
> GC>              }
> 
>  Granted, it could be argued that there is no real clarity loss here
> because the loop body is so tiny that you can easily see that the loop
> iterates over columns. But OTOH what is the gain here?

Evidently our parsers differ: maybe yours recognizes word boundaries,
whereas mine is tuned to something like Levenshtein distance, so I see:
  for(col = col; col < col; col) {col}
and my eyes glaze over.

>  But looking at a bigger loop, e.g. this one:
> 
> GC> @@ -538,32 +538,32 @@ void 
> group_quote_pdf_generator_wx::add_ledger(Ledger const& ledger)
[...]
> GC> -    for(int col = 0; col < e_col_max; ++col)
> GC> +    for(int i = 0; i < e_col_max; ++i)
> GC>          {
[...]
> GC> -        switch(static_cast<enum_group_quote_columns>(col))
> GC> +        switch(static_cast<enum_group_quote_columns>(i))
> GC>              {
> GC>              case e_col_number:
> GC>                  {
> GC>                  // Row numbers shown to human beings should be 1-based.
> GC> -                rd.output_values[col] = wxString::Format("%d", row_num_ 
> + 1).ToStdString();
> GC> +                rd.output_values[i] = wxString::Format("%d", row_num_ + 
> 1).ToStdString();
[...snip more than eighty lines...]
> GC>              case e_col_total_premium:
> GC>                  {
> GC>                  double const z = invar.ModalMinimumPremium.at(year) + 
> invar.ModalMinimumDumpin;
> GC> -                rd.output_values[col] = '$' + ledger_format(z, f2);
> GC> +                rd.output_values[i] = '$' + ledger_format(z, f2);
> GC>                  if(is_composite)
> GC>                      {
> GC> -                    totals_.total(col, z);
> GC> +                    totals_.total(i, z);
> GC>                      }
> GC>                  }
> GC>                  break;
> 
>  The code seems to be obviously more cryptic now, when you don't see what
> does "i" correspond to.

On the contrary--I can't remember what 'col' means, but 'i' is obviously
an iterator, so it's clearer to me with 'i'.

>  As before, I would propose a rule saying that "i" should be used for the
> loop variable if it's really just a mute index, i.e. carries no meaning of
> its own. But when it's a column, or a line, or a position, or a year, it
> would make much more sense to me to call it appropriately instead of using
> one letter names.

Almost all of commit 0d5b38d deals with 'col', as discussed above.
Reviewing the remainder again, first I saw:
  for(auto const& page : pages_)
and I thought that wasn't necessarily too awful, but then when I saw
  for(auto const& line : lines)
my heart sank--I thought I had overlooked a glaring defect. I had to move
my eyes from 'line' to 'lines' and back several times before I recognized
that they really are different.