Passing an array to a sub routine

[Bill H]

I am using the following 2 subroutines to load an array from a file and
save it. The LoadDataFile routine works correctly, but the SaveDataFile
routine only saves the last item in the array. The file that is read
from is straight text in the format of:

ITEM1\tVALUE1
ITEM2\tVALUE2

etc. The \t is a tab.

I call the LoadDataFile with this:

%myarray = &LoadDataFile("this is the file name");

and it loads the %myarray with the correct values.

I call the SaveDataFile with this:

&SaveDataFile("this is the filename",%myarray);

But it only saves the last entry. Am I doing somethign wrong in the
call or the SaveDataFile routine?

Here are the routines:

sub LoadDataFile
{
my $filename = shift;
my $temp = "";
my @dbf = ();
my %ldf_dbf = ();
open(LDF_LPFILE,$filename);
flock(LDF_LPFILE,2);
@LDF_LPFILE = <LDF_LPFILE>;
close(LDF_LPFILE);
foreach $temp (@LDF_LPFILE)
{
chop $temp;
@dbf = split(/\t/,$temp);
$ldf_dbf{$dbf[0]} = $dbf[1];
}
return (%ldf_dbf);
}

sub SaveDataFile
{
my $filename = shift;
my %this_dbf = shift;
my $temp = "";
open(LDF_LPFILE,">$filename");
flock(LDF_LPFILE,2);
foreach $temp (keys(%this_dbf))
{
print LDF_LPFILE "$temp\t$this_dbf{$temp}\n";
}
close(LDF_LPFILE);
return;
}

Any help is appreciated

Bill H

 

[Paul Lalli]

I am using the following 2 subroutines to load an array from a file and
save it. The LoadDataFile routine works correctly, but the SaveDataFile
routine only saves the last item in the array. The file that is read
from is straight text in the format of:

ITEM1\tVALUE1
ITEM2\tVALUE2

etc. The \t is a tab.

I call the LoadDataFile with this:

%myarray = &LoadDataFile("this is the file name");

Why do you have a hash variable named "myarray"? That's asking for
maintainability issues.

Why are you using the & to call the subroutine? Do you know what
additional features that enables? Do you want those features? 99% of
the time, the answer is no.

Why aren't you declaring %myarray with my? Did you do it elsewhere
without showing us, or are you not enabling strict (and I'm guessing
warnings as well)?

Have you read the Posting Guidelines for this group?

and it loads the %myarray with the correct values.

I call the SaveDataFile with this:

&SaveDataFile("this is the filename",%myarray);

But it only saves the last entry. Am I doing somethign wrong in the
call
Yes.

or the SaveDataFile routine?

Yes.

Arrays and hashes "flatten" when placed in a list. You did not pass a
hash to SaveDataFile. You passed a list of 2x+1 values, where x is the
number of key/value pairs in the hash %myarray.

It's possible this is what you want. It's also possible that you might
be able to recover from this mistake in the subroutine definition.
However, in the general case, do not pass a hash - pass a refernce to
that hash:

SaveDataFile("this is a filename", \%myarray);

sub SaveDataFile
{
my $filename = shift;
my %this_dbf = shift;

This is why the Posting Guidelines tell you to enable warnings when
developing your code. If you had done so, Perl would have told you
what you did wrong:
Odd number of elements in hash assignment
shift() removes and returns the *first* element from the array. It
does not return all the remaining elements. You were trying to assign
%this_dbf to be the first element that was passed into the function
when %myarray was flattened.

If you follow my advice above and instead pass a hash reference, change
this to:
my $this_dbf_ref = shift;
and then de-reference this reference as needed.

Otherwise, assign %this_dbf to all the remaining elements in the array
@_:
my %this_dbf = @_;

Note that this will work only because %myarray happened to be the last
thing passed into the subroutine, so all its elements were at the end.
This will not work in the general case.

my $temp = "";
open(LDF_LPFILE,">$filename");

ALWAYS check to make sure the open succeeded before continuing!!
Also, use the three-argument form of open(), and lexical filehandles
instead of global barewords:
open my $LDF_LPFILE, '>', $filename or die "Cannot open '$filename':
$!";


Read more about references in:
perldoc perlreftut

Paul Lalli

 

[Brian McCauley]

Subject: Passing an array to a sub routine

Your question is Frequently Asked: How can I pass/return a {Function,
FileHandle, Array, Hash, Method, Regex}?

Please refrain from posting FAQs.

I am using the following 2 subroutines to load an array from a file and
save it. The LoadDataFile routine works correctly, but the SaveDataFile
routine only saves the last item in the array. The file that is read
from is straight text in the format of:

ITEM1\tVALUE1
ITEM2\tVALUE2

etc. The \t is a tab.

I call the LoadDataFile with this:

%myarray = &LoadDataFile("this is the file name");

Why is that & in there? Do you know what it does? If not then remove
it.

It is very unlikely that you really want to replace the contents of an
existing variable %myarray with the return value of the function. It
is far more likely that this is where you want %myarray to come into
being.

In Perl we don't usually use the word "array" to refer to associative
array - we use the (pendantically less correct term) "hash".

my %myhash = LoadDataFile("this is the file name");

and it loads the %myarray with the correct values.

I call the SaveDataFile with this:

&SaveDataFile("this is the filename",%myarray);

But it only saves the last entry. Am I doing somethign wrong in the
call or the SaveDataFile routine?
Yes.


Here are the routines:

sub LoadDataFile
{
my $filename = shift;
my $temp = "";
my @dbf = ();
my %ldf_dbf = ();

Nasty case of premature declaration you've got there. Also the
initializations are redundant.

open(LDF_LPFILE,$filename);

You forgot to make the file handle local.

In recent Perl a lexical filehandle would be neater.

You forgot to check for errors.

The 2-arg open is largely a legacy (mis-)feature. All new code should
use the 3-arg one (IMNSHO).

open( my $ldf_lpfile, '<',$filename) or die "$filename: $!";

flock(LDF_LPFILE,2);

You forgot to check for errors.

@LDF_LPFILE = <LDF_LPFILE>;

There is no reason to slurp. If you want to precess the data a line at
a time then read it a line at a time.

close(LDF_LPFILE);

If you'd localized the handle there'd be no need to explicitly close()
except to check for errors.

foreach $temp (@LDF_LPFILE)
{
chop $temp;
@dbf = split(/\t/,$temp);
$ldf_dbf{$dbf[0]} = $dbf[1];

Always use the most natural representation of things unless there is a
reason to so otherwise. $dbf[0] and $dbf[1] are natually two
independant scalars, not the elements of an array.

}
return (%ldf_dbf);

See FAQ.

}

sub SaveDataFile
{
my $filename = shift;
my %this_dbf = shift;

There is a problem wilth this line that Perl would have told you about.

Did you perhaps "forget" to enable warnings?

If you change 'shift' to '@_' then I suspect your orginal code would
work - but it would still be far from optimal.

 

[Anno Siegel]

I am using the following 2 subroutines to load an array from a file and

You say array, but later you're using a hash. In Perl, these are very
different data structures.

save it. The LoadDataFile routine works correctly, but the SaveDataFile
routine only saves the last item in the array. The file that is read
from is straight text in the format of:

ITEM1\tVALUE1
ITEM2\tVALUE2

etc. The \t is a tab.

I call the LoadDataFile with this:

%myarray = &LoadDataFile("this is the file name");

and it loads the %myarray with the correct values.

I call the SaveDataFile with this:

&SaveDataFile("this is the filename",%myarray);

But it only saves the last entry. Am I doing somethign wrong in the
call or the SaveDataFile routine?

Here are the routines:

sub LoadDataFile
{
my $filename = shift;
my $temp = "";
my @dbf = ();
my %ldf_dbf = ();

Declare variables as close as possible to their first use, preferably
*with* their first use.

open(LDF_LPFILE,$filename);

Always check the return value of open().

flock(LDF_LPFILE,2);

Why are you requesting a write lock? You're only reading. Do you need file
locking at all?

@LDF_LPFILE = <LDF_LPFILE>;
close(LDF_LPFILE);
foreach $temp (@LDF_LPFILE)

Don't introduce variables with nondescript names like $tmp. Perl has
built-in variables with nondescript names like $_. Use that, other
Per programmers will understand you better. Or use a meaningful name.

{
chop $temp;

Use chomp(), not chop(). It will work right in case the file is missing a
trailing line feed.

@dbf = split(/\t/,$temp);

Here it would be worthwhile to spend two variables with explanatory names
instead of the anonymous @dbf. Only you can do that.

$ldf_dbf{$dbf[0]} = $dbf[1];
}
return (%ldf_dbf);
}

You might as well go over the file line by line. It won't make much of
a difference in the time the file is open, if that was your concern.

sub SaveDataFile
{
my $filename = shift;
my %this_dbf = shift;

Okay, here is your basic problem. You don't understand how parameters
are passed in Perl, especially that aggregates like hashes must be treated
differently from scalar parameters. Read it up in perlsub.

And you're running without warnings. Leave warnings switched on all
the time, unless you know what you are doing.

The last line above of your code assigns one scalar value (the result of
shift()) to the hash %this_dbf. That gives the hash one key with an
undefined value. Perl would have warned you about this if you let it.

Replace the line with

my %this_dbf = @_;

and your code should work as expected, as far as I see.

my $temp = "";
open(LDF_LPFILE,">$filename");
flock(LDF_LPFILE,2);
foreach $temp (keys(%this_dbf))
{
print LDF_LPFILE "$temp\t$this_dbf{$temp}\n";
}
close(LDF_LPFILE);
return;
}

Also acquaint yourself with the alternative of passing a hash reference
to the sub. It may be much more efficient if there is a lot of data.

Anno