Looking to improve program

T

Tom Ewall

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.
-----------------------------------------------------------------------


#!\Perl\bin\perl

use strict;

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


opendir(LMODIR, $lmoDir) or die "could not open LMO directory";
my @allfiles = grep !/^\.\.?\z/, readdir LMODIR; # exclude . and ..
files
closedir(LMODIR);

foreach my $file (@allfiles){
open LMO, "$lmoDir/$file" or die "Cannot open file: $!";
my @list=<LMO>;
foreach (@list) {
if (/Run/){
$run = "$_";
chomp ($run);
}
if (/Total/){
$total = "$_";
chomp ($total);
}
}
@total = split(/\t/, $total);
$total = @total[$#total - 2];
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;
}
}

$_ = $runA;
s/,//;
s/\$//;
$runA = $_;

$_ = $runB;
s/,//;
s/\$//;
$runB = $_;

$_ = $runC;
s/,//;
s/\$//;
$runC = $_;

$_ = $runD;
s/,//;
s/\$//;
$runD = $_;
 
P

Paul Lalli

Tom Ewall said:
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:

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
 
U

Uri Guttman

PL> foreach my $key (%run){
PL> $run{$key} =~ s/[,$]//g;

in recent perls values returns aliases to the actual values of a
hash. so that could become:

s/[,$]//g for values %run ;

and since tr is faster for char operations:

tr/,$//d for values %run ;

uri
 
P

Paul Lalli

Uri Guttman said:
PL> foreach my $key (%run){
PL> $run{$key} =~ s/[,$]//g;

in recent perls values returns aliases to the actual values of a
hash. so that could become:

s/[,$]//g for values %run ;

huh. I didn't realize that. Good to know. Thanks.
and since tr is faster for char operations:

tr/,$//d for values %run ;

Somehow I always manage to forget about tr///. Thanks again.

Paul Lalli
 
J

John W. Krahn

Tom said:
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.
-----------------------------------------------------------------------


#!\Perl\bin\perl

use strict;

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


opendir(LMODIR, $lmoDir) or die "could not open LMO directory";
my @allfiles = grep !/^\.\.?\z/, readdir LMODIR; # exclude . and .. files
closedir(LMODIR);

foreach my $file (@allfiles){
open LMO, "$lmoDir/$file" or die "Cannot open file: $!";
my @list=<LMO>;
foreach (@list) {
if (/Run/){
$run = "$_";
chomp ($run);
}
if (/Total/){
$total = "$_";
chomp ($total);
}
}
@total = split(/\t/, $total);
$total = @total[$#total - 2];
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;
}
}

$_ = $runA;
s/,//;
s/\$//;
$runA = $_;

$_ = $runB;
s/,//;
s/\$//;
$runB = $_;

$_ = $runC;
s/,//;
s/\$//;
$runC = $_;

$_ = $runD;
s/,//;
s/\$//;
$runD = $_;

If you want idiomatic perl then:

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

my %totals;

{ local( @ARGV, $/ ) = grep -f, </__lmo/*>;
while ( <> ) {
my ( $run ) = /Run ([ABCD])/ or next;
next unless /(.*Total.*)/;
( $totals{ $run } = ( split /\t/, $1 )[ -3 ] ) =~ tr/$,//d;
}
}

for my $run ( sort keys %totals ) {
print "Run: $run Total: $totals{$run}\n";
}

__END__



John
 

Ask a Question

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

You'll need to choose a username for the site, which only take a couple of moments. After that, you can post your question and our members will help you out.

Ask a Question

Members online

Forum statistics

Threads
473,744
Messages
2,569,484
Members
44,903
Latest member
orderPeak8CBDGummies

Latest Threads

Top