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

[Vadim Zeitlin]

On Sun, 27 May 2018 02:04:52 +0000 Greg Chicares <address@hidden> wrote:

GC> On 2018-05-26 15:25, Vadim Zeitlin wrote:
GC> > On Sat, 26 May 2018 09:54:17 -0400 (EDT) Greg Chicares <address@hidden> 
wrote:
GC> [...]
GC> > GC>     Prefer single-character iterator names
GC> > GC>     
GC> > GC>     The natural name for the [dereferenced] iterator in [ranged] 
for(...) is
GC> > GC>     i in an outer loop and j in an inner loop.
GC> > 
GC> >  Sorry, but I just have to ask again: are you sure about this? IMO this
GC> > hurts clarity for a very tiny gain in brevity and so is absolutely not
GC> > worth it.
GC> 
GC> For me, this is mainly an improvement in clarity; brevity is just a nice
GC> incidental benefit. Consider (capitalization added for emphasis):
GC> 
GC> @@ -388,9 +387,9 @@ void wx_table_generator::output_horz_separator
GC> 
GC> -    for(std::size_t COL = begin_COLumn; COL < end_COLumn; ++COL)
GC>          {
GC> -        x2 += all_COLumns().at(COL).COL_width();
GC>          }
GC> 
GC> Six unique words, excluding C++ keywords and std:: names...
GC>   begin_COLumn
GC>     end_COLumn
GC>     all_COLumns
GC>         COL_width
GC>         COL
GC>         x2
GC> Five of them contain the syllable COL. Four already did; therefore, of all
GC> the names we could use for the fifth, the worst possible choices are COL
GC> and COLumn.

 Sorry, I hadn't realized it but apparently we just have completely
different mental models for reading. For me it's completely immaterial that
"col" is contained in "col_width" or "all_columns", it can be helpful (as
mentioned below), but even assuming it isn't, I just don't see any problem
with this. Do you really just select one syllable of the word to recognize
it? I am flabbergasted that anybody could possibly confuse "begin_column"
or "col_width" with "col". For me the commonality between these names is
useful because allows me to pick immediately on the relationship between
them.

GC> +    for(std::size_t i = begin_COLumn; i < end_COLumn; ++i)
GC>          {
GC> +        x2 += all_COLumns().at(i).COL_width();
GC>          }
GC> 
GC> And the best of all possible choices is 'i'. When we see 'i', we think
GC> 'iterator', which is what this is. It stands out clearly from everything
GC> else; and it's different in kind and in function from everything else.

 But you don't know what does this iterator iterate over. It's like calling
all local variables "lv" because it's a "local variable" and what could be
more clear than that? I understand that I'm not going to change your mind
now if it already works like this but please believe me that at least for
me (and, IME, for plenty of other people), using single character variable
names is much less clear than using anything descriptive. In fact, it seems
to be such a widely known fact that I had no idea it could work differently
for anybody. Now I do...

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

 Again, sure, it's an iterator (or maybe an index? because indices are also
"i"s...), but what does it give you? Nothing whatsoever without knowing
what does it iterate over. This information is just lost in the code and
unless you have it in your short-term memory, you need to scroll up (well,
using "[{" motion in Vim a couple of times is better than this) to recover
it. IMNSHO this cryptifies the code significantly.

 Without even mentioning the fact that the compact for loop form in C++11
does its best to isolate us from the fact that we're dealing with
iterators. And, of course, to be totally precise, "i" is _not_ an iterator
but a reference to the element of the collection so, to add insult to
injury, not only it's unclear what it points to, but it also seems like
there is a missing dereference here (as you'd need with a real iterator).

GC> >  As before, I would propose a rule saying that "i" should be used for the
GC> > loop variable if it's really just a mute index, i.e. carries no meaning of
GC> > its own. But when it's a column, or a line, or a position, or a year, it
GC> > would make much more sense to me to call it appropriately instead of using
GC> > one letter names.
GC> 
GC> Almost all of commit 0d5b38d deals with 'col', as discussed above.
GC> Reviewing the remainder again, first I saw:
GC>   for(auto const& page : pages_)
GC> and I thought that wasn't necessarily too awful, but then when I saw
GC>   for(auto const& line : lines)
GC> my heart sank--I thought I had overlooked a glaring defect.

 I honestly absolutely no idea what this defect could be :-(

GC> I had to move my eyes from 'line' to 'lines' and back several times
GC> before I recognized that they really are different.

 How could they not? The code wouldn't even compile otherwise.


 Anyhow, I'll end this thread because it doesn't seem useful to continue it
further, but I've now found my new absolutely least favourite (and by far)
lmi style rule :-(

 Regards,
VZ