In article <6via61$d0u$1@nnrp1.dejanews.com>, <baillie@my-dejanews.com> wrote:
>if(defined($ARGV[0])) {
> $host=$ARGV[0];
> system "rsh $host alex-sched -LP 0 > /tmp/opck.cache";
>} else {
> $host='localhost';
> system "alex-sched -LP 0 > /tmp/opck.cache";
>}
Five lines become four, a variable disappears, and you get some error checking:
$command = 'alex-sched -LP 0';
$command = "rsh $ARGV[0] $command" if $ARGV[0];
open (FILE, "$command |")
or die "Couldn't execute `$command'; aborting";
Also, you no longer have to manage a file. That is a big win. Files suck.
>while(<FILE>) {
> if(/^Total Bytes to Backup/) {
> chomp($to_store=$_);
> $to_store=~tr/[a-z A-Z:]//d;
> $to_store/=1024;
> }
Four lines become three:
while (<FILE>) {
if (/Total Bytes to Backup/) {
($to_store) = /(\d+)/ ; # Extract the first number from this line $to_store /= 1024;
}
> if(/^Current KBytes Stored/) {
> chomp($store_array[$i]=$_);
> $i++;
> }
> }
Three lines become two, and we get rid of two unnecessary variables:
elsif (/^Current KBytes Stored/) {
($amount_stored) = /(\d+)/; # Extract number from this line # You never use the @store_array, so I got rid of it.
}
}
>close(FILE) or die "Couldn't close file: $!";
Good! But I would change the message to
close(FILE) or die "Couldn't finish command `$command': $!";
>unlink("/tmp/opck.cache") or warn "Couldn't remove file: $!";
>
> # Use the last value of the array for the current
> # amount of data that's been stored
>$amount_stored=$store_array[$#store_array];
> # Is this (below) right?..to grep out only the numbers
>$amount_stored=~tr/[a-z A-Z:]//d;
>undef(@store_array);
>undef($i);
7 lines are replaced by zero!
>$cntr=2;
This isn't doing anything; you reinitialize $cntr to 0 later on.
>$screen_width=`tput cols`;
>$percentage=($amount_stored/$to_store)*100;
>$vis_disp=($amount_stored/$to_store)*$screen_width;
>
>printf "\t$host amount to store\t=\t%.0f\n", $to_store;
>print "\t\tamount stored\t\t=\t$amount_stored\n";
>
>system "tput smso"; # Is there a better way to do this (curses)?
All this stuff is plausible, but see below for a different approach.
>for($cntr=0; $cntr<=$vis_disp; $cntr++) {
> print "=";
>}
Three lines become one line, and a variable disappears:
print "=" x $vis_disp; # I think yours printed one too many?
>printf "\t\t\t\t\tOpcard is%2d", $percentage;
I personally would have gotten `percentage' into the right format the first time:
$percentage = sprintf "%2d", 100*($amount_stored/$to_store);
...
print "\t\t\t\t\tOpcard is $percentage";
Then the `print' statement reads naturally, and related things (the computation and the formatting for $percentage) are together.
>system "tput rmso";
It bugs me to have all these `system' commands interspersed into the `prints'. It looks messy! It's hard to understand what the output will look like. You are inviting buffering problems. I think it would be better to take a different approach.
You suggested using a module; I think Term::Cap would be suitable here. But here's a different approach:
foreach $escape (qw(rmso bold smso cols)) {
$T{$escape} = qx{tput $escape};
}
Now the escape codes are available there when you need them, and you don't have to bust up the output, so it's easier to see what is going to come out:
print "$T{rmso}$T{bold}\t\t\t\t\tOpcard is $percentage$T{rmso}% complete\n\n";
You might like to have the code names in caps?
foreach $escape (qw(rmso bold smso cols)) {
$T{uc $escape} = qx{tput $escape};
}
print "$T{RMSO}$T{BOLD}\t\t\t\t\tOpcard is $percentage$T{RMSO}% complete\n\n";
I liked them better in lowercase, but it's up to you.
So here's the reworked version of the program:
#!/usr/bin/perl
$command = 'alex-sched -LP 0';
$command = "rsh $ARGV[0] $command" if $ARGV[0];
$host = $ARGV[0] || 'localhost';
open (ALEX, "$command |")
or die "Couldn't execute `$command'; aborting";
while (<ALEX>) {
if (/Total Bytes to Backup/) {
($to_store) = /(\d+)/ ;
$to_store /= 1024;
} elsif (/^Current KBytes Stored/) {
($amount_stored) = /(\d+)/;
}
}
close(ALEX) or die "Couldn't finish command `$command': $!";
# Get terminal escape sequences for output formatting
foreach $escape (qw(rmso bold smso cols)) {
$T{$escape} = qx{tput $escape}; # `T' for `Terminal'
}
$percentage= sprintf "%2d", ($amount_stored/$to_store)*100;
printf "\t$host amount to store\t=\t%.0f\n", $to_store; print "\t\tamount stored\t\t=\t$amount_stored\n";
print $T{smso},
( '=' x ($amount_stored/$to_store) * $T{cols} ), ">\n", $T{rmso};
print "$T{bold}\t\t\t\t\tOpcard is $percentage$T{rmso}% complete\n\n";
It's gone from 37 lines to 20, lost a bunch of superfluous variables, has better error checking, no longer uses a temporary file, calls `tput rmso' only once instead of twice, and I think it's clearer than it was before.
Hope this helps. Now you know how to find the last instance without saving all the previous instances into an array.
|
