Code review request.

[Michael Press]

Hello. I am reading in a line of permutation cycles,
preparatory to multiplying them. Want to parse the line
into tokens for linear scanning: permutation elements
and parentheses. Also want to initialize a hash whose
key element pairs are permutation elements. The initial
state of the hash is to be the identity map. I thought
I could make this code more tight or efficient, but
failed; particularly the map <- grep pipe. Will someone
comment on this code? Thanks for listening.

________________CUT_______________
#! /usr/bin/perl -w

sub pm;

while (<DATA>) { pm $_ }

sub pm
{
my $z;
my @line;
my %p;

# Parse the cycles, and initialize the permutation map.
$z = shift (@_);
@line = $z =~ m/(\w+|[()])/g;
%p = map { $_ => $_ } grep { m/\w+/} @line;

printf ("%s\n", join ':', @line), "\n";
while (( $key, $value) = each (%p)) { printf "%7s%7s\n", $key, $value}
}

__END__
(99)(0)(3)(1 4 2 6)(5 10 7)(8 9 )(1 2 4 8 9 3 6 )(5 7)(0 2 )(1 5 99)(3 10)(4 7 8 )(6)(9)
________________CUT________________

 

[John W. Krahn]

Hello. I am reading in a line of permutation cycles,
preparatory to multiplying them. Want to parse the line
into tokens for linear scanning: permutation elements
and parentheses. Also want to initialize a hash whose
key element pairs are permutation elements. The initial
state of the hash is to be the identity map. I thought
I could make this code more tight or efficient, but
failed; particularly the map <- grep pipe. Will someone
comment on this code? Thanks for listening.

________________CUT_______________
#! /usr/bin/perl -w

sub pm;

while (<DATA>) { pm $_ }

Put the code inside the while loop instead of calling a subroutine.

sub pm
{
my $z;
my @line;
my %p;

Define and declare the variables at the same time.

# Parse the cycles, and initialize the permutation map.
$z = shift (@_);
@line = $z =~ m/(\w+|[()])/g;

You don't need the capturing parentheses in the pattern.

%p = map { $_ => $_ } grep { m/\w+/} @line;

printf ("%s\n", join ':', @line), "\n";

Use print instead of printf. The ', "\n"' at the end is not doing anything so
why is it there?

while (( $key, $value) = each (%p)) { printf "%7s%7s\n", $key, $value}
}

__END__
(99)(0)(3)(1 4 2 6)(5 10 7)(8 9 )(1 2 4 8 9 3 6 )(5 7)(0 2 )(1 5 99)(3 10)(4 7 8 )(6)(9)

sub pm {
my %p = map { /\w/ ? ( $_ => $_ ) : () } my @line = $_[0] =~ /\w+|[()]/g;

print join( ':', @line ), "\n";

while ( my ( $key, $value ) = each %p ) {
printf "%7s%7s\n", $key, $value;
}
}





John

 

[Michael Press]

Put the code inside the while loop instead of calling a subroutine.

No. This is example code.
I use the subroutine like this:

my $alpha = "(99)(0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22)";
my $beta = "(99)(0)(3 6 12 1 2 4 8 16 9 18 13)(15 7 14 5 10 20 17 11 22 21 19)";
my $gamma = "(99 0)(1 22)(2 11)(3 15)(4 17)(5 9)(6 19)(7 13)(8 20)(10 16)(12 21)(14 18)";
my $delta = "(99)(0)(3)(15)(1 18 4 2 6)(5 21 20 10 7)(8 16 13 9 12)(11 19 22 14 17)";
my $x;

print "(delta alpha^11)^5 has shape 1^6 3^6\n";
$x = ($delta . $alpha x 11) x 5;
pm $x;
print "\n";

print "beta = alpha^5 gamma alpha^5 gamma alpha^14 gamma alpha^18 \n";
$x = $alpha x 5 . $gamma . $alpha x 5 . $gamma . $alpha x 14 . $gamma . $alpha x 18;
pm $x;
$x = $beta;
pm $x;
print "\n";
....

sub pm
{
my $z;
my @line;
my %p;

Define and declare the variables at the same time.
Yes.

# Parse the cycles, and initialize the permutation map.
$z = shift (@_);
@line = $z =~ m/(\w+|[()])/g;

You don't need the capturing parentheses in the pattern.

Dang. Where is the specification for this? Examples in
Programming Perl use capturing parentheses.

Use print instead of printf. The ', "\n"' at the end is not doing anything so
why is it there?

Cut and paste error when editing for submission to this forum.
It runs, but I did not see the problem.

while (( $key, $value) = each (%p)) { printf "%7s%7s\n", $key, $value}
}

__END__
(99)(0)(3)(1 4 2 6)(5 10 7)(8 9 )(1 2 4 8 9 3 6 )(5 7)(0 2 )(1 5 99)(3 10)(4 7 8 )(6)(9)

sub pm {
my %p = map { /\w/ ? ( $_ => $_ ) : () } my @line = $_[0] =~ /\w+|[()]/g;

print join( ':', @line ), "\n";

while ( my ( $key, $value ) = each %p ) {
printf "%7s%7s\n", $key, $value;
}
}

Thank you. Use of $_[0] noted; `each %p' noted.
I still do not use `():?' enough.

 

[John W. Krahn]

# Parse the cycles, and initialize the permutation map.
$z = shift (@_);
@line = $z =~ m/(\w+|[()])/g;

You don't need the capturing parentheses in the pattern.

Dang. Where is the specification for this? Examples in
Programming Perl use capturing parentheses.

perldoc perlop
m/PATTERN/cgimosx
/PATTERN/cgimosx
[snip]

The "/g" modifier specifies global pattern matching--that is,
matching as many times as possible within the string. How it
behaves depends on the context. In list context, it returns a
list of the substrings matched by any capturing parentheses in
the regular expression. If there are no parentheses, it
returns a list of all the matched strings, as if there were
parentheses around the whole pattern.



John

 

[Tad McClellan]

Michael Press wrote:

@line = $z =~ m/(\w+|[()])/g;

You don't need the capturing parentheses in the pattern.

Dang. Where is the specification for this?

in perlop:

The C</g> modifier specifies global pattern matching--that is,
matching as many times as possible within the string. How it behaves
depends on the context. In list context, it returns a list of the
substrings matched by any capturing parentheses in the regular
expression. If there are no parentheses, it returns a list of all
the matched strings, as if there were parentheses around the whole
pattern.

Examples in
Programming Perl use capturing parentheses.

Examples of m//g in a list context where you want to capture
the entire pattern?

 

[Michael Press]

Michael Press wrote:

@line = $z =~ m/(\w+|[()])/g;

You don't need the capturing parentheses in the pattern.

Dang. Where is the specification for this?

in perlop:

The C</g> modifier specifies global pattern matching--that is,
matching as many times as possible within the string. How it behaves
depends on the context. In list context, it returns a list of the
substrings matched by any capturing parentheses in the regular
expression. If there are no parentheses, it returns a list of all
the matched strings, as if there were parentheses around the whole
pattern.

Examples in
Programming Perl use capturing parentheses.

Examples of m//g in a list context where you want to capture
the entire pattern?

No example looks like what I wrote. I put in the
parentheses myself based on examples like

($a, $b) = /(\w+) (\w+)/;

When JK corrected me I did not infer that Programming Perl
had misled me. And on page 151 (third edition) we read

"If there are no capturing parentheses within the /g
pattern, then the complete matches are returned."

Thanks.

 

[Dave Weaver]

Hello. I am reading in a line of permutation cycles,
preparatory to multiplying them. Want to parse the line
into tokens for linear scanning: permutation elements
and parentheses. Also want to initialize a hash whose
key element pairs are permutation elements. The initial
state of the hash is to be the identity map. I thought
I could make this code more tight or efficient, but
failed; particularly the map <- grep pipe. Will someone
comment on this code? Thanks for listening.

________________CUT_______________
#! /usr/bin/perl -w

With modern perls "use warnings" is better than -w
Also, remember to "use strict".

sub pm;

while (<DATA>) { pm $_ }

A matter of personal taste, but I'm not keen on sub pre-declaration
(I just think it adds avoidable noise) . I'd either define the
function before it's used:

sub pm {...}

while (<DATA>) { pm $_ }

or use parentheses when calling the sub to avoid the need for
pre-declaration:

while (<DATA>) { pm($_) }

sub pm {...}

sub pm

A horrible name for a subroutine - I can guess from your description
above that it means "permute" (or something similar) but a more
descriptive name will greatly aid future code maintenance.

{
my $z;
my @line;
my %p;

More horrible variable names! z and p mean nothing (to me). @line doesn't
seem to be holding lines, so its name is misleading.

Also, your code would be more readable (and shorter) if you declared
the variables where you first use them (see below)

# Parse the cycles, and initialize the permutation map.
$z = shift (@_);

my $line = shift;

@line = $z =~ m/(\w+|[()])/g;

my @tokens = $line =~ m/(\w+|[()])/g;

%p = map { $_ => $_ } grep { m/\w+/} @line;

my %p = map { $_ => $_ } grep { m/\w+/} @tokens;

printf ("%s\n", join ':', @line), "\n";

'printf (...) interpreted as function at - line 18.'

Why enable warnings, only to ignore them?

printf "%s\n", join(':', @tokens), "\n"; #(untested)

while (( $key, $value) = each (%p)) { printf "%7s%7s\n", $key, $value}

You don't declare $key/$value ("use strict" would have caught this).
Another use of variable names that are too generic and thus make code
maintenance difficult.

HTH.

 

[Adam Funk]

With modern perls "use warnings" is better than -w

I didn't know that -- what's the difference?

Also, remember to "use strict".

Definitely useful advice!

 

[John W. Krahn]

'printf (...) interpreted as function at - line 18.'

Why enable warnings, only to ignore them?

printf "%s\n", join(':', @tokens), "\n"; #(untested)

You have a somewhat similar problem to the OP's code in that the ', "\n"' at
the end is not doing anything (there is no format in the format string that
uses that value.)



John

 

[Tad McClellan]

I didn't know that -- what's the difference?

The first sentence of the description in:

perldoc warnings

tells the difference.

 

[Dave Weaver]

You have a somewhat similar problem to the OP's code in that the ', "\n"' at
the end is not doing anything (there is no format in the format string that
uses that value.)

Oops.
Thank goodness for the "(untested)" caveat. :-/

You're quite right. It should be:

printf "%s\n\n", join(':', @tokens);

or, if you have a distaste for parentheses (which I think, in this
case, help to make things clearer):

printf "%s\n\n", join ':', @tokens;

 

[Adam Funk]

The first sentence of the description in:
perldoc warnings
tells the difference.

--> The "warnings" pragma is a replacement for the command line
--> flag "-w", but the pragma is limited to the enclosing block,
--> while the flag is global. See perllexwarn for more
--> information.

At first, I thought that meant that putting "use warnings" at the
beginning (more or less right after "#!/usr/bin/perl") of a
free-standing Perl program would have the same effect as putting "-w"
on the first line, but on reflection I think that "use warnings"
wouldn't percolate down into module code used by the main program --
is that correct?

 

[Tad McClellan]

--> The "warnings" pragma is a replacement for the command line
--> flag "-w", but the pragma is limited to the enclosing block,
--> while the flag is global. See perllexwarn for more
--> information.

At first, I thought that meant that putting "use warnings" at the
beginning (more or less right after "#!/usr/bin/perl") of a
free-standing Perl program would have the same effect as putting "-w"
on the first line, but on reflection I think that "use warnings"
wouldn't percolate down into module code used by the main program --
is that correct?

Yes, that is correct.

 

[Adam Funk]

Yes, that is correct.

Thanks. You've sold me on the idea and I'll switch to "use warnings".