What's the mistake in my recursive subroutine?

Question

This subroutine generates string combinations of the letters using the letters from A to the Mth letter of the Alphabet with length N.

sub genString
{
   my($m,$n,$str,$letter,$temp,$i) = @_;
   if($n == 0){
      $letter = chr(ord("A")+($i+=1));
       if($temp == 1){ print "$str\n"; }
       else{
          for($j = 0 ; $j < temp-1 ; $j++){
             if(ord(substr($str,$j,1)) < ord(substr($str,$j+1,1))){$do_print = 1;}
             else{
                $do_print = 0;
                break;
             }
          }
       if($do_print == 1){ print "$str\n"; }
       }
   }
   else{
       for($j = ord($letter) ; $j < ord($letter)+$m ; $j++){
           genString($m,$n-1,$str.chr($j),$letter,$temp,$i);
      }
   }
}
&genString($m,$n,$str,"A",$n,0);

Example: Input: M=4; N=3; Output: ABC ABD ACD BCD

I tried similar to this in Ruby and it works, but in Perl, it's an infinite loop, and I don't know why. I'm new here in Perl. What should I do? (Sorry if my code is kinda lengthy)

http://stackoverflow.com/questions/635768/how-can-i-generate-all-permutations-of-an-array-in-perl, reference to [perlfaq](http://learn.perl.org/faq/perlfaq4.html#How-do-I-permute-N-elements-of-a-list-) — TLP, Aug 24 '12 at 19:30
And there is no `break` in Perl, you most likely want to use `last` there. — matthias krull, Aug 24 '12 at 19:32
Please make sure your code can compile before posting it here. See `perldoc perlrun` and read about the -c switch. — shawnhcorey, Aug 24 '12 at 22:53

score 3 · Answer 1 · answered Aug 24 '12 at 18:46

Please always use use strict; and use warnings; in your code, especially when posting code and asking for help. Also always declare local variables with my.

In this case even without having tried it I'm pretty sure something like $j referring to a global variable is causing you a lot of headache -- something use strict would have caught.

ysth · Answer 2 · 2012-08-24T20:20:55.180

3

By default, variables are globals in perl (though undeclared and unqualified use of them will be prevented by use strict). For your recursion to work, you'll need to make some of them lexical, for instance, changing:

for($j = 0 ; $j < temp-1 ; $j++){

to

for (my $j = 0; $j < $temp-1; $j++) {

or better yet, just

for my $j (0..$temp-2) {

edited Aug 24 '12 at 20:20

answered Aug 24 '12 at 19:06

ysth

96,171
6
121
214

score 1 · Answer 3 · answered Aug 24 '12 at 19:13

Your code is very hard to read. I can't understand the algorithm, and I don't see the purpose of so many parameters to the subroutine, especially $temp which doesn't appear to change, and you don't say what its initial value is set to in the outermost call.

This code appears to do what you want, with a similar algorithm

use strict;
use warnings;

genString(4, 3);

sub genString {

   my ($m, $n, $str, $i) = @_;

   if ($n == 0) {
     print $str, "\n";
   }
   else {
     for my $off ($i // 0 .. $m - $n) {
       $str //= '';
       genString($m, $n-1, $str.chr(ord('A') + $off), $off+1);
     }
   }
}

output

ABC
ABD
ACD
BCD

What's the mistake in my recursive subroutine?

3 Answers3