Re: Looking to improve program

From: Paul Lalli (mritty_at_gmail.com)
Date: 11/10/04 

Date: Wed, 10 Nov 2004 16:27:37 GMT

"Tom Ewall" <tewall@lycos.com> wrote in message
news:85bf428.0411100802.1d234182@posting.google.com...
> The following code looks for files in a directory with lables of "Run
> A" "Run B" "Run C" or "Run D". Each of the files has a total line.
> It grabs the total, something like "$211,153" and strips out the "$"
> and the ",".
>
> The program works, but I'm sure there are more efficient/perl
> idiomatic ways of doing things than how I've done them here. I'm
> looking for suggested improvements.

suggestions noted below:

> #!\Perl\bin\perl
>
> use strict;
use warnings;

> my $total;
> my @total;
> my $runA;
> my $runB;
> my $runC;
> my $runD;
> my @list;
> my $lmoDir = "/__lmo";

1) You generally should declare your variables in the smallest scope
possible, which usually means "not until you need them".
2) Even if you insist on declaring them all at once, at least don't
bother with so many extra declarations:
my ($total, @total, $runA, $runB, $runC, $runD, @list);
my $lmnoDir = '/__lmno';

> opendir(LMODIR, $lmoDir) or die "could not open LMO directory";

1) Using lexical file handles is preferred to bareword filehandles.
2) If the opendir fails, it'd be nice to know *why* it failed:

opendir $dir, $lmnoDir or die "Could not open directory: $!";

> my @allfiles = grep !/^\.\.?\z/, readdir LMODIR; # exclude . and ..
> files
> closedir(LMODIR);
>
> foreach my $file (@allfiles){

rather than reading all the file names into memory at once, I'd probably
read them one at a time, and pick out the ones I don't want within the
loop:

while (my $file = readdir ($dir)){
   next if $file =~ /^\.{1,2}$/;

> open LMO, "$lmoDir/$file" or die "Cannot open file: $!";

again, lexical file handles are preferred:
open $lmo, "$lmoDir/$file" or die "Cannot open file: $!";

> my @list=<LMO>;
> foreach (@list) {

Again, no reason to slurp the entire file into memory:

while (<$lmo>){
    chomp; #rather than chomping conditionally twice below

> if (/Run/){
> $run = "$_";

see the PerlFAQ "What's wrong with always quoting "$vars"?":
perldoc -q quoting

> chomp ($run);
> }
> if (/Total/){
> $total = "$_";
> chomp ($total);
> }
> }
> @total = split(/\t/, $total);
> $total = @total[$#total - 2];

Do you really not know how many fields will *precede* the field you
want, and instead only know how many will follow? If that's true, I
suppose this is okay. Otherwise, if you know that the desired field
will always be, say, 2nd, I'd recommend eliminating the extraneous
array:

(undef, $total) = split (/\t/, $total);

As side notes, you should never use @ to access a single element of an
array. You might also want to learn about negative subscripts to
arrays, to avoid the yucky $#total syntax. That line could/should have
been:

$total = $total[-3];

> if ($run eq 'Run A'){
> $runA = $total;
> }
> if ($run eq 'Run B'){
> $runB = $total;
> }
> if ($run eq 'Run C'){
> $runC = $total;
> }
> if ($run eq 'Run D'){
> $runD = $total;
> }

This kind of structure screams for using a hash instead of four
independent variables:
my %run; #before the for loop

if ($run =~ /^Run ([A-D])$/){
    $run{$1} = $total;
}

> }
>
> $_ = $runA;
> s/,//;
> s/\$//;
> $runA = $_;
>
> $_ = $runB;
> s/,//;
> s/\$//;
> $runB = $_;
>
> $_ = $runC;
> s/,//;
> s/\$//;
> $runC = $_;
>
> $_ = $runD;
> s/,//;
> s/\$//;
> $runD = $_;

Now that you have the hash, you can use it instead of four nearly
identical code fragments. There's also no real reason to be assigning
to and from $_, rather than just using the variables you have. You can
also combine the two substitutes:
foreach my $key (%run){
   $run{$key} =~ s/[,$]//g;
}

Hope this is helpful,
Paul Lalli

Relevant Pages

  • Re: help with array within another array
    ... you could make a hash with reference to hash values: ... it seems like you might want to consider using the node identifier as the first key in %SESSION, and make the value a reference to hash keyed by the owner and severity: ... I am trying to declare two associative array ...
    (perl.beginners)
  • return array/hash ref from function
    ... I have a number of functions in a program I'm writing that return a reference to ... an array or hash. ... I declare the variable to return with 'my' ...
    (perl.beginners)
  • Re: return array/hash ref from function
    ... > an array or hash. ... I declare the variable to return ...
    (perl.beginners)
  • Re: Passing an array of structuresfrom a pointer?
    ... to only declare structs in headers and then define the ... the struct should be declared ... what if you have a simple array like this: ... In the header we would declare? ...
    (microsoft.public.vc.language)
  • Duck Typed Concepts for Ruby (was Re: A use case for an ordered hash)
    ... An Sequencable mixin can be defined that implements all sorts of operations such as append, concat, splice, sort, etc. ... extending an instance of Array with Sorted if the array is known to be sorted. ... Now returning to the thread at hand we can see that the difference between the associative array and hash hierarchies is that the hash hierarchy depends upon Hashable keys. ...
    (comp.lang.ruby)