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

[Vadim Zeitlin]

On Sat, 26 May 2018 09:54:17 -0400 (EDT) Greg Chicares <address@hidden> wrote:

GC> branch: master
GC> commit 0d5b38d32939d8fbd92a98f530cb9738d7f22719
GC> Author: Gregory W. Chicares <address@hidden>
GC> Commit: Gregory W. Chicares <address@hidden>
GC> 
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.

 I understand that in lmi "i" and "j" are the names used for the loop
variables by convention when there are no better names, just as "z" is used
by convention for a local variable without any better, more specific name.
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
(or, in some code which I wrote recently and now am wondering whether I
should rewrite it) instead of just "i"?

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? AFAICS there is
absolutely none neither.

 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>      bool const is_composite = ledger.is_composite();
GC>  
GC>      row_data rd;
GC> -    for(int col = 0; col < e_col_max; ++col)
GC> +    for(int i = 0; i < e_col_max; ++i)
GC>          {
GC>          // The cast is only used to ensure that if any new elements are 
added
GC>          // to the enum, the compiler would warn about their values not 
being
GC>          // present in this switch.
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();
GC>                  }
GC>                  break;
GC>              case e_col_name:
GC>                  {
GC> -                rd.output_values[col] = invar.Insured1;
GC> +                rd.output_values[i] = invar.Insured1;
GC>                  }
GC>                  break;
GC>              case e_col_age:
GC>                  {
GC> -                rd.output_values[col] = wxString::Format("%.0f", 
invar.Age).ToStdString();
GC> +                rd.output_values[i] = wxString::Format("%.0f", 
invar.Age).ToStdString();
GC>                  }
GC>                  break;
GC>              case e_col_dob:
GC>                  {
GC> -                rd.output_values[col] = ConvertDateToWx
GC> +                rd.output_values[i] = ConvertDateToWx
GC>                      (jdn_t(static_cast<int>(invar.DateOfBirthJdn))
GC>                      ).FormatDate().ToStdString(wxConvUTF8);
GC>                  }
GC> @@ -571,60 +571,60 @@ void group_quote_pdf_generator_wx::add_ledger(Ledger 
const& ledger)
GC>              case e_col_basic_face_amount:
GC>                  {
GC>                  double const z = invar.SpecAmt.at(year);
GC> -                rd.output_values[col] = '$' + ledger_format(z, f0);
GC> +                rd.output_values[i] = '$' + ledger_format(z, f0);
GC>                  if(is_composite)
GC>                      {
GC> -                    totals_.total(col, z);
GC> +                    totals_.total(i, z);
GC>                      }
GC>                  }
GC>                  break;
GC>              case e_col_basic_premium:
GC>                  {
GC>                  double const z = invar.ErModalMinimumPremium.at(year);
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;
GC>              case e_col_supplemental_face_amount:
GC>                  {
GC>                  double const z = invar.TermSpecAmt.at(year);
GC> -                rd.output_values[col] = '$' + ledger_format(z, f0);
GC> +                rd.output_values[i] = '$' + ledger_format(z, f0);
GC>                  if(is_composite)
GC>                      {
GC> -                    totals_.total(col, z);
GC> +                    totals_.total(i, z);
GC>                      }
GC>                  }
GC>                  break;
GC>              case e_col_additional_premium:
GC>                  {
GC>                  double const z = invar.EeModalMinimumPremium.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;
GC>              case e_col_total_face_amount:
GC>                  {
GC>                  double const z = invar.SpecAmt.at(year) + 
invar.TermSpecAmt.at(year);
GC> -                rd.output_values[col] = '$' + ledger_format(z, f0);
GC> +                rd.output_values[i] = '$' + ledger_format(z, f0);
GC>                  if(is_composite)
GC>                      {
GC> -                    totals_.total(col, z);
GC> +                    totals_.total(i, z);
GC>                      }
GC>                  }
GC>                  break;
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. Calling "total(col, z)" can be more or less
directly be seen as meaning as "account for z in the total for the given
column". Can you really see immediately what does "total(i, z)" do? I know
I can't...

 I will, of course, [try to] follow this convention in the new code if
you're adamant about keeping it, but I just don't understand the motivation
behind taking relatively clear code and replacing it with a more cryptic
equivalent (except in in coding golf competitions) and it makes me sad and
uncomfortable to make a special effort to not write code as clearly as I'd
like to.

 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.

 Thanks for reading,
VZ