Hi Paul, a few comments on your code - so you'll know how to write Perl better.
On Wednesday 05 May 2010 02:52:03 Paul Fontenot wrote: > Hi, > > I'm stuck on using an array to determine the out come of a foreach loop. > The script is below. > --------------------------------------------------------------------------- > --------------------------------- > #!/usr/bin/perl > # Always add "use strict;" and "use warnings;" at the top of your programs (and fix whatever they point to). > @conditions = ("NET", "eth"); Declare your variables using "my". (use strict requires it). > $hostname = (`/bin/hostname`); Why are the backticks inside a "( ... )"? It shouldn't hurt, though. > $logfile = "/var/log/messages"; > $output = "/root/bin/output.txt"; > > open(OUTPUT, ">>", $output) or die $!; > open(LOGFILE, "<", $logfile) or die $!; > It's good that you're using three-args-open followed by "or die", but: 1. You should include a more descriptive error. 2. Don't use bareword filehandles - use lexical ones: open(my $output, ">>", $output) or die "Could not append to output - $!"; > foreach $condition (@conditions) { > print "\n"; > print "$condition detected on $hostname"; > print > "=========================================================================\ > n"; I don't understand why you are printing this message without checking for this condition? > > foreach $line (<LOGFILE>) { while (my $line = <$logfile>) would be a better idea than foreach $line. Furthermore, this won't work at the second iteration of the @conditions loop because LOGFILE reached end-of-file (EOF). You probably need to open it in every iteration of the loop. > if ($line =~ /$condition/) { 1. Perhaps you want to check for both conditions one after the other, and collect their results, so you won't have to go over the file's lines twice. 2. Doing /$condition/ will evaluate "$condition" as a mini-regex, which is potentially dangerous. Please consider doing \Q$condition\E or maybe use http://perldoc.perl.org/functions/index.html . > print $line; > } > } > } > > close(OUTPUT); > > if ( -s $output) { > system(`/bin/mail -s \"$condition for \${HOSTNAME}\" > wpfonten...@cox.net < $output`); You shouldn't use both system and backicks. Which one of them do you need? > unlink $output; > } > else { > unlink $output; > } You should probably put the unlink after the if/else conditional to avoid duplicate code. > close(LOGFILE); > Regards, Shlomi Fish -- ----------------------------------------------------------------- Shlomi Fish http://www.shlomifish.org/ Parody on "The Fountainhead" - http://shlom.in/towtf God considered inflicting XSLT as the tenth plague of Egypt, but then decided against it because he thought it would be too evil. Please reply to list if it's a mailing list post - http://shlom.in/reply . -- To unsubscribe, e-mail: beginners-unsubscr...@perl.org For additional commands, e-mail: beginners-h...@perl.org http://learn.perl.org/