>>>>> "M" == Marc <sono...@fannullone.us> writes:
M> The script works great but I'd like to get advice on how I can
M> clean up my code, so any comments are welcome.
M> Also, do I really need the foreach block in there? I couldn't
M> get it to work without it, but it seems like I should be able
M> to. =:\
M> use strict;
M> use warnings;
good.
M> use File::Find;
M> use File::HomeDir;
M> find(\&wanted, $path_to_search);
M> sub wanted {
M> if ($File::Find::name =~ /$file_name/) {
it would be much cleaner IMO to collect the paths of the files you want
and then processs those paths in a sub. the rule is generally collect
the data you need and process later and elsewhere. this is good for
several reasons. you could write each part to be more generic and
reusable. when they are tied like this you have only one use for the
code. also it make it easier to debug. you can test the mailer sub by
passing it a path and never executing the search part.
M> my $msg = "MIME-Version: 1.0\n".
M> "Content-Type: text/plain\n".
M> "To: $to_address\n".
M> "From: $from_address\n".
M> "Subject: $subject\n\n";
instead of multiple lines with . operators this is much easier with a
here doc. no quotes, no \n, no . ops.
my $msg = <<MSG ;
MIME-Version: 1.0
Content-Type: text/plain
To: $to_address
From: $from_address
Subject: $subject
MSG
but beyond this calling sendmail directly isn't cool anymore. there are
many useful mail modules that make this easier and work with other
mailers or directly with smtp.
M> open (my $mail_fh, "|$mail_app -t -oi -oem") || die "Can't open
sendmail!";
M> print {$mail_fh} "$msg";
don't quote scalar vars like that. it makes an unneeded copy and can be
a bug in some situations.
also you can get the file text and do one print to the mailer.
M> print $msg;
M> open (my $file_fh, '<', $File::Find::name) ||
M> die "Can't open $file_name: $!";
have to plug File::Slurp here. it can read the whole file into a scalar
and is much cleaner and faster than your code.
M> my (@lines) = <$file_fh>;
M> foreach my $line (@lines) {
M> print "$line";
M> }
M> print {$mail_fh} "@lines";
that quoted array will put a space between each line. it may not be what
you want. this should be all you need:
use File::Slurp ;
my $log_text = read_file( $File::Find::name ) ;
print $mail_fh $msg, $log_text ;
print $log_text ;
M> close ($file_fh) || die "Can't close file";
not needed with slurp.
M> close $mail_fh || die "Can't close mail_fh";
M> unlink ($File::Find::name);
M> }
M> }
M> ----------------------------
M> In case anyone's interested, here's the shell script it replaces:
if you simplified it to separate subs like i mention and use mailer
modules and file::slurp, it will be almost as short as the shell version.
uri
--
Uri Guttman -- uri AT perlhunter DOT com --- http://www.perlhunter.com --
------------ Perl Developer Recruiting and Placement Services -------------
----- Perl Code Review, Architecture, Development, Training, Support -------
--
To unsubscribe, e-mail: beginners-unsubscr...@perl.org
For additional commands, e-mail: beginners-h...@perl.org
http://learn.perl.org/
