legLess has asked for the wisdom of the Perl Monks concerning the following question:

Perl Monks,

With some trepidation I present my first attempt at a Perl CGI application. The goal is very simple: let people add their email addresses to our launch announcement list. It works, and there aren't any bugs that I'm aware of.

I'd very much appreciate any and all feedback, on coding, style, Perlishness, security, speed, or anything else. LART me if you must, but at least tell me why first. Following are some notes and questions about the script. It's 190 lines, so it's below the read more line (I hope).

Thanks for your time.
#!/usr/bin/perl -wT use strict; use warnings; use diagnostics; use DBI; use CGI qw(:param); use CGI::Carp qw(fatalsToBrowser); # will be removed for production $|++; $CGI::POST_MAX = 512; # ################################################################## sub main { my($action, $email, $name, $warning_msg) = check_params(param('action'), param('email'), param('name')); my $script_loc = 'xxxxx'; write_header($script_loc); # HTML header, removed for brevity if ($action eq 'default') { write_default($script_loc); } elsif ($action eq 'confirm') { write_confirm($script_loc, $email, $name); } elsif ($action eq 'commit') { write_commit($email, $name); } elsif ($action eq 'warning') { write_warning($script_loc, $email, $name, $warning_msg); } write_footer(); # HTML footer, removed for brevity } # ################################################################## sub check_params { my($action, $email, $name) = @_; my $warning_msg = ''; if ($action eq 'Sign up') { $action = 'confirm'; } elsif ($action eq 'Confirm signup') { $action = 'commit'; } unless ($action =~ /^confirm|commit|warning$/) { return('default'); } else { $email = "\L$email"; if ($email =~ s/[^\w\-@\.\+]//gm) { $action = 'warning'; $warning_msg .= 'illegal characters removed'; } unless ($email =~ /^.+?@.+?\..+?/) { $action = 'warning'; $warning_msg .= 'wrong form'; } unless ($action eq 'warning') { if (duplicate_email($email)) { $action = 'warning'; $warning_msg .= 'duplicate email'; } } if ($name =~ s/[^\w \-]//g) { $action = 'warning'; $warning_msg .= 'illegal characters removed'; } return($action, $email, $name, $warning_msg); } } # ################################################################## sub write_default { my($script_loc) = @_; print <<DEFAULT; <FORM ACTION="$script_loc" METHOD="post"> <INPUT TYPE="submit" NAME="action" VALUE="Sign up"></FORM> DEFAULT } # ################################################################## sub write_confirm { my($script_loc, $email, $name) = @_; print <<CONFIRM; <FORM ACTION="$script_loc" METHOD="post"> <INPUT TYPE="submit" NAME="action" VALUE="Confirm signup"></FORM> # to make changes ... <FORM ACTION="$script_loc" METHOD="post"> <INPUT TYPE="submit"NAME="action" VALUE="Sign up"></FORM> CONFIRM } # ################################################################## sub write_warning { my($script_loc, $email, $name, $warning_msg) = @_; print <<WARNING; <FORM ACTION="$script_loc" METHOD="post"> $warning_msg <INPUT TYPE="submit" NAME="action" VALUE="Sign up"></FORM> WARNING } # ################################################################## sub write_commit { my($email, $name) = @_; my $dbh = database_connect(); my $origin = $ENV{'REMOTE_ADDR'}; my $time = get_time(); my $query = qq(INSERT INTO announce VALUES ('$email','$name','$origi +n','$time')); my $sth = $dbh->prepare($query) || die 'Query failed to prepare: ' . + DBI->errstr; $sth->execute || die 'Query failed to prepare: ' . DBI->errstr; $dbh->disconnect; if ($name) { $name = " $name"; } print <<COMMIT; <P>You've just been signed up$name; thank you!</P> COMMIT } # ################################################################## sub duplicate_email { my($email) = @_; if ($email =~ /\@xxxxx$/) { # our domain ;) return 1; } else { my $dbh = database_connect(); my $query = qq(SELECT email FROM announce WHERE email = '$email'); my $sth = $dbh->prepare($query) || die 'Query failed to prepare: ' + . DBI->errstr; $sth->execute || die 'Query failed to prepare: ' . DBI->errstr; my @rows = $sth->fetchrow_array; $dbh->disconnect; return @rows; } } # ################################################################## sub get_time { my ($sec, $min, $hour, $mday, $mon, $year) = gmtime(time()); $year += 1900; $mday += 1; $mon += 1; return("$year-$mon-$mday $hour:$min:$sec"); } # ################################################################## sub database_connect { my $dbh = DBI->connect("DBI:mysql:xxxxx;mysql_read_default_file=/usr +/home/xxxxx/.my.cnf","",""); unless (defined($dbh)) { die "Database error (connecting): " . DBI->errstr . "\n"; } return $dbh; } # ################################################################## main();

Replies are listed 'Best First'.
Re: First Perl CGI app; desirous of peer review.
by Beatnik (Parson) on Jun 24, 2001 at 13:25 UTC
      Thanks. I'd been assuming that Email::Valid would take an ice age, but I just tested it (duh) and (of course) I was wrong. So that's been changed.

      Update: Email::Valid appears to allow domains with underscores, but I thought that was illegal? Hmmph. More research ...
      --
      man with no legs, inc.

(ar0n) Re: First Perl CGI app; desirous of peer review.
by ar0n (Priest) on Jun 24, 2001 at 14:37 UTC
    About the DBI error handling: When you create the database handle, you can specify RaiseError => 1 as an option. This will do all the dying for you (i.e. you can remove all those die statements you have).
    my $dbh = DBI->connect("DBI:mysql:database", "user", "password", { Rai +seError => 1 });
    See the DBI manpage for more information.

    ar0n ]

      Thank you; that's good info, and it'll save me some typing.

      I'm still hoping to do something a little more elegant with the errors, like display a generic "Opps, the database barfed, try again or come back later" page and send myself an email.
      --
      man with no legs, inc.

Re: First Perl CGI app; desirous of peer review.
by Masem (Monsignor) on Jun 24, 2001 at 15:58 UTC
    A slightly better, and somewhat more readable, extensable way, to write your main sub is to use a hash instead of a nested switch. That is, you could write it as:
    my %functions = ( default => \&write_default, warning => \&write_warning, confirm => \&write_confirm, commit => \&write_commit );
    However, you would need to modify each sub to take the same arguments, which isn't that hard (see below). In addition, while your script does not do anything if the action type is unknown beyond print out header and footer (even though you have checked for it at check_param), you should still cover it here, so that your function calling routine can look like:
    # This is outside of main, in the 'global' block my %function = ( # see above ); # The rest is inside of main $action = ( $function{ $action } ) ? $action : 'default'; # you could also have an 'invalid' block here for an invalid # parameter &$function{ $action } ( $script_loc, $email, $name, $warning_msg );

    Dr. Michael K. Neylon - mneylon-pm@masemware.com || "You've left the lens cap of your mind on again, Pinky" - The Brain
      Thanks Michael; that's pretty code. You're right that it's a bit of a pain to add new arguments. As soon as I understand your hash example, I'll implement.

      And actually, check_param sets '$action' to 'default' if '$action' isn't recognized, so the script does print the default page, as it should.
      --
      man with no legs, inc.

Re: First Perl CGI app; desirous of peer review.
by chromatic (Archbishop) on Jun 24, 2001 at 21:48 UTC
    Regarding the SQL query in duplicate_email(), unless there's a compelling need to return anything more than true or false, I'd change it to SELECT COUNT(*) FROM email WHERE email='$email'. Why make the database do more work than it needs, especially if you're returning the parameter you passed in if there are duplicates. :) (Hey, this isn't a big inefficiency unless you have an oustandingly large number of rows to return, but it's a good principle.)

    Regarding Masem's suggestion for a dispatch table, instead of changing the function signatures for all of your subroutines, you can use anonymous subs:

    my %functions = ( default => sub { write_default($script_loc) }, warning => sub { write_warning($script_loc, $email, $name, $warnin +g_msg) }, confirm => sub { write_confirm($script_loc, $email, $name) }, commit => sub { write_commit($email, $name) }, );
    Ahh, closures.

    Update: mischief caught a missing curly brace.

      Thanks chromatic. You're right - count() is a much better way to do this. I don't totally understand Masem's suggestion, and yours is way over my head right now, but I'll understand 'em both by the end of the day.
      --
      man with no legs, inc.