Re: Recursive function always returns 0

[liverpole]

Hi jpfarmer,

imp's answer is correct.  The whole point of recursion is, of course, that you have to propogate the answer back up the stack.  In your case, you were discarding the return value from unpack_row, so the calling subroutine (also unpack_row) didn't get anything to work with.

Here's a more-or-less working version of your code.  Note the return statement in the if(ref($hash) eq 'HASH') block.  You may have to modify what you're returning; as I'm not exactly sure what you're trying to do.

use strict;
use warnings;
use Data::Dumper;

# Prototype defined to avoid 'function not defined' warnings
# on compilation.
sub unpack_row{
    my $hash = shift;
    my @row = @_;
    if(ref($hash) eq 'HASH'){
        my $header = $hash->{'header'};
        my @results = ( );
        while(my ($key, $value) = each(%$hash)){
            # Save the results the way you need them...
            push @results, unpack_row($value, (@row, $key));
        }
        return (@results);  # ... or whatever you return
    } else {
        print Dumper((@row, $hash));
        return (@row, $hash);
    }
}

my $hash = {};
$hash->{'foo'}->{'bar'}->{'bat'} = 5;
print unpack_row($hash, ());

I'll point out that you don't need prototypes; just leave off the parentheses "( ... )" when you define the subroutine unpack_row and you'll be all set.

Also, you don't need to shift @_; @_ is the thing shifted by default.

[jpfarmer]

After a lot of help from liverpole, I think I've straightened this out. At the very least, it's giving me the output I was looking for.

Here's the final version for the curious:

sub unpack_row($@); 
sub unpack_row($@){
    my $hash = shift(@_);
    my @row = @_;

    my @stack;
    if(ref($hash) eq 'HASH'){
        my $header = $hash->{'header'};
        while(my ($key, $value) = each(%$hash)){
             push(@stack, unpack_row($value, (@row, $key)));
        }
    }
    else {
        return([@row, $hash]);
    }
    return @stack;
}

my $hash = {};
$hash->{'foo'}->{'bar'}->{'bat'} = 5;
$hash->{'foo'}->{'bar'}->{'bird'} = 2;
$hash->{'goo'}->{'tab'}->{'bird'} = 2;
print Dumper($hash);
print Dumper(unpack_row($hash, ()));

which generates the following output:

$VAR1 = {
          'goo' => {
                     'tab' => {
                                'bird' => 2
                              }
                   },
          'foo' => {
                     'bar' => {
                                'bat' => 5,
                                'bird' => 2
                              }
                   }
        };
$VAR1 = [
          'goo',
          'tab',
          'bird',
          2
        ];
$VAR2 = [
          'foo',
          'bar',
          'bat',
          5
        ];
$VAR3 = [
          'foo',
          'bar',
          'bird',
          2
        ];

[liverpole]

Good job!

I'm still going to recommend that you stop using prototypes.  Not using prototypes is a lesson I learned from my wise fellow monks.  This article is a "must read" on the subject.

You can also simplify your code somewhat.  The following are just suggestions, of course (ultimately you should do what makes the most sense for you), but here are some things to consider ...

Why not pull all your arguments out of @_ in one fell swoop, with my ($hash, @row) = @_?

When I see an if { ... } else { ... } where the first block is fairly long and the second very short, I often like to reverse them, so the short block is first.  Then you can often get rid of the else clause as well.  So instead of:

 if(ref($hash) eq 'HASH'){
        my $header = $hash->{'header'};
        while(my ($key, $value) = each(%$hash)){
             push(@stack, unpack_row($value, (@row, $key)));
        }
    }
    else {
        return([@row, $hash]);
    }

I prefer the more "balanced" looking:

if(ref($hash) ne 'HASH'){
    return([@row, $hash]);
}

my $header = $hash->{'header'};
while(my ($key, $value) = each(%$hash)){
    push(@stack, unpack_row($value, (@row, $key)));
}

Then I noticed you're not using $header, so you can remove that line.

Additionally, you can make the construction of @stack within the loop into a map statement, and then throw away @stack entirely, and just return the result from map.

Here's the entire program (with a few other simplifications):

use strict;
use warnings;
use Data::Dumper;

sub unpack_row {
    my ($hash, @row) = @_;
    return [ @row, $hash ] unless (ref $hash eq 'HASH');
    return map { unpack_row($hash->{$_}, @row, $_) } keys %$hash;
}

my $hash = {
    foo => { bar => { bat => 5, bird => 2 } },
    goo => { tab => { bird => 2 } },
};

print Dumper($hash);
print Dumper(unpack_row($hash, ()));

Isn't that a bit easier to follow?

[jpfarmer]

Ah, yes. That makes sense. I feel pretty dumb now.

Thank you very much for your help!