Looking to improve program

Discussion in 'Perl Misc' started by Tom Ewall, Nov 10, 2004.

  1. Tom Ewall

    Tom Ewall Guest

    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 = $_;
    Tom Ewall, Nov 10, 2004
    #1
    1. Advertising

  2. Tom Ewall

    Paul Lalli Guest

    "Tom Ewall" <> wrote in message
    news:...
    > 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
    Paul Lalli, Nov 10, 2004
    #2
    1. Advertising

  3. Tom Ewall

    Uri Guttman Guest

    >>>>> "PL" == Paul Lalli <> writes:

    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
    Uri Guttman, Nov 10, 2004
    #3
  4. Tom Ewall

    Paul Lalli Guest

    "Uri Guttman" <> wrote in message
    news:...
    > >>>>> "PL" == Paul Lalli <> writes:

    >
    > 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
    Paul Lalli, Nov 10, 2004
    #4
  5. Tom Ewall

    Tom Ewall Guest

    Thanks very much for the suggestions!
    Tom Ewall, Nov 12, 2004
    #5
  6. Tom Ewall wrote:
    > 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
    --
    use Perl;
    program
    fulfillment
    John W. Krahn, Nov 12, 2004
    #6
    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. stelios xanthakis

    Looking for Benchmarklets to improve pyvm

    stelios xanthakis, Apr 1, 2005, in forum: Python
    Replies:
    2
    Views:
    290
    stelios xanthakis
    Apr 2, 2005
  2. goosen_cug
    Replies:
    9
    Views:
    320
    Dan Bloomquist
    Nov 10, 2006
  3. DG
    Replies:
    4
    Views:
    262
  4. Replies:
    14
    Views:
    508
  5. Esmail Bonakdarian

    1st program -- how would you improve this?

    Esmail Bonakdarian, Oct 5, 2006, in forum: Ruby
    Replies:
    9
    Views:
    97
    Esmail Bonakdarian
    Oct 6, 2006
Loading...

Share This Page