views:

191

answers:

2

Sorry about all these silly questions, I've been thrust into Perl programming and I'm finding it really hard to think like a Perl programmer.

Silly question for today: I load a pipe delimited file into a hash using the id field as the key, like so

#open file

my %hash;
while (<MY_FILE>) {
    chomp;

    my ($id, $path, $date) = split /\|/;

    $hash{$id} = {
        "path" => $path,
        "date" => $date
    };
}

There a few times, however, when I actually need the key to be the path because, for whatever reason (and no, it can't be changed) the id isn't unique, so I had the bright idea that I could put it all into a subroutine and pass the name of the variable to use as the key to it, kinda like so:

load_hash("path");

sub load_hash {
    my $key = shift;

    #do stuff, and then in while loop
    $hash{${$key}} = #and so on
}

but in perldb x ${$key} is always undef, although x ${path} prints the value in $path.

Is there some way of doing what I'm trying to?

TIA

+2  A: 

Something like this?

use Carp 'confess';

sub load_hash {
    my $key = shift;

    # ...

    while (...) {
        # ...
        my %line;  # important that this is *inside* the loop
        @line{qw (id path date)} = split /\|/;
        confess "BUG: unknown key '$key'"  unless exists $line{$key};  # error checking
        $hash{$line{$key}} = \%line;
        delete $line{$key};  # assuming you don't want the key value duplicated
    }
}
Dave Hinton
Thanks, that worked.
Sparkles
+9  A: 

You're trying to use "symbolic references". If you have a problem and you think "hey, I'll solve this with symbolic references" you now have two problems.

First off, they only work on globals. You've declared $path as a lexical (only visible in the block which it was declared) and thus load_path can't see it. No, don't make $path global.

Second, symbolic refs create spaghetti code. Globals are bad enough. They can be accessed anywhere, anytime by anything. With a symbolic reference to a global you can't even see WHICH global is being accessed. This makes it impossible to track what might change what. This is why strict turns them off. Turn on strict and leave it on until you know when you should turn it off.

I'm not entirely sure what you're trying to accomplish, but it seems this is fine.

my %hash;
while (<MY_FILE>) {
    chomp;

    my ($id, $path, $date) = split /\|/;

    $hash{$path} = {
        "path" => $path,
        "date" => $date
    };
}

But I'd probably move the parsing of the line into a function and leave the hash assignment to the main loop. Parsing the line is a clear chunk of logic and can be totally separated from assigning the line to the file hash. A good sign is that %hash does not have to be global.

my %hash;
while (<MY_FILE>) {
    my $line = parse_line($_);

    my $id = $line->{path};
    $hash{$id} = $line;
}


my @fields = qw(id path date);
sub parse_line {
    my $line = shift;
    chomp $line;

    my %data;
    # This is assigning to a hash slice.  Look it up, its handy.
    @data{@fields} = split m{\|}, $line;

    return \%data;
}
Schwern
Don't worry, I always use strict and warnings (although I had to turn off warnings when trying this).
Sparkles
@Sparkles, if you had to turn off warnings, you are probably doing something wrong.
Brad Gilbert
upvoted for quoting the most self-righteous Perl hater alive? (Naggum is the most self-righteous Perl hater, but he died.)
jrockway
@Sparkles That's some new definition of "always". Don't just turn warnings off, figure out what's causing the warning and fix it. @jrockway I thought I was quoting mjd (he was talking about AUTOLOAD). Seems jwz wasn't the original, either: http://➡.ws/醺緎
Schwern