develooper Front page | perl.perl5.porters | Postings from February 2015

[perl #123898] [PATCH] Modernize the second part of perldsc.pod.

Thread Next
From:
Shlomi Fish via RT
Date:
February 25, 2015 09:28
Subject:
[perl #123898] [PATCH] Modernize the second part of perldsc.pod.
Message ID:
rt-4.0.18-10549-1424856474-1837.123898-15-0@perl.org
Hi Aristotle and James,

sorry for the late reply.

On Sat Feb 21 14:34:24 2015, aristotle wrote:
> * James E Keenan via RT <perlbug-followup@perl.org> [2015-02-21 21:00]:
> > #####
> >  # print the whole thing one at a time
> >  for $i ( 0 .. $#AoA ) {
> >      for $j ( 0 .. $#{ $AoA[$i] } ) {
> >          print "elt $i $j is $AoA[$i][$j]\n";
> >      }
> >  }
> > #####
> >
> > After your revision, this section would read:
> > #####
> >  # print the whole thing one at a time
> >  for my $i ( 0 .. $#AoA ) {
> >      my $row = $AoA[$i];
> >      for my $j ( 0 .. $#{ $row } ) {
> >          print "elt $i $j is $row->[$j]\n";
> >      }
> >  }
> > #####
> >
> > The assignment to $row *does* mean that the lines in the inner loop
> > are easier to read. For training someone in Perl, that's perfectly
> > fine. But $row is, IMO, an example of what MJD long ago described as
> > a "synthetic variable" ripe for refactoring away. AFAICT, it does not
> > reduce the number of array element lookups we have to do subsequently.
> 
> Uh, every iteration of the inner loop is spared the lookup of $AoA[$i].
> 
> However, while I *would* perform this refactoring on real code, I find
> it a change for the worse in this instance in that it conceals the
> correspondence between the structure of the code and the data structure.
> 

I see - well, I'll wait for a consensus or veto here.

> For the same reason I dislike the corresponding change in the HoA and
> AoH blocks titled “print the whole thing with indices” near line 440 and
> line 550, respectively.
> 
> OTOH, with the correspondence illustrated, I don’t have a problem with
> e.g. the “print the whole thing one at a time” block from the respective
> same section pulling out the common subexpression.
> 
> > The other changes in this patch are, IMO, quite satisfactory.
> 
> Most are unobjectionable. There is one style change I strenuously object
> to, however. E.g.:
> 
>       # reading from file
>       # flintstones: lead=fred pal=barney wife=wilma pet=dino
>     + LINES:
>       while ( <> ) {
>     -     next unless s/^(.*?):\s*//;
>     +     next LINES unless s/^(.*?):\s*//;
> 
> (Repeat some two dozen times.)
> 
> I cannot find a single case where this helps comprehension. It just adds
> noise. Every single time.

I added the LINES label per the best practice of http://perl-begin.org/tutorials/bad-elements/#flow-stmts-without-labels (which also appears in the book Perl Best Practices). If there's a consensus or a veto that it shouldn't be added, then I'll remove it in the redone patch.

> 
> The added `my`s, OTOH, are all benign. But in two cases their addition
> is insufficiently deliberate. First is the “using temp vars” block near
> line 360, which is changed from
> 
>     for $i ( 1 .. 10 ) {
>         @tmp = somefunc($i);
>         $AoA[$i] = [ @tmp ];
>     }
> 
> to
> 
>     for my $i ( 1 .. 10 ) {
>         my @tmp = somefunc($i);
>         $AoA[$i] = [ @tmp ];
>     }
> 
> The added `my`s are fine, but consider the “after” version. If you have
> just declared @tmp inside the loop body to hold the return values of
> a function call, why then flatten it in an anonymous array constructor,
> which makes a copy of its contents? Just take a reference to it:
> 
>     for my $i ( 1 .. 10 ) {
>         my @tmp = somefunc($i);
>         $AoA[$i] = \@tmp;
>     }
> 

Code like that (= declaring a lexical aggregate variable and storing a reference to it in a container variable) is exactly what the text that I removed from the previous patchset to perldsc.pod warned against doing (and I thought such a warning was no longer necessary in this day and age). So what is the verdict regarding doing it?




---
via perlbug:  queue: perl5 status: open
https://rt.perl.org/Ticket/Display.html?id=123898

Thread Next


nntp.perl.org: Perl Programming lists via nntp and http.
Comments to Ask Bjørn Hansen at ask@perl.org | Group listing | About