sub routine trouble

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

I am passing a hash and an array by reference into a sub routine and during test they printed out everything fine but when calling the sub routine and then checking the hash tables, there values never get updated. Can you please explain what I have done wrong in my sub routine?


sub_info(\%ca, \@ca);

sub_info(\%nv, \@nv);

@keys = ();
@keys = sort { $a cmp $b } (keys %ca);
foreach $key (@keys)
{
    $val = $ca{$key};
    print "$key $val\n";
}
 
@keys = ();
@keys = sort { $a cmp $b } (keys %nv);
foreach $key (@keys)
{
    $val = $nv{$key};
    print "$key $val\n";
}
 
exit 0;

sub upd_info ()
{
    $params1 = shift;
    %paramhash = %$params1;
 
    $params2 = shift;
    @paramarray = @$params2;
 
    while (($key, $val) = each(%paramhash))
    {
        $upd_flg = 0;
        foreach $item (sort { $a cmp $b } @paramarray)
        {
            if ($item =~ /^$key\s+(\w+)$/)
            {
                $upd_flg = $upd_flg + 1;
                $paramhash{$key} .= " $1";
                last;
            }
        }
        if ($upd_flg == 0)
        {
            $paramhash{$key} .= " NONE";
        }
}
[download]

Comment on sub routine trouble Download Code

Replies are listed 'Best First'.
Re: sub routine trouble by ccn (Vicar) on Dec 10, 2004 at 18:31 UTC
It's because you make a copy of an array and a hash `sub upd_info () { $params1 = shift; %paramhash = %$params1; # COPY!! $params2 = shift; @paramarray = @$params2; # COPY!!` [download] You may use `$params1->{$key}` and `$params2->[$index]` notation to achieve what you want	[reply] [d/l] [select]
Re: sub routine trouble by ikegami (Patriarch) on Dec 10, 2004 at 18:32 UTC
You're updating a copy of the hashes. Fix alternative 1: `sub upd_info { # Create non-reference aliases to arguments. our %paramhash; local paramhash = shift; our @paramarray; local paramarray = shift; ... keep rest as is ... }` [download] Fix alternative 2: `sub upd_info { my ($paramshash, $paramarray) = @_; while (($key, $val) = each(%$paramhash)) # added $ { $upd_flg = 0; foreach $item (sort { $a cmp $b } @$paramarray) # added $ { if ($item =~ /^$key\s+(\w+)$/) { $upd_flg = $upd_flg + 1; $paramhash->{$key} .= " $1"; # added -> last; } } if ($upd_flg == 0) { $paramhash->{$key} .= " NONE"; # added -> } }` [download] btw, you're gonna get yourself into trouble sooner or later if you don't tighten the scope of your local variables by using `my` (`my $key; my $val; my $item;`, etc) It's best if you use `use strict;`, which will rightfully complain about these variables.	[reply] [d/l] [select]
Re^2: sub routine trouble by tc1364 (Beadle) on Dec 10, 2004 at 19:14 UTC
It is a blessing to be able to ask someone of your ablities for some help. The code works great now. Thank you very much for your time and consideration regarding this subject matter.	[reply]
Re: sub routine trouble by Zaxo (Archbishop) on Dec 10, 2004 at 18:51 UTC
Two problems, one hidden by your order of definition. The hidden one is from the use of an empty prototype. Properly ordered, your code would fail when you gave arguments to the sub. As it is, with warnings on, you would see the "Too late for prototype. . ." message. When you say `%paramhash = %$params1;`, you are making a copy of the hash referenced by the argument. Manipulate directly through the reference to make your sub mutate the external structures. `sub upd_info { my ($hashref, $aryref) = @_; for my $key (keys %$hashref) { my $upd_flag = 0; for (sort @$aryref) { if ( /^\Q$key\E\s+(\w+)$/ ) { $upd_flag++; $hashref->{$key} .= $1; last; } } $upd_flag or $hashref->{$key} .= ' NONE'; } 1; }` [download] I've taken care to make the variables lexical. There is no reason to have all those internal variables lurking in your namespace. Using strict and warnings will improve your code. Not knowing your intent, I copied your program logic directly. After Compline, Zaxo	[reply] [d/l] [select]
Re: sub routine trouble by osunderdog (Deacon) on Dec 10, 2004 at 19:56 UTC
What everyone else said AND Use `my` to declare all variables and start your script with `use strict;` [download] It looks like all your variables are used, but you never know until you add `use strict;` *"Look, Shiny Things!"* is not a better business strategy than compatibility and reuse. OSUnderdog	[reply] [d/l] [select]
Re: sub routine trouble by Eimi Metamorphoumai (Deacon) on Dec 10, 2004 at 18:45 UTC
The real problem is that when you do `%paramhash = %$params1` you're getting a copy of the hash, and updating that copy doesn't update the original. So you'll have to keep a hashref, instead of a hash. Here's some code that should work for you. I've also modified a few other things, just for style purposes (for instance, `sort` defaults to an alphabetical sort, so there's no reason to pass the block to it). `sub upd_info { my ($hashref, $arrayref) = @_; for my $key (keys %{$hashref}) { my $upd_flg = 0; for my $item (sort @{$arrayref}) { if ($item =~ /^$key\s+(\w+)$/) { $upd_flg++; $hashref->{$key} .= " $1"; last; } } if ($upd_flg == 0) { $hashref->{$key} .= " NONE"; } } }` [download]	[reply] [d/l] [select]