Code review request.

Discussion in 'Perl Misc' started by Michael Press, Mar 22, 2006.

  1. 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________________

    --
    Michael Press
    Michael Press, Mar 22, 2006
    #1
    1. Advertising

  2. Michael Press wrote:
    > 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
    --
    use Perl;
    program
    fulfillment
    John W. Krahn, Mar 22, 2006
    #2
    1. Advertising

  3. In article <rSjUf.238$%H.103@clgrps13>,
    "John W. Krahn" <> wrote:

    > Michael Press wrote:
    > > 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.


    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.

    > > %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?


    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.

    --
    Michael Press
    Michael Press, Mar 22, 2006
    #3
  4. Michael Press wrote:
    > In article <rSjUf.238$%H.103@clgrps13>,
    > "John W. Krahn" <> wrote:
    >
    >>Michael Press wrote:
    >>>
    >>> # 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
    --
    use Perl;
    program
    fulfillment
    John W. Krahn, Mar 23, 2006
    #4
  5. Michael Press <> wrote:
    > In article <rSjUf.238$%H.103@clgrps13>,
    > "John W. Krahn" <> wrote:
    >
    >> 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?


    --
    Tad McClellan SGML consulting
    Perl programming
    Fort Worth, Texas
    Tad McClellan, Mar 23, 2006
    #5
  6. In article <>,
    Tad McClellan <> wrote:

    > Michael Press <> wrote:
    > > In article <rSjUf.238$%H.103@clgrps13>,
    > > "John W. Krahn" <> wrote:
    > >
    > >> 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.

    --
    Michael Press
    Michael Press, Mar 23, 2006
    #6
  7. Michael Press

    Dave Weaver Guest

    Michael Press <> wrote:
    > 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.
    Dave Weaver, Mar 23, 2006
    #7
  8. Michael Press

    Adam Funk Guest

    On 2006-03-23, Dave Weaver <> wrote:

    > 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!
    Adam Funk, Mar 23, 2006
    #8
  9. Dave Weaver wrote:
    > Michael Press <> wrote:
    >>
    >> 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)


    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
    --
    use Perl;
    program
    fulfillment
    John W. Krahn, Mar 23, 2006
    #9
  10. Adam Funk <> wrote:
    > On 2006-03-23, Dave Weaver <> wrote:
    >
    >> With modern perls "use warnings" is better than -w

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



    The first sentence of the description in:

    perldoc warnings

    tells the difference.


    --
    Tad McClellan SGML consulting
    Perl programming
    Fort Worth, Texas
    Tad McClellan, Mar 23, 2006
    #10
  11. Michael Press

    Dave Weaver Guest

    John W. Krahn <> wrote:
    > Dave Weaver wrote:
    > > Michael Press <> wrote:
    > >>
    > >> 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)

    >
    > 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;
    Dave Weaver, Mar 24, 2006
    #11
  12. Michael Press

    Adam Funk Guest

    On 2006-03-23, Tad McClellan <> wrote:
    > Adam Funk <> wrote:
    >> On 2006-03-23, Dave Weaver <> wrote:
    >>
    >>> With modern perls "use warnings" is better than -w

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

    >
    > 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?
    Adam Funk, Mar 24, 2006
    #12
  13. Adam Funk <> wrote:
    > On 2006-03-23, Tad McClellan <> wrote:
    >> Adam Funk <> wrote:
    >>> On 2006-03-23, Dave Weaver <> wrote:
    >>>
    >>>> With modern perls "use warnings" is better than -w
    >>>
    >>> I didn't know that -- what's the difference?

    >>
    >> 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?



    Yes, that is correct.


    --
    Tad McClellan SGML consulting
    Perl programming
    Fort Worth, Texas
    Tad McClellan, Mar 24, 2006
    #13
  14. Michael Press

    Adam Funk Guest

    On 2006-03-24, Tad McClellan <> wrote:
    > Adam Funk <> wrote:
    >> On 2006-03-23, Tad McClellan <> wrote:
    >>> Adam Funk <> wrote:
    >>>> On 2006-03-23, Dave Weaver <> wrote:
    >>>>
    >>>>> With modern perls "use warnings" is better than -w
    >>>>
    >>>> I didn't know that -- what's the difference?
    >>>
    >>> 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?

    >
    >
    > Yes, that is correct.


    Thanks. You've sold me on the idea and I'll switch to "use warnings".
    Adam Funk, Mar 24, 2006
    #14
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. Code Review Request

    , Jun 30, 2005, in forum: Java
    Replies:
    2
    Views:
    510
    Andrew Thompson
    Jun 30, 2005
  2. P Kenter

    Request for code review

    P Kenter, May 28, 2004, in forum: C++
    Replies:
    4
    Views:
    355
    P Kenter
    Jun 2, 2004
  3. Ben Hanson

    Request for Code Review

    Ben Hanson, Jul 2, 2004, in forum: C++
    Replies:
    19
    Views:
    654
    Chris Gordon-Smith
    Jul 4, 2004
  4. Debashish Chakravarty

    request for code review

    Debashish Chakravarty, Nov 18, 2003, in forum: C Programming
    Replies:
    3
    Views:
    381
    Robert Stankowic
    Nov 18, 2003
  5. www
    Replies:
    51
    Views:
    1,459
Loading...

Share This Page