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

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

Thread Previous | Thread Next
From:
Aristotle Pagaltzis
Date:
February 21, 2015 22:33
Subject:
Re: [perl #123898] [PATCH] Modernize the second part of perldsc.pod.
Message ID:
20150221223350.GA43720@plasmasturm.org
* 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.

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.

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;
    }

Note that changing [ @tmp ] to \@tmp in the “before” version would be
incorrect – it would break the code: because @tmp was not scoped to the
loop, taking a reference to it any number of times would just repeatedly
yield a reference to the same array – whose contents are overwritten on
each iteration. Therefore the “before” version *needs* to make a copy of
the array before adding a ref to @AoA. Once you scope @tmp to the loop
body, though, the copying becomes a waste of cycles and memory.

There is a likewise shortfall of editing further down, where

    while (<>) {
        %fields = split /[\s=]+/;
        push @members, { %fields };
    }

becomes

    while (<>) {
        my %fields = split /[\s=]+/;
        push @members, { %fields };
    }

when it should become

    while (<>) {
        my %fields = split /[\s=]+/;
        push @members, \%fields;
    }


That’s all I can spot at a cursory glance.

Regards,
-- 
Aristotle Pagaltzis // <http://plasmasturm.org/>

Thread Previous | 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