> #!/usr/bin/perl -w
>
> use strict;
> use warnings;
You probably don't want both -w and 'use warnings'. Personally I would
stick with the 'use warnings' unless you have to deal with older
versions of Perl which is not terribly likely. The -w turns on warnings
for the whole script, which will include those produced by CGI,
Mail::Mailer and any other CPAN modules used. 'use warnings' will be
for only this script (or any other file you add it too) which will allow
you to only hear about those things you can fix.
>
> use CGI();
> use Mail::Mailer;
>
> my $q = CGI->new();
>
> print $q->header();
>
> #-----------------------------------------
> # * startup methods -> global variables.
>
> check_fields();
>
> #-----------------------------------------
>
> sub check_fields {
> my $blanks;
> my @fields = qw(name email city state message);
> foreach my $field (@fields) {
> $blanks++ if !$q->param($field);
Using $q here has just broken encapsulation. If you move this subroutine
to the top of the file or into a library of its own it no longer works,
'strict' is not complaining because it is file scoped and sort of
"global". You should be passing in $q as an argument or similar rather
than getting lucky that its scoped to the whole file.
> }
> if($blanks) {
> print qq(Error: There were some blank fields.);
> exit;
You might consider using 'die' here instead of a print followed by 'exit'.
perldoc -f die
perldoc Carp
> }
> unless($q->param('email') =~ /[EMAIL PROTECTED]/) {
> print qq(Error: Please enter a valid email address.);
Before you exited on failure, why not here? Also the above regex is not
sufficient for checking email addresses. If you really want to check for
a valid email address you should consider,
Email::Valid
>From the CPAN.
> }
> send_email();
Why is calling send_email a part of checking the fields' validity? This
is more likely better off being called from the main. Subroutines should
usually take parameters and return a value. In this case you could
return nothing on success or an error message on failure, then your
error handling could be done outside of the routine.
> }
>
>
> sub send_email {
> my $m = Mail::Mailer->new('sendmail');
> $m->open({ From => $q->param('email'),
> To => 'perl <[EMAIL PROTECTED]>',
> Subject => '[INFO] Site Comment',
> }) or die $!;
Now your error handling has switched to using 'die'. You should use a
consistent approach, it is the consistency more than which approach that
you choose that will provide the most return in the long run. Again,
using $q here has broken the encapsulation.
> print $m "Name: " . ucfirst($q->param('name')) . "\n";
> print $m "Location: " . ucfirst($q->param('city')) . ", " .
> $q->param('state') . "\n\n";
> print $m $q->param('message');
> $m->close();
> print qq(Thank you for emailing us. We will contact you in the next 24
> hours.);
Printing the success is similarly misplaced to calling 'send_email' was
above. The subroutine should provide a single functionality, in this
case sending email. Again, let it return a value, if that value is
success then the handling of the content sent back to the user is done
in the main, otherwise handle the error. Keep similar functionality at
similar levels.
> }
>
As for security, it is often (always?) a good idea to turn on taint
checking.
perldoc perlsec
HTH,
http://danconia.org
--
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
<http://learn.perl.org/> <http://learn.perl.org/first-response>
