Jason Gray wrote:
What do you exactly mean by passing $q as an argument?
In general you want to avoid the use of globals, and "encapsulate" your subroutines such that they only use information that has been specifically given to them, either through their argument list or because they are a method call (aka object/class, etc.). In your code your subroutines access $q, which is not provided to the subroutine in one of these manners, it just happens to be available because the subroutines are in the same file as the initialization of $q, and because it is file scoped. If you move the subroutines into their own library, or above the rest of the main body of the script they will no longer work and will complain about not knowing what $q is. As your code gets more advanced you will want to be able to move large chunks of code without worrying about what may or may not break because of a particular scope or another. If for no other reason than it becomes difficult to test and debug. This is a difficult case because $q acts like a singleton, which is the one time you might want to use this approach, but in that case you are better off making it a package variable and using its fully qualified name, so that moving the subroutines into a library will still have access to the variable, even as a singleton (though I don't really like this approach either).
So you end up with either the passing method, such that your calls look like:
check_field($q);
Then in the subroutine you would add something similar to
sub check_fields {
my $query = shift;# use $query here instead of $q (name doesn't matter) ... }
Or you could declare $q like,
our $q = new CGI;
Then in your subroutine,
sub check_fields {
# use $main::q to access the variable here
}If you feel that going through this when creating your subroutines is overkill or not beneficial, then I would do as the other poster mentioned and re-examine whether you really needed the code put into subroutines in the first place.
Does this help at all? This is a pretty basic (though difficult) design philosophy, and I feel I haven't done a very good job explaining it.
http://danconia.org
----- Original Message ----- From: "Wiggins d Anconia" <[EMAIL PROTECTED]>
To: "Jason Gray" <[EMAIL PROTECTED]>; <[EMAIL PROTECTED]>
Sent: Tuesday, June 15, 2004 10:18 AM
Subject: Re: a way to make this more secured and better written?
#!/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>
