New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[PATCH] Modernize the second part of perldsc.pod. #14532
Comments
From @shlomifThis patch modernizes the second part of perldsc.pod. It adds declarations Regards, Shlomi Fish -- Shlomi Fish http://www.shlomifish.org/ E‐mail, web feeds, and doing something productive — choose two. Please reply to list if it's a mailing list post - http://shlom.in/reply . |
From @shlomif0001-perldsc-Modernize-the-code-samples-in-2nd-part.patchFrom 5fba9e7c503f0a7c80e19f00554358c225c75dc1 Mon Sep 17 00:00:00 2001
From: Shlomi Fish <shlomif@shlomifish.org>
Date: Sat, 21 Feb 2015 17:39:09 +0200
Subject: [PATCH] [perldsc] Modernize the code samples in 2nd part.
This patch modernizes and refactors the code samples, adding my
declarations and some changes.
---
pod/perldsc.pod | 191 ++++++++++++++++++++++++++++++++------------------------
1 file changed, 108 insertions(+), 83 deletions(-)
diff --git a/pod/perldsc.pod b/pod/perldsc.pod
index e2c4eae..7804231 100644
--- a/pod/perldsc.pod
+++ b/pod/perldsc.pod
@@ -276,8 +276,10 @@ If this is starting to sound scarier than it's worth, relax. Perl has
some features to help you avoid its most common pitfalls. The best
way to avoid getting confused is to start every program like this:
- #!/usr/bin/perl -w
+ #!/usr/bin/perl
+
use strict;
+ use warnings;
This way, you'll be forced to declare all your variables with my() and
also disallow accidental "symbolic dereferencing". Therefore if you'd done
@@ -336,7 +338,7 @@ X<array of arrays> X<AoA>
=head2 Declaration of an ARRAY OF ARRAYS
- @AoA = (
+ my @AoA = (
[ "fred", "barney" ],
[ "george", "jane", "elroy" ],
[ "homer", "marge", "bart" ],
@@ -350,13 +352,14 @@ X<array of arrays> X<AoA>
}
# calling a function
- for $i ( 1 .. 10 ) {
+ # Note that $AoA[0] will not be initialised.
+ for my $i ( 1 .. 10 ) {
$AoA[$i] = [ somefunc($i) ];
}
# using temp vars
- for $i ( 1 .. 10 ) {
- @tmp = somefunc($i);
+ for my $i ( 1 .. 10 ) {
+ my @tmp = somefunc($i);
$AoA[$i] = [ @tmp ];
}
@@ -372,19 +375,20 @@ X<array of arrays> X<AoA>
$AoA[1][1] =~ s/(\w)/\u$1/;
# print the whole thing with refs
- for $aref ( @AoA ) {
+ for my $aref ( @AoA ) {
print "\t [ @$aref ],\n";
}
# print the whole thing with indices
- for $i ( 0 .. $#AoA ) {
+ for my $i ( 0 .. $#AoA ) {
print "\t [ @{$AoA[$i]} ],\n";
}
# 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";
+ for my $i ( 0 .. $#AoA ) {
+ my $row = $AoA[$i];
+ for my $j ( 0 .. $#{ $row } ) {
+ print "elt $i $j is $row->[$j]\n";
}
}
@@ -393,7 +397,7 @@ X<hash of arrays> X<HoA>
=head2 Declaration of a HASH OF ARRAYS
- %HoA = (
+ my %HoA = (
flintstones => [ "fred", "barney" ],
jetsons => [ "george", "jane", "elroy" ],
simpsons => [ "homer", "marge", "bart" ],
@@ -410,19 +414,19 @@ X<hash of arrays> X<HoA>
# reading from file; more temps
# flintstones: fred barney wilma dino
- while ( $line = <> ) {
- ($who, $rest) = split /:\s*/, $line, 2;
- @fields = split ' ', $rest;
+ while ( my $line = <> ) {
+ my ($who, $rest) = split /:\s*/, $line, 2;
+ my @fields = split ' ', $rest;
$HoA{$who} = [ @fields ];
}
# calling a function that returns a list
- for $group ( "simpsons", "jetsons", "flintstones" ) {
+ for my $group ( "simpsons", "jetsons", "flintstones" ) {
$HoA{$group} = [ get_family($group) ];
}
# likewise, but using temps
- for $group ( "simpsons", "jetsons", "flintstones" ) {
+ for my $group ( "simpsons", "jetsons", "flintstones" ) {
@members = get_family($group);
$HoA{$group} = [ @members ];
}
@@ -439,26 +443,27 @@ X<hash of arrays> X<HoA>
$HoA{simpsons}[1] =~ s/(\w)/\u$1/;
# print the whole thing
- foreach $family ( keys %HoA ) {
+ foreach my $family ( keys %HoA ) {
print "$family: @{ $HoA{$family} }\n"
}
# print the whole thing with indices
- foreach $family ( keys %HoA ) {
+ foreach my $family ( keys %HoA ) {
print "family: ";
- foreach $i ( 0 .. $#{ $HoA{$family} } ) {
- print " $i = $HoA{$family}[$i]";
+ my $family_members = $HoA{$family};
+ foreach $i ( 0 .. $#{ $family_members } ) {
+ print " $i = $family_members->[$i]";
}
print "\n";
}
# print the whole thing sorted by number of members
- foreach $family ( sort { @{$HoA{$b}} <=> @{$HoA{$a}} } keys %HoA ) {
+ foreach my $family ( sort { @{$HoA{$b}} <=> @{$HoA{$a}} } keys %HoA ) {
print "$family: @{ $HoA{$family} }\n"
}
# print the whole thing sorted by number of members and name
- foreach $family ( sort {
+ foreach my $family ( sort {
@{$HoA{$b}} <=> @{$HoA{$a}}
||
$a cmp $b
@@ -472,7 +477,7 @@ X<array of hashes> X<AoH>
=head2 Declaration of an ARRAY OF HASHES
- @AoH = (
+ my @AoH = (
{
Lead => "fred",
Friend => "barney",
@@ -494,9 +499,9 @@ X<array of hashes> X<AoH>
# reading from file
# format: LEAD=fred FRIEND=barney
while ( <> ) {
- $rec = {};
+ my $rec = {};
for $field ( split ) {
- ($key, $value) = split /=/, $field;
+ my ($key, $value) = split /=/, $field;
$rec->{$key} = $value;
}
push @AoH, $rec;
@@ -512,7 +517,7 @@ X<array of hashes> X<AoH>
# calling a function that returns a key/value pair list, like
# "lead","fred","daughter","pebbles"
- while ( %fields = getnextpairset() ) {
+ while ( my %fields = getnextpairset() ) {
push @AoH, { %fields };
}
@@ -534,27 +539,29 @@ X<array of hashes> X<AoH>
$AoH[1]{lead} =~ s/(\w)/\u$1/;
# print the whole thing with refs
- for $href ( @AoH ) {
+ for my $href ( @AoH ) {
print "{ ";
- for $role ( keys %$href ) {
+ for my $role ( keys %$href ) {
print "$role=$href->{$role} ";
}
print "}\n";
}
# print the whole thing with indices
- for $i ( 0 .. $#AoH ) {
+ for my $i ( 0 .. $#AoH ) {
print "$i is { ";
- for $role ( keys %{ $AoH[$i] } ) {
- print "$role=$AoH[$i]{$role} ";
+ my $href = $AoH[$i];
+ for $role ( keys %{ $href } ) {
+ print "$role=$href->{$role} ";
}
print "}\n";
}
# print the whole thing one at a time
- for $i ( 0 .. $#AoH ) {
- for $role ( keys %{ $AoH[$i] } ) {
- print "elt $i $role is $AoH[$i]{$role}\n";
+ for my $i ( 0 .. $#AoH ) {
+ my $href = $AoH[$i];
+ for $role ( keys %{ $href } ) {
+ print "elt $i $role is $href->{$role}\n";
}
}
@@ -563,7 +570,7 @@ X<hash of hashes> X<HoH>
=head2 Declaration of a HASH OF HASHES
- %HoH = (
+ my %HoH = (
flintstones => {
lead => "fred",
pal => "barney",
@@ -584,48 +591,56 @@ X<hash of hashes> X<HoH>
# reading from file
# flintstones: lead=fred pal=barney wife=wilma pet=dino
+ LINES:
while ( <> ) {
- next unless s/^(.*?):\s*//;
- $who = $1;
- for $field ( split ) {
- ($key, $value) = split /=/, $field;
+ next LINES unless s/^(.*?):\s*//;
+ my $who = $1;
+ for my $field ( split ) {
+ my ($key, $value) = split /=/, $field;
$HoH{$who}{$key} = $value;
}
# reading from file; more temps
+ LINES:
while ( <> ) {
- next unless s/^(.*?):\s*//;
- $who = $1;
- $rec = {};
+ next LINES unless s/^(.*?):\s*//;
+ my $who = $1;
+ my $rec = {};
$HoH{$who} = $rec;
- for $field ( split ) {
- ($key, $value) = split /=/, $field;
+ for my $field ( split ) {
+ my ($key, $value) = split /=/, $field;
$rec->{$key} = $value;
}
}
# calling a function that returns a key,value hash
- for $group ( "simpsons", "jetsons", "flintstones" ) {
+ for my $group ( "simpsons", "jetsons", "flintstones" ) {
$HoH{$group} = { get_family($group) };
}
# likewise, but using temps
- for $group ( "simpsons", "jetsons", "flintstones" ) {
- %members = get_family($group);
+ for my $group ( "simpsons", "jetsons", "flintstones" ) {
+ my %members = get_family($group);
$HoH{$group} = { %members };
}
# append new members to an existing family
- %new_folks = (
+ my %new_folks = (
wife => "wilma",
pet => "dino",
);
- for $what (keys %new_folks) {
+ for my $what (keys %new_folks) {
$HoH{flintstones}{$what} = $new_folks{$what};
}
+ # Alternatively
+ {
+ my $family = $HoH{flintstones};
+ %{ $family } = (%{ $family }, %new_folks);
+ }
+
=head2 Access and Printing of a HASH OF HASHES
# one element
@@ -635,43 +650,52 @@ X<hash of hashes> X<HoH>
$HoH{simpsons}{lead} =~ s/(\w)/\u$1/;
# print the whole thing
- foreach $family ( keys %HoH ) {
+ foreach my $family ( keys %HoH ) {
print "$family: { ";
- for $role ( keys %{ $HoH{$family} } ) {
- print "$role=$HoH{$family}{$role} ";
+ my $family_record = $HoH{$family};
+ for my $role ( keys %{ $family_record } ) {
+ print "$role=$family_record->{$role} ";
}
print "}\n";
}
# print the whole thing somewhat sorted
- foreach $family ( sort keys %HoH ) {
+ foreach my $family ( sort keys %HoH ) {
print "$family: { ";
- for $role ( sort keys %{ $HoH{$family} } ) {
- print "$role=$HoH{$family}{$role} ";
+ my $family_record = $HoH{$family};
+ for my $role ( sort keys %{ $family_record } ) {
+ print "$role=$family_record->{$role} ";
}
print "}\n";
}
# print the whole thing sorted by number of members
- foreach $family ( sort { keys %{$HoH{$b}} <=> keys %{$HoH{$a}} } keys %HoH ) {
+ foreach my $family ( sort { keys %{$HoH{$b}} <=> keys %{$HoH{$a}} } keys %HoH ) {
print "$family: { ";
- for $role ( sort keys %{ $HoH{$family} } ) {
- print "$role=$HoH{$family}{$role} ";
+ my $family_record = $HoH{$family};
+ for my $role ( sort keys %{ $family_record } ) {
+ print "$role=$family_record->{$role} ";
}
print "}\n";
}
# establish a sort order (rank) for each role
- $i = 0;
- for ( qw(lead wife son daughter pal pet) ) { $rank{$_} = ++$i }
+ my %rank;
+ {
+ my $i = 0;
+ for my $key ( qw(lead wife son daughter pal pet) ) {
+ $rank{$_} = ++$i;
+ }
+ }
# now print the whole thing sorted by number of members
- foreach $family ( sort { keys %{ $HoH{$b} } <=> keys %{ $HoH{$a} } } keys %HoH ) {
+ foreach my $family ( sort { keys %{ $HoH{$b} } <=> keys %{ $HoH{$a} } } keys %HoH ) {
print "$family: { ";
+ my $family_record = $HoH{$family};
# and print these according to rank order
- for $role ( sort { $rank{$a} <=> $rank{$b} } keys %{ $HoH{$family} } ) {
- print "$role=$HoH{$family}{$role} ";
+ for my $role ( sort { $rank{$a} <=> $rank{$b} } keys %{ $family_record } ) {
+ print "$role=$family_record->{$role} ";
}
print "}\n";
}
@@ -685,7 +709,7 @@ X<record> X<structure> X<struct>
Here's a sample showing how to create and use a record whose fields are of
many different sorts:
- $rec = {
+ my $rec = {
TEXT => $string,
SEQUENCE => [ @old_values ],
LOOKUP => { %some_table },
@@ -697,24 +721,23 @@ many different sorts:
print $rec->{TEXT};
print $rec->{SEQUENCE}[0];
- $last = pop @ { $rec->{SEQUENCE} };
+ my $last = pop @ { $rec->{SEQUENCE} };
print $rec->{LOOKUP}{"key"};
- ($first_k, $first_v) = each %{ $rec->{LOOKUP} };
+ my ($first_k, $first_v) = each %{ $rec->{LOOKUP} };
- $answer = $rec->{THATCODE}->($arg);
- $answer = $rec->{THISCODE}->($arg1, $arg2);
+ my $answer = $rec->{THATCODE}->($arg);
+ my $second_answer = $rec->{THISCODE}->($arg1, $arg2);
# careful of extra block braces on fh ref
print { $rec->{HANDLE} } "a string\n";
- use FileHandle;
$rec->{HANDLE}->autoflush(1);
$rec->{HANDLE}->print(" a string\n");
=head2 Declaration of a HASH OF COMPLEX RECORDS
- %TV = (
+ my %TV = (
flintstones => {
series => "flintstones",
nights => [ qw(monday thursday friday) ],
@@ -755,14 +778,14 @@ many different sorts:
# sometimes it's easiest to do that
# here's a piece by piece build up
- $rec = {};
+ my $rec = {};
$rec->{series} = "flintstones";
$rec->{nights} = [ find_days() ];
- @members = ();
+ my @members;
# assume this file in field=value syntax
while (<>) {
- %fields = split /[\s=]+/;
+ my %fields = split /[\s=]+/;
push @members, { %fields };
}
$rec->{members} = [ @members ];
@@ -778,10 +801,10 @@ many different sorts:
# to an array of the kids' records without having duplicate
# records and thus update problems.
###########################################################
- foreach $family (keys %TV) {
- $rec = $TV{$family}; # temp pointer
- @kids = ();
- for $person ( @{ $rec->{members} } ) {
+ foreach my $family (keys %TV) {
+ my $rec = $TV{$family}; # temp pointer
+ my @kids;
+ for my $person ( @{ $rec->{members} } ) {
if ($person->{role} =~ /kid|son|daughter/) {
push @kids, $person;
}
@@ -803,16 +826,18 @@ many different sorts:
# both point to the same underlying anonymous hash table
# print the whole thing
- foreach $family ( keys %TV ) {
+ foreach my $family ( keys %TV ) {
+ my $family_record = $TV{$family};
print "the $family";
- print " is on during @{ $TV{$family}{nights} }\n";
+ print " is on during @{ $family_record->{nights} }\n";
print "its members are:\n";
- for $who ( @{ $TV{$family}{members} } ) {
+ for my $who ( @{ $family_record->{members} } ) {
print " $who->{name} ($who->{role}), age $who->{age}\n";
}
- print "it turns out that $TV{$family}{lead} has ";
- print scalar ( @{ $TV{$family}{kids} } ), " kids named ";
- print join (", ", map { $_->{name} } @{ $TV{$family}{kids} } );
+ print "it turns out that $family_record->{lead} has ";
+ my $kids = $family_record->{kids};
+ print scalar ( @{ $kids } ), " kids named ";
+ print join (", ", map { $_->{name} } @{ $kids } );
print "\n";
}
--
2.3.0
|
From @jkeenanOn Sat Feb 21 07:49:48 2015, shlomif@shlomifish.org wrote:
Shlomi, In some of the cases where you "extract repeating expressions into variables," I think that that extraction is not needed and just adds one more line to the code example. For example, under the section "Access and Printing of an ARRAY OF ARRAYS", we currently have this code example (blead, starting at line 387): ##### After your revision, this section would read: 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. I'm currently attuned to this because I've been refactoring one of my older CPAN distributions with the objective of speeding up its performance without changing the interface. I've found that eliminating unnecessary assignments to synthetic variables is an easy way to get a measurable (though usually modest) improvement in function performance. The other changes in this patch are, IMO, quite satisfactory. Thank you very much. |
The RT System itself - Status changed from 'new' to 'open' |
From @ap* James E Keenan via RT <perlbug-followup@perl.org> [2015-02-21 21:00]:
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 For the same reason I dislike the corresponding change in the HoA and OTOH, with the correspondence illustrated, I don’t have a problem with
Most are unobjectionable. There is one style change I strenuously object # reading from file (Repeat some two dozen times.) I cannot find a single case where this helps comprehension. It just adds The added `my`s, OTOH, are all benign. But in two cases their addition for $i ( 1 .. 10 ) { to for my $i ( 1 .. 10 ) { The added `my`s are fine, but consider the “after” version. If you have for my $i ( 1 .. 10 ) { Note that changing [ @tmp ] to \@tmp in the “before” version would be There is a likewise shortfall of editing further down, where while (<>) { becomes while (<>) { when it should become while (<>) { That’s all I can spot at a cursory glance. Regards, |
From @shlomifHi Aristotle and James, sorry for the late reply. On Sat Feb 21 14:34:24 2015, aristotle wrote:
I see - well, I'll wait for a consensus or veto here.
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.
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? |
From @rjbs* Shlomi Fish via RT <perlbug-followup@perl.org> [2015-02-25T04:27:55]
I am with Aristotle. While I'd make the change in product code, I think the
Here, I'm not with Aristotle except in the practical outcome. I nearly always
I had a flip through the history on that file and I couldn't find what you're Is your previous change relevant to that? If so, could you point it out to me? -- |
From @shlomifOn Wed Feb 25 19:17:45 2015, perl.p5p@rjbs.manxome.org wrote:
Very well, then, I'll change it back.
OK.
Sorry, that was an awkward phrasing on my part. I meant that I wanted to remove a paragraph which warned about doing exactly that and that it was considered still relevant, and was vetoed against. I'm referring to: [QUOTE] That's because my() is more of a run-time statement than it is a [/QUOTE] And now I was told to convert the code to do exactly that. So what should we do? Regards, -- Shlomi Fish |
From @rjbs* Shlomi Fish via RT <perlbug-followup@perl.org> [2015-02-26T02:38:21]
Thanks, this helped. Sorry for my delayed reply, I had this message open in a I agree: there are a couple things in conflict here. Here is my thinking: Tom's summary is: $AoA[$i] = [ @array ]; # usually best The question about whether to use \@array is about how certain you are that the I think perldsc is largely a document for beginners. Let's stick with [ @tmp ] Possibly your previous patch can be revisited to separate things out. I might -- |
From @jkeenanOn Tue, 03 Mar 2015 01:08:30 GMT, perl.p5p@rjbs.manxome.org wrote:
There wasn't sufficient consensus in the discussion in this RT to warrant going forward with any patch at this time. So I'm marking this ticket Rejected but we'll be open to a patch in a new ticket that takes into account the discussion herein. Thank you very much. -- |
@jkeenan - Status changed from 'open' to 'rejected' |
Migrated from rt.perl.org#123898 (status was 'rejected')
Searchable as RT123898$
The text was updated successfully, but these errors were encountered: