Improved with a hatchet?

Discussion in 'Perl Misc' started by Bill H, Aug 25, 2006.

  1. Bill H

    Bill H Guest

    I am using the following snippet of code to rotate a list starting at a
    random postion and it seems to run real slow (it works the way I want
    it to), can someone tell me how it can be improved?

    sub RotateList
    {
    my $results = "";
    my $srq = rand(@rotquest);
    my $i = 0;
    my $j = 0;
    for($i = 0;$i < @rotquest;$i++)
    {
    $j = $srq;
    $srq++;
    if ($srq >= @rotquest){$srq = 0;}
    $results .= $rotquest[$j];
    }
    return ($results);
    }


    And here is how I enter the routine:

    @rotquest = (
    qq~ <INPUT TYPE=CHECKBOX NAME="EMPLOYED1" VALUE="An
    automobile manufacturer or dealer ">An automobile manufacturer or
    dealer<BR>~,
    qq~ <INPUT TYPE=CHECKBOX NAME="EMPLOYED2" VALUE="A
    market research company">A market research company<BR>~,
    qq~ <INPUT TYPE=CHECKBOX NAME="EMPLOYED3" VALUE="A
    newspaper or TV station">A newspaper or TV station<BR>~,
    qq~ <INPUT TYPE=CHECKBOX NAME="EMPLOYED4" VALUE="An
    advertising agency">An advertising agency<BR>~);
    $q4 = &RotateList();
    $q4 .= qq~ <INPUT TYPE=CHECKBOX NAME="EMPLOYED5"
    VALUE="None of the above">None of the above\n~;

    I am not concerned that it isn't returning a list since I just then
    dump the results out to a web page.

    Bill H www.ts1000.us
     
    Bill H, Aug 25, 2006
    #1
    1. Advertising

  2. Bill H

    -berlin.de Guest

    Bill H <> wrote in comp.lang.perl.misc:
    > I am using the following snippet of code to rotate a list starting at a
    > random postion and it seems to run real slow (it works the way I want
    > it to), can someone tell me how it can be improved?
    >
    > sub RotateList
    > {
    > my $results = "";
    > my $srq = rand(@rotquest);
    > my $i = 0;
    > my $j = 0;
    > for($i = 0;$i < @rotquest;$i++)
    > {
    > $j = $srq;
    > $srq++;
    > if ($srq >= @rotquest){$srq = 0;}
    > $results .= $rotquest[$j];
    > }
    > return ($results);
    > }
    >
    >
    > And here is how I enter the routine:


    [example snipped, useless because the output is missing]

    > I am not concerned that it isn't returning a list since I just then
    > dump the results out to a web page.


    You could as well stringify the list and dump that. A routine that
    promises to rotate a list should return a list, not a string. Also,
    Perl has list manipulating functions that make the task very easy,
    but those return lists.

    While we're at it, a sub should also take parameters (the list to
    rotate, the number of steps to rotate), not and not rely on global
    variables or generate parameters internally. With these changes,
    plus some internal cleanup your routine becomes

    sub RotateList {
    my ( $srq, @rotquest) = @_;
    my @results;
    for( 1 .. @rotquest )
    {
    my $j = $srq;
    $srq++;
    if ($srq >= @rotquest){$srq = 0;}
    push @results, $rotquest[$j];
    }
    return (@results);
    }

    Your routine is slow because it moves each element at a time. Perl has
    builtins that deal with lists and parts of lists as a whole. Since
    rotation essentially means "cut the list in two and swap the parts"
    They are more suited to the task. Here are some examples:

    # using splice() to extract the parts and push() to swap them
    sub RotateList1 {
    my ( $srq, @rotquest) = @_;
    push @rotquest, splice @rotquest, 0, $srq;
    @rotquest;
    }

    # list slices to extract the parts, list concatenation to swap them
    sub RotateList2 {
    my ( $srq, @rotquest) = @_;
    ( @rotquest[ $srq .. $#rotquest], @rotquest[ 0 .. $srq - 1]);
    }

    # generate a list of swapped indices arithmetically. a list slice
    # establishes the new order
    sub RotateList3 {
    my ( $srq, @rotquest) = @_;
    @rotquest[ map $_ % @rotquest, $srq .. $srq + $#rotquest];
    }

    Anno
     
    -berlin.de, Aug 25, 2006
    #2
    1. Advertising

  3. Glenn Jackman wrote:
    > At 2006-08-25 10:29AM, "Bill H" wrote:
    > > it seems to run real slow (it works the way I want
    > > it to), can someone tell me how it can be improved?


    > sub RotateList
    > {
    > my @list = @_;
    > my $index = int rand $#list;
    > return @list[$index+1 .. $#list], @list[0 .. $index];
    > }


    Since we're optomising for speed I'd get rid of the redundant @list

    sub RotateList
    {
    my $index = int rand $#_;
    return @_[$index+1 .. $#list, 0 .. $index];
    }
     
    Brian McCauley, Aug 25, 2006
    #3
  4. Glenn Jackman wrote:

    > return splice(@list, $index+1), @list


    Whilst that's likely to be fast and efficient and give the right result
    it still gives me an uneasy feeling that it's behaviour is, strictly
    speaking, undefined and could, at least in principle, change in a
    future release.
     
    Brian McCauley, Aug 25, 2006
    #4
  5. Brian McCauley wrote:
    > Glenn Jackman wrote:
    > > At 2006-08-25 10:29AM, "Bill H" wrote:
    > > > it seems to run real slow (it works the way I want
    > > > it to), can someone tell me how it can be improved?

    >
    > > sub RotateList
    > > {
    > > my @list = @_;
    > > my $index = int rand $#list;
    > > return @list[$index+1 .. $#list], @list[0 .. $index];
    > > }

    >
    > Since we're optomising for speed I'd get rid of the redundant @list
    >
    > sub RotateList
    > {
    > my $index = int rand $#_;
    > return @_[$index+1 .. $#list, 0 .. $index];
    > }


    Oops, the OP wanted a string... (oh, and there was a typo above).

    sub RotateList
    {
    my $index = int rand $#_;
    return join '', @_[$index+1 .. $#_, 0 .. $index];
    }

    Actually rather than pass a list to subrouine it's usually more
    efficient to pass an arrayref.

    sub RotateList
    {
    my $list = shift;
    my $index = int rand $#$list;
    return join '', @$list[$index+1 .. $#$list, 0 .. $index];
    }
     
    Brian McCauley, Aug 25, 2006
    #5
  6. Bill H

    Bill H Guest

    Brian McCauley wrote:
    > Brian McCauley wrote:
    > > Glenn Jackman wrote:
    > > > At 2006-08-25 10:29AM, "Bill H" wrote:
    > > > > it seems to run real slow (it works the way I want
    > > > > it to), can someone tell me how it can be improved?

    > >
    > > > sub RotateList
    > > > {
    > > > my @list = @_;
    > > > my $index = int rand $#list;
    > > > return @list[$index+1 .. $#list], @list[0 .. $index];
    > > > }

    > >
    > > Since we're optomising for speed I'd get rid of the redundant @list
    > >
    > > sub RotateList
    > > {
    > > my $index = int rand $#_;
    > > return @_[$index+1 .. $#list, 0 .. $index];
    > > }

    >
    > Oops, the OP wanted a string... (oh, and there was a typo above).
    >
    > sub RotateList
    > {
    > my $index = int rand $#_;
    > return join '', @_[$index+1 .. $#_, 0 .. $index];
    > }
    >
    > Actually rather than pass a list to subrouine it's usually more
    > efficient to pass an arrayref.
    >
    > sub RotateList
    > {
    > my $list = shift;
    > my $index = int rand $#$list;
    > return join '', @$list[$index+1 .. $#$list, 0 .. $index];
    > }


    Brian, I used your 1st example and will stick with a list instead of
    making it a string. I have to make one change cause your example
    wouldn't start on the 1st element (ie not rotate it). Here is what I
    used:

    sub RotateList
    {
    my $index = int rand $#_;
    return join '', @_[$index .. $#_, 0 .. $index - 1];
    }


    Thanks to everyone for the help!

    Bill H
     
    Bill H, Aug 25, 2006
    #6
  7. Bill H

    Guest

    wrote:
    > At 2006-08-25 11:38AM, "Brian McCauley" wrote:
    > > Glenn Jackman wrote:
    > >
    > > > return splice(@list, $index+1), @list

    > >
    > > Whilst that's likely to be fast and efficient and give the right
    > > result it still gives me an uneasy feeling that it's behaviour is,
    > > strictly speaking, undefined and could, at least in principle, change
    > > in a future release.

    >
    > How is it undefined? The splice docs say:
    > Removes the elements designated by OFFSET and LENGTH
    > from an array, and replaces them with the elements
    > of LIST, if any. In list context, returns the
    > elements removed from the array.


    Is the second occurence of @list in the return statement evaluated before
    or after the splice is done to it? In other words, is the comma between
    splice(...),@list as sequence point or not?

    Xho

    --
    -------------------- http://NewsReader.Com/ --------------------
    Usenet Newsgroup Service $9.95/Month 30GB
     
    , Aug 25, 2006
    #7
  8. Bill H wrote:
    > I am using the following snippet of code to rotate a list starting at a
    > random postion and it seems to run real slow (it works the way I want
    > it to), can someone tell me how it can be improved?
    >
    > sub RotateList
    > {
    > my $results = "";
    > my $srq = rand(@rotquest);
    > my $i = 0;
    > my $j = 0;
    > for($i = 0;$i < @rotquest;$i++)
    > {
    > $j = $srq;
    > $srq++;
    > if ($srq >= @rotquest){$srq = 0;}
    > $results .= $rotquest[$j];
    > }
    > return ($results);
    > }
    >
    >
    > And here is how I enter the routine:
    >
    > @rotquest = (
    > qq~ <INPUT TYPE=CHECKBOX NAME="EMPLOYED1" VALUE="An
    > automobile manufacturer or dealer ">An automobile manufacturer or
    > dealer<BR>~,
    > qq~ <INPUT TYPE=CHECKBOX NAME="EMPLOYED2" VALUE="A
    > market research company">A market research company<BR>~,
    > qq~ <INPUT TYPE=CHECKBOX NAME="EMPLOYED3" VALUE="A
    > newspaper or TV station">A newspaper or TV station<BR>~,
    > qq~ <INPUT TYPE=CHECKBOX NAME="EMPLOYED4" VALUE="An
    > advertising agency">An advertising agency<BR>~);
    > $q4 = &RotateList();
    > $q4 .= qq~ <INPUT TYPE=CHECKBOX NAME="EMPLOYED5"
    > VALUE="None of the above">None of the above\n~;
    >
    > I am not concerned that it isn't returning a list since I just then
    > dump the results out to a web page.


    my @rotquest = (

    q~ <INPUT TYPE=CHECKBOX NAME="EMPLOYED1" VALUE="An automobile
    manufacturer or dealer ">An automobile manufacturer or dealer<BR>~,
    q~ <INPUT TYPE=CHECKBOX NAME="EMPLOYED2" VALUE="A market research
    company">A market research company<BR>~,
    q~ <INPUT TYPE=CHECKBOX NAME="EMPLOYED3" VALUE="A newspaper or TV
    station">A newspaper or TV station<BR>~,
    q~ <INPUT TYPE=CHECKBOX NAME="EMPLOYED4" VALUE="An advertising
    agency">An advertising agency<BR>~
    );

    print splice( @rotquest, rand @rotquest ), @rotquest, q~ <INPUT
    TYPE=CHECKBOX NAME="EMPLOYED5" VALUE="None of the above">None of the above\n~;



    John
    --
    use Perl;
    program
    fulfillment
     
    John W. Krahn, Aug 25, 2006
    #8
  9. Bill H

    Ben Morrow Guest

    Quoth :
    > wrote:
    > > At 2006-08-25 11:38AM, "Brian McCauley" wrote:
    > > > Glenn Jackman wrote:
    > > >
    > > > > return splice(@list, $index+1), @list
    > > >
    > > > Whilst that's likely to be fast and efficient and give the right
    > > > result it still gives me an uneasy feeling that it's behaviour is,
    > > > strictly speaking, undefined and could, at least in principle, change
    > > > in a future release.

    > >
    > > How is it undefined? The splice docs say:

    [snip]
    >
    > Is the second occurence of @list in the return statement evaluated before
    > or after the splice is done to it? In other words, is the comma between
    > splice(...),@list as sequence point or not?


    As perl is currently implemented, it is. The scalar and list comma
    operators are actually the same underneath (a generic evaluate-and-build
    -a-list op), and the main use of the comma op in scalar context is as a
    sequence point. Now, there's obviously no official spec for Perl that I
    can wave at you :), but I seriously doubt p5p would change something
    like that without a major amount of fuss.

    All of this only goes for Perl5, of course. Perl6 may well change
    things, but, well, that's kinda the point :).

    Ben

    --
    The cosmos, at best, is like a rubbish heap scattered at random.
    Heraclitus
     
    Ben Morrow, Aug 25, 2006
    #9
  10. Ben Morrow wrote:

    > As perl is currently implemented, it is. The scalar and list comma
    > operators are actually the same underneath (a generic evaluate-and-build
    > -a-list op), and the main use of the comma op in scalar context is as a
    > sequence point.


    That is true but to say that the list context comma is a sequence point
    is a gross over- simplification...

    my $foo = 1;
    print $foo,$foo++; # prints 21

    my @list = ('foo','bar');
    print @list,do { $_=uc for @list; @list }; # prints FOOBARFOOBAR

    It happens that the comma causes an array on its LHS to be resolved to
    a list of lvalues before the RHS is evaluated but it does not cause
    those lvalues to be resolved to rvalues.

    It would be a relatively small future change for the comma operator to
    leave the array unresolved.
     
    Brian McCauley, Aug 28, 2006
    #10
    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. Kieran Benton
    Replies:
    3
    Views:
    516
    Ray Cassick \(home\)
    Sep 11, 2003
  2. Franck

    improved ImageButton does it exist?

    Franck, Sep 7, 2005, in forum: ASP .Net
    Replies:
    1
    Views:
    383
    Bruce Barker
    Sep 7, 2005
  3. Jesper Nordenberg
    Replies:
    22
    Views:
    5,293
    Neal Gafter
    Apr 13, 2004
  4. Tony Morris
    Replies:
    1
    Views:
    2,709
    =?ISO-8859-1?Q?Daniel_Sj=F6blom?=
    Apr 6, 2004
  5. Andrew Cameron

    x01 - Improved!

    Andrew Cameron, Sep 15, 2003, in forum: HTML
    Replies:
    14
    Views:
    759
    Seth Honeywell
    Sep 16, 2003
Loading...

Share This Page