CODEWITHSUNDEEP
perlhashsubroutine
Ramy
Ramy

Reputation: 21261

how to pass a hash along with other arguments to a subroutine in perl?

my code:

$self->commitToPerforce($network_name[0], {"size_of_table=>$row->{numRecords}"});

sub commitToPerforce {
    my $self = shift;
    my $network = shift;
    my %metadata = shift;

    open my $FH, ">$local_metadata_file" || die "Could not open file $local_metadata_file";
    print $FH Dumper(%metadata);
    close $FH;
}

problem is, this is what's getting into the file:

$VAR1 = 'HASH(0x320adf0)';
$VAR2 = undef;

not what I'm looking for.

Also tried this:

print $FH Dumper(\%metadata);

which just gives me this:

$VAR1 = {
      'HASH(0x223cea0)' => undef
    };

I want the contents of the hash. Not the reference to the hash.

getting closer now:

my $hash = {"size_of_table=>$row->{numRecords}"};
$self->commitToPerforce($network_name[0], $hash);

   open( my $FH, ">", $local_metadata_file ) || die "Could not open file $local_metadata_file";
print $FH Dumper($metadata);
close $FH;

results in:

 $VAR1 = {
      'size_of_table=>0' => undef
    };

what's with the 'undef'??

oh. i see it now. my hash should not be a single string.

and just because there's no where else for me to gripe: why is it a good idea to spend so much time thinking about my data structures in this way?

Upvotes: 0

Views: 154

Answers (2)

Borodin
Borodin

Reputation: 126722

Here are some comments on your code

  • I hope the call to commit_to_perforce is in a different file from the method definition?

  • It is often much clearer to define a hash parameter separately and then pass a reference to it, rather than passing an anonymous hash directly in the parameter list

  • Within a method, it is usually best to shift the vlaue of $self off the parameter list, and then do a list assignment for the rest of the parameters

  • You should use the three-parameter form of open, and either check that it succeeded with a die string that includes $! to give the reason for the failure, or just use autodie.

    Because of the precedence of the || operator, your code checks the truth of the string ">$local_metadata_file" instead of the return value from open. You can either use the low-precedence or operator instead or put parentheses around the parameters to open

  • It is common practice to reserve upper case letters for global identifiers, such as the package Dumper. Local variables should normally be named using lower-case letters, numbers, and underscore

Taking all of that into account, here's how your code should look

my %meta = (
  size_of_table => $row->{numRecords},
);

$self->commit_to_perforce($network_name[0], \%meta);


sub commit_to_perforce {
  my $self = shift;
  my ($network, $metadata) = @_;

  open my $fh, '>', $local_metadata_file
      or die "Could not open file '$local_metadata_file' for output: $!";
  print $fh Dumper $metadata;
  close $fh;
}

I hope this helps

Upvotes: 3

Andy Lester
Andy Lester

Reputation: 93636

First problem: You are passing in a hash reference to the function (which is the right way to do it), but you're treating is as a hash inside the function.

Second problem: You also apparently don't have use warnings turned on because you should have received this warning:

Odd number of elements in hash assignment at programname.pl line X

So, first thing, go put use warnings; at the top of your program.

Third problem: You have a bug with your open because it will never fail, due to operator precedence. This statement:

open my $FH, ">$local_metadata_file" || die "Could not open file $local_metadata_file";

is effectively:

open my $FH, (">$local_metadata_file" || die "Could not open file $local_metadata_file");

So the ">$local_metadata_file" will always be true, and always evaluate to that string, and then nothing checks the return code of open. While you're at it, change to modern 3-argument open. Change to either

open my $FH, ">", $local_metadata_file or die "Could not open file $local_metadata_file";

or

open( my $FH, ">", $local_metadata_file" ) || die "Could not open file $local_metadata_file";

Roll it all up into this function:

sub commitToPerforce {
    my $self = shift;
    my $network = shift;
    my $metadata = shift; # Hash reference, not a hash.

    open my $FH, ">", $local_metadata_file or die "Could not open file $local_metadata_file";
    print $FH Dumper(%{$metadata});
    close $FH;
}

You can also pass a hashreference to Dumper and it will treat it as a hash rather than just a flattened list of key/value pairs, which is what happens when you pass in a hash.

    print $FH Dumper($metadata);

Learn about references. They are crucial to using Perl effectively. Run perldoc perlreftut to read the tutorial that comes with Perl.

EDIT

I just noticed the other thing you're doing wrong. This line

{"size_of_table=>$row->{numRecords}"};

is building a one-element hash with a string for a key, and no value. What you really want is

{ size_of_table => $row->{numRecords} };

Now you have a one-element hash that has a single key size_of_table that refers to the numeric value that lives in $row->{numRecords}, whatever that may be.

Upvotes: 1

Related Questions