PERL Beginners - Array to Hash

Author

Array to Hash

Yitzle

2007-04-18, 6:58 pm

Any tips on compacting this sub?

sub readFile($) {
my $fileName = shift;
open FILE, "<", $fileName;
while (<FILE> ) {
my($name,$oldCount,$oldNum) = split /~/;
$dHash{$name}{'oldCount'} = $oldCount;
$dHash{$name}{'oldNum'} = $oldNum;
}
close FILE;
}

Chas Owens

2007-04-18, 6:58 pm

On 4/18/07, yitzle <yitzle@users.sourceforge.net> wrote:
> Any tips on compacting this sub?
>
> sub readFile($) {
> my $fileName = shift;
> open FILE, "<", $fileName;
> while (<FILE> ) {
> my($name,$oldCount,$oldNum) = split /~/;
> $dHash{$name}{'oldCount'} = $oldCount;
> $dHash{$name}{'oldNum'} = $oldNum;
> }
> close FILE;
> }

Don't use prototypes. They are broken and will give you a false sense
of security.

Don't use global variables (%dHash), they make your code harder to
read and maintain. Read up on decoupling.

Use lexical file handles instead and you don't have to close it (they
auto-close when they go out of scope).

open my $file, "<", $fileName or die "Could not open $fileName: $!";
while (<$file> ) {

Use an anonymous hash ref in stead of assigning and use smaller names
for variables with limited scope.

my ($name,$cnt,$num) = split /~/;
$dHash{$name} = { oldCount => $cnt, oldNum => $num };

Alternatively, use a hash slice if you don't want to disturb other
keys that may exist at the level in %dHash

my ($name,$cnt,$num) = split /~/;
@{$dHash{$name}}{qw(oldCount oldNum)} = ($cnt, $num);

Here is my version:
sub readFile {
croak "readFile expects (filename, hashref)" unless @_ == 2 and
ref $_[1] == 'HASH';
my ($file, $h) = @_;
open my $f, "<", $file or die "could not open $f for reading: $!";
while (<$f> ) {
my ($name, $cnt, $num) = split /~/;
@{$h->{$name}}{qw(oldCount oldNum)} = ($cnt, $num);
}
}

Rob Dixon

2007-04-18, 6:58 pm

yitzle wrote:
>
> Any tips on compacting this sub?
>
> sub readFile($) {
> my $fileName = shift;
> open FILE, "<", $fileName;
> while (<FILE> ) {
> my($name,$oldCount,$oldNum) = split /~/;
> $dHash{$name}{'oldCount'} = $oldCount;
> $dHash{$name}{'oldNum'} = $oldNum;
> }
> close FILE;
> }

sub readFile {
open my $fh, shift or die $!;
while (<$fh> ) {
my ($name, $count, $num) = split /~/;
$dHash{$name} = { oldCount => $count, oldNum => $num };
}
}

Rob

Uri Guttman

2007-04-18, 6:58 pm

>>>>> "Y" == Yitzle <yitzle@users.sourceforge.net> writes:

Y> Any tips on compacting this sub?

why do you want it compacted? it is pretty short as it is.

Y> sub readFile($) {

don't use prototypes. they don't do what you think they do.

Y> my $fileName = shift;
Y> open FILE, "<", $fileName;

ALWAYS check open calls for success. use lexical file handles.

open my $fh, "<", $fileName or die "can't open $filename $!" ;

Y> while (<FILE> ) {
Y> my($name,$oldCount,$oldNum) = split /~/;
Y> $dHash{$name}{'oldCount'} = $oldCount;

where is %dHash declared? it must be a global or file lexical and that
is a bad thing. populate a lexical hash in the sub and return that. let
the caller deal with that hash (assign it, pass it around, etc.).

Y> $dHash{$name}{'oldNum'} = $oldNum;

pick better names than dHash. we know it is a hash. but what is d?? you
have the ability and wisdom to choose meaningful names so do that.

the only normal shortening i can think of is to use a hash slice but it
doesn't gain much here:

@{$dHash{$name}{ qw( oldCount oldNum ) } = ( $oldCount, $oldNum ) ;

a more drastic shortening and more fun IMO is (untested):

use File::Slurp ;

my %dH ;

my $old_text = read_file( $filename ) ;
$dH{ $1 } = { oldCount => $2, oldNum => $3 }
while $old_text =~ /^([^~]+)~([^~]+)~([^~]+)$/mg ;

i leave that as an exercise to figure out (and debug :).

uri

--
Uri Guttman ------ uri@stemsystems.com -------- http://www.stemsystems.com
--Perl Consulting, Stem Development, Systems Architecture, Design and Coding-
Search or Offer Perl Jobs ---------------------------- http://jobs.perl.org

Sponsored Links