Marc wrote:
I've written a script to traverse my web server, find any files called
"error_log", e-mail them to me, and then delete them. It is triggered
by cron twice a day.
The script works great but I'd like to get advice on how I can clean
up my code, so any comments are welcome.
Also, do I really need the foreach block in there? I couldn't get it
to work without it, but it seems like I should be able to. =:\
Thanks,
Marc
------------------------------------
#!/usr/bin/perl
use strict;
use warnings;
use File::Find;
use File::HomeDir;
my $path_to_search = File::HomeDir->my_home.'/public_html';
The use of File::HomeDir is great, if this program were running on a
variety of different operating systems, but your use of
'/usr/sbin/sendmail' implies that this is only running on a Unix-like
operating system so you probably only need $ENV{HOME}.
my $file_name = 'error_log';
my $from_address = 'x...@xxx.com';
my $to_address = 'x...@xxx.com';
my $mail_app = '/usr/sbin/sendmail';
my $subject = 'An "error_log" was found';
find(\&wanted, $path_to_search);
sub wanted {
if ($File::Find::name =~ /$file_name/) {
Why compare the full path using a regular expression? Just compare the
file name itself:
return if $_ ne $file_name;
my $msg = "MIME-Version: 1.0\n".
"Content-Type: text/plain\n".
"To: $to_address\n".
"From: $from_address\n".
"Subject: $subject\n\n";
This variable doesn't change during inside the loop, so don't put it
inside the loop.
open (my $mail_fh, "|$mail_app -t -oi -oem") || die "Can't open
sendmail!";
You should include the $! variable in the error message so you know why
it failed. You should use the list form of pipe open so you don't
invoke a shell to run $mail_app:
open my $mail_fh, '|-', $mail_app, '-t', '-oi' '-oem' or die "Can't
open '$mail_app' because: $!";
print {$mail_fh} "$msg";
No need to copy $msg to a string:
print $mail_fh $msg;
print $msg;
open (my $file_fh, '<', $File::Find::name) || die "Can't
open $file_name: $!";
my (@lines) =<$file_fh>;
foreach my $line (@lines) {
print "$line";
}
print {$mail_fh} "@lines";
No, you don't need a foreach loop. And no, you don't need to quote
variables (copy variable contents to strings.)
my @lines = <$file_fh>;
print @lines;
print $mail_fh @lines;
close ($file_fh) || die "Can't close file";
You should include the $! variable in the error message so you know why
it failed.
close $mail_fh || die "Can't close mail_fh";
You should verify that the pipe was closed correctly:
close $mail_fh or warn $! ? "Error closing '$mail_app' pipe: $!"
: "Exit status $? from '$mail_app'";
unlink ($File::Find::name);
You should verify that unlink() worked correctly:
unlink( $File::Find::name ) or warn "Cannot unlink '$File::Find::name'
because: $!";
}
}
----------------------------
In case anyone's interested, here's the shell script it replaces:
#!/bin/sh
## Variables ##
FILES=`find /home/user/public_html -name error_log`
ADDRESS=x...@xxx.com
for file in $FILES
do
if [ -e "$file" ]
then
mail -s "$file" $ADDRESS< $file
rm -r $file # delete log file
fi
done
John
--
Any intelligent fool can make things bigger and
more complex... It takes a touch of genius -
and a lot of courage to move in the opposite
direction. -- Albert Einstein
--
To unsubscribe, e-mail: beginners-unsubscr...@perl.org
For additional commands, e-mail: beginners-h...@perl.org
http://learn.perl.org/
