Anonymous Monk has asked for the wisdom of the Perl Monks concerning the following question:

Hello Monks

I am having a problem with the following bit of code. It will only print the first sequence and then seems to stop.

I want it to print the correct number of sequences and then push them onto an array

Here is the code I have so far 

$number_of_sequences=12;
$max_length=50;
$min_length=30;

for ($i=0; $i < $number_of_sequences;) {

    $aminoseq = genseq($aminoseq);
 print "$aminoseq\n\n";
 push (@set,$aminoseq);
}


sub genseq {

    $leg=randomlength();

for ($i=0; $i<$leg; $i++) {

    $seq.=randomaminoacid();
}
return $seq;
}

[download]

Thanks for your help in advance a newbie

Replies are listed 'Best First'.
Re: Running a subroutine a certain number of times
by dreadpiratepeter (Priest) on Aug 05, 2002 at 14:29 UTC
    Your bug is caused by serious scoping problems. Because you are not using any scoping or lexical variables. The $i in your outer look is the same variable as the $i in your subroutine.
    As long as $leg is greater than $number_of_sequences (and if it is generated using $min_length, it will be), the loop will terminate after one call.
    You probably wanted: 
    

$number_of_sequences=12;
$max_length=50;
$min_length=30;

my @set;
for (my $i=0; $i < $number_of_sequences;$i++) {
   my $aminoseq = genseq($aminoseq);
   print "$aminoseq\n\n";
   push (@set,$aminoseq);
}


sub genseq {

  my $leg=randomlength();
  my $seq;
  for (my $i=0; $i<$leg; $i++) {
    $seq.=randomaminoacid();
  }
return $seq;
}

    [download]
    use strict would have caught this and many other problems in your code. Incidently, if the three variables at the top of your code are constant, if would declare them as: 
    use const NUMBER_OF_SEQUENCES => 12;
use const MAX_LENGTH => 50;
use const MIN_LENGTH => 30;

    [download]
    That way it is clear that they are constants, and it is impossible to inadvertently change them.
    Read the Camel book for more information on scoping and lexical variables.
    Hope this helps,

    -pete
    "Pain heals. Chicks dig scars. Glory lasts forever."
[reply]
[d/l]
[select]

      Also, you almost certainly don't need those old C-style for loops. and actually, you don't need those $i variables at all.

      $number_of_sequences=12;
$max_length=50;
$min_length=30;

my @set;
foreach (0 .. $number_of_sequences) {
   my $aminoseq = genseq($aminoseq);
   print "$aminoseq\n\n";
   push (@set, $aminoseq);
}

sub genseq {

  my $leg = randomlength();
  my $seq;
  foreach (0 .. $leg) {
    $seq .= randomaminoacid();
  }
  return $seq;
}

      [download]
      or even 
      sub genseq {
  my $seq;

  $seq .= randomaminoacid() foreach 0 .. randomlength();

  return $seq;
}

      [download]
      --
      <http://www.dave.org.uk>

      "The first rule of Perl club is you do not talk about Perl club."
      -- Chip Salzenberg

[reply]
[d/l]
[select]
        I definitely agree. I just didn't want to take his code too far astray. Plus, it's really hard to demonstrate scoping problems while removing all the variables that are causing the scoping problems.

        -pete
        "Pain heals. Chicks dig scars. Glory lasts forever."
[reply]
Re: Running a subroutine a certain number of times
by Abigail-II (Bishop) on Aug 05, 2002 at 14:26 UTC
    Your $i isn't lexical. Hence, the $i you are using in the outer loop, is the same $i you are using in the subroutine. Hence, after exiting the subroutine, $i == $leg, which is probably equal or larger than $number_of_sequences.

    Use lexical loop variables that are local to just the loop. And for the first loop, you need to increment $i.

    Abigail

[reply]
[d/l]
[select]
please, PLEASE use strict!
by flocto (Pilgrim) on Aug 05, 2002 at 14:25 UTC

    If you were using strict, you could see that $i (in the main program) and $i (in the subroutine) are the same variables, since they're both global. Using strict would prevent this..

    Regards,
    -octo

[reply]
      I was waiting for someone to say that.

      No, use strict would not magically solve the problem. By putting use strict on top of the code, and my $i above the first loop, you would still have had the same problem: the loop variable is still shared.

      You solve the problem by putting my $i in the subroutine, or in the inner block. But you don't need use strict to do so.

      Abigail

[reply]
[d/l]
[select]
        True, but I find that getting people in the habit of using strict also gets them in the habit of scoping all their variables properly, so the mistake becomes far less likely to happen.
        But I agree, use strict is not a substitute for use your_brain.


        -pete
        "Pain heals. Chicks dig scars. Glory lasts forever."
[reply]
[d/l]
[select]

        I did not say that 'use strict' magically solves all your problems, but that this mistake would not have been as easy to happen.

        Regards,
        -octo

[reply]

Back to Seekers of Perl Wisdom