>>>>> "FL" == Federico Lucifredi <[EMAIL PROTECTED]> writes:
FL> use strict;
where is use warnings?
FL> my @commands = ( '/bin/netstat -ape | /usr/bin/wc -l',
FL> '/usr/bin/lsof | wc -l',
FL> '/bin/ps ax -L | wc -l' );
FL> my @triggers = qw( 0 0 0);
since @triggers must be the same length as @commands then use that fact:
my @triggers = ('0') x @triggers ;
FL> #-------------------------------------
FL> for(my $flag = 1; $flag;) #hate truth as a number...
i can't imagine ever doing a for loop on a flag. use flow control ops
(last/next/redo)
FL> {
FL> my $date = `/bin/date -R`; #three lines seem much for this - any more
FL> elegant way to chomp things up?
FL> chomp $date;
FL> print $date."\t\t";
perldoc POSIX and look for strftime. never a need to shell out to date.
FL> for (my $i = 0; $i < @commands; $i++)
i hate seeing c style for loops in perl. to me they are a trigger that
there is a better way to do this. and there is!
FL> {
FL> my $cmd = $commands[$i]; #I don't like these lookups, but I don't
FL> see how to foreach this one
FL> my $trig = $triggers[$i];
when you see parallel arrays that are index like this, you should
immediately think about using data structures. multiple arrays are very
annoying and hard to manage. you can't pass around a single entry to
each array and index variables are so anti-perl IMO :).
there are two main choices for the structurs, array of arrays (each
sub-array has one cmd and one trigger) or an array of hashes (each hash
also has a cmd and trigger). i prefer the hash since then the keys can
be named vs using integer indexes. so here is how i would declare the
data:
my @commands = (
{
cmd => '/bin/netstat -ape | /usr/bin/wc -l',
trigger => 0,
},
{
cmd => '/usr/bin/lsof | wc -l'
trigger => 0,
},
{
cmd => '/bin/ps ax -L | wc -l',
trigger => 0,
},
) ;
then you loop over that in normal perl fashion:
for my $cmd_data ( @commands ) {
you could grab the cmd/trig values in several ways (and you may not even
need to grab it if you only use it once as you do!
my $cmd = $cmd_data->{cmd} ;
my $trig = $cmd_data->{trigger} ;
but as i said, skip that if you don't need it.
FL> my $result = `$cmd` or die ("could not execute command".$cmd."\n");
i don't like the look of that. it isn't bad code but not my style. just
think about the failure mode of the shell commands you are running. they
may print nothing to stdout but something to stderr. that or die may
catch it but it would be better to test $? and see what the process
itself exited with. also some commands may return some error text in
stdout. i would do it this way:
my $result = `$cmd` ;
$? == 0 or die "could not execute command [$cmd]\n" ;
also check that your command will do the right thing with their exit
codes (which is what $? gets along with signal info).
and in any case, you can just use $cmd in the die string.
my $result = `$cmd` or die "could not execute command [$cmd]\n" ;
i put [] around some values in case i want to see exactly what is in
there (especially where spaces can have meaning).
FL> $result == $trig ? print ON_RED, $result, RESET : print $result;
FL> print "\t";
that is a big no-no! don't put side effect operations inside ?:. in
almost all cases you can factor out the operation (print in this case)
and use ?: for its intended use of returning a conditional value. your
code is a form of flow control and the value of ?: is never used. so
either use a proper if/else block or use ?: for its value only like
this:
print( ($result == $trig) ?
(ON_RED, $result, RESET) :
$result,
"\t"
) ;
the whole result == trig issue is another story. you store '0' (as i did
with in my rewrite) in the triggers but compare as numbers here with
==. the result value is a string (`` only can return a string). so is ==
proper here? if you are sure it will be better to compare as numbers
(say result is 00) then use == but then @triggers should also be 0 and
not '0' to be more meaningful. subtle but useful little thing to do.
FL> $flag = 0 if ($result == $trig); # finish the internal round,
FL> terminate the external. Any nicer way to do it ?
last!
last if $result == $trig ;
but what did i just see here but $result == $trig being tested twice in
a row. that should be factored out and now we need a full if/else
block to handle all the code.
if( $result == $trig ) {
print ON_RED, $result, RESET, "\t" ;
last ;
}
else {
print $result, "\t" ;
}
that should be enough to keep you busy! :)
uri
--
Uri Guttman ------ [EMAIL PROTECTED] -------- http://www.stemsystems.com
--Perl Consulting, Stem Development, Systems Architecture, Design and Coding-
Search or Offer Perl Jobs ---------------------------- http://jobs.perl.org
_______________________________________________
Boston-pm mailing list
[email protected]
http://mail.pm.org/mailman/listinfo/boston-pm
