Passing an array to a sub routine

Discussion in 'Perl Misc' started by Bill H, Jan 17, 2006.

  1. Bill H

    Bill H Guest

    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
    Bill H, Jan 17, 2006
    #1
    1. Advertising

  2. Bill H

    Paul Lalli Guest

    Bill H wrote:
    > 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
    Paul Lalli, Jan 17, 2006
    #2
    1. Advertising

  3. Bill H wrote:

    > 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.
    Brian McCauley, Jan 17, 2006
    #3
  4. Bill H

    Anno Siegel Guest

    Bill H <> wrote in comp.lang.perl.misc:
    > 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
    --
    If you want to post a followup via groups.google.com, don't use
    the broken "Reply" link at the bottom of the article. Click on
    "show options" at the top of the article, then click on the
    "Reply" at the bottom of the article headers.
    Anno Siegel, Jan 17, 2006
    #4
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. Neo
    Replies:
    6
    Views:
    395
    Mark A. Odell
    Dec 2, 2003
  2. Neo
    Replies:
    2
    Views:
    311
    Arthur J. O'Dwyer
    Dec 9, 2003
  3. Ben
    Replies:
    2
    Views:
    858
  4. Lawrence D'Oliveiro

    Death To Sub-Sub-Sub-Directories!

    Lawrence D'Oliveiro, May 5, 2011, in forum: Java
    Replies:
    92
    Views:
    1,960
    Lawrence D'Oliveiro
    May 20, 2011
  5. erik
    Replies:
    1
    Views:
    179
    A. Sinan Unur
    May 10, 2005
Loading...

Share This Page