On 12/20/05, Federico Lucifredi <[EMAIL PROTECTED]> wrote:
> Hello Guys,
> More Perl Style lessons for me, if anyone wants to chip in. Following
> is the script on the chopping block today - in the comments the parts
> that I did not manage to "elegantize" as much as I wanted.
>
> use Term::ANSIColor qw(:constants);
> use strict;
Where is warnings?
> #----------------CONFIGURATION--------
>
>
> my $timelen = 3;
>
> my @commands = ( '/bin/netstat -ape | /usr/bin/wc -l',
> '/usr/bin/lsof | wc -l',
> '/bin/ps ax -L | wc -l' );
I would indent this like so:
my @commands = (
'/bin/netstat -ape | /usr/bin/wc -l',
'/usr/bin/lsof | wc -l',
'/bin/ps ax -L | wc -l',
);
See Code Complete's advice on indentation for why.
> my @triggers = qw( 0 0 0);
>
> #-------------------------------------
>
> for(my $flag = 1; $flag;) #hate truth as a number...
Gah.
1. NEVER use "flag" as the name of a flag. One of the most common
programming errors is to accidentally reverse the meaning of a flag.
Make the name of the flag a yes/no question that the value of the flag
is the answer to. Then you will never again make that mistake.
2. Avoid C-style for loops whenever you have an alternative. You have
an alternative here - use a while or until loop. I would write the
above as:
my $last_loop;
until ($last_loop) {
...
That clearly documents intent.
> {
> my $date = `/bin/date -R`; #three lines seem much for this - any more
> elegant way to chomp things up?
> chomp $date;
> print $date."\t\t";
Is there a reason to not use Perl's built-in localtime() function?
If you need specific formatting, you can save a line by chomping while
assigning.
chomp(my $date = `/bin/date -R`);
Be careful about stacking functions too much, you might have fewer
lines but you are doing as much. However this combination is so
common that I suspect that it mentally "chunks".
> for (my $i = 0; $i < @commands; $i++)
Again, avoid C-style for loops. I would write the above as:
for my $i (0..$#commands) {
...
> {
> my $cmd = $commands[$i]; #I don't like these lookups, but I don't
> see how to foreach this one
> my $trig = $triggers[$i];
If you really don't like the lookups you can use Tye McQueens
Algorithm::Loops and do a mapcar here. I personally think that the
lookups are better.
> my $result = `$cmd` or die ("could not execute command".$cmd."\n");
> chomp $result;
> $result == $trig ? print ON_RED, $result, RESET : print $result;
> print "\t";
>
> $flag = 0 if ($result == $trig); # finish the internal round,
> terminate the external. Any nicer way to do it ?
Nope. If you wanted to terminate the inner or both, then you could
just use last. But if you want to terminate the outer, you need to
keep track of state.
However see my previous comments about the name of your flag.
> }
>
> print "\n";
> }
>
>
> Hum - the mail client is insisting in wrapping at 80-chars - usually
> nice but very appropriate to mess up things here :D
Code that runs into problems when wrapped at 80 characters needs to be
reformatted and/or rewritten. :-P
Cheers,
Ben
_______________________________________________
Boston-pm mailing list
[email protected]
http://mail.pm.org/mailman/listinfo/boston-pm
