On Thu, Jun 28, 2001 at 09:06:38AM -0700, Jon Riddle wrote:
Be forewarned, what follows is a critique of code you didn't ask about.
> #!/usr/local/bin/perl -w
>
> use CGI;
> use CGI::Carp qw(carpout fatalsToBrowser);
Always:
use strict;
when debugging code.
> $Q = CGI;
This assigns the string 'CGI' to $Q. I doubt that's what you intended.
my $Q = new CGI;
Remove this from here:
> $content_length = $ENV{'CONTENT_LENGTH'};
> read (STDIN, $posted_information, $content_length);
>
> $posted_information =~ s/%([\dA-Fa-f][\dA-Fa-f])/pack ("C", hex ($1))/eg;
> $posted_information =~ s/\+/ /g;
>
> @fields = split (/&/, $posted_information);
to here.
Do not roll your own CGI parser, you will almost assuredly miss something.
> ($label, $name) = split (/=/, $fields[0]);
> ($label, $street) = split (/=/, $fields[1]);
> ($label, $city) = split (/=/, $fields[2]);
> ($label, $state) = split (/=/, $fields[3]);
> ($label, $zip) = split (/=/, $fields[4]);
> ($label, $PHONE_AREA) = split (/=/, $fields[5]);
> ($label, $PHONE_PRE) = split (/=/, $fields[6]);
> ($label, $PHONE_SUFF) = split (/=/, $fields[7]);
> ($label, $serviceProvider) = split (/=/, $fields[8]);
> ($label, $lapDance) = split (/=/, $fields[9]);
> ($label, $lapDance2) = split (/=/, $fields[10]);
> ($label, $lapDance3) = split (/=/, $fields[11]);
> ($label, $lapDance4) = split (/=/, $fields[12]);
> ($label, $lapDance5) = split (/=/, $fields[13]);
Use:
my $name = $Q->param('name' );
my $street = $Q->param('street');
etc.
Better yet, don't assign them to temporary variables, use them directly when
necessary, or stick them in a hash.
> #Compares correct anwsers
> $lapResults .= ",$lapDance,$lapDance2,$lapDance3,$lapDance4,$lapDance5";
> if ($lapResults eq
> ",AvaVincent,JessicaDrake,JewellDeNyle,ShaylaLaVeaux,ShaySweet") { $correct
> = 1}
This looks like a bad way of doing this. You should probably put the
expected names in an array, then iterate over each of your parameters and
make sure they're in the array.
my @params = ($lapDance, $lapDance2, $lapDance3, $lapDance4, $lapDance5);
my $correct = 1;
for (my $i = 0; $i < @params; $i++) {
if ($params[$i] ne $EXPECTED_NAMES[$i]) {
$correct = 0;
last;
}
}
> if (!$correct) {
> print $Q->redirect('http://www.ten.com/contest/thanklost.htm');
> }
> else {
> if (one_time(1)) {
> print
> $Q->redirect('http://www.ten.com/contest/already.html');
> }
> else {
> open (OUTPUT, ">> lapdance.db") || die "Cannot open lapdance.db: $!";
> print OUTPUT $name, ":", $street, ":", $city, ":", $state, ":",
> $zip,":", $PHONE_AREA, ":", $PHONE_PRE, ":", $PHONE_SUFF,"\n";
Use join:
print OUTPUT join(":", $name, $street, $city, $state, ...), "\n";
> close OUTPUT;
> print $Q->redirect('http://www.ten.com/contest/thankwon.htm');
> }
> }
> # Routine that checks DB for previous valid
> # entry comparing telephone number
> sub one_time {
> open (VARIFY, "< lapdance.db") || die "Cannot open lapdance.db: $!";
> while (<VARIFY>) {
> $stats = <VARIFY>;
This:
> $compare = { split (/:/, $stats)};
> @records = \$compare;
is very likely the source of your asked problem. You are constructing an
anonymous hash with '$compare = { split (/:/, $stats)}', then assigning a
reference to that anonymous hash to $records[0].
Replace this with:
@records = split(/:/, $stats);
> if (($records[6] eq $PHONE_AREA) && ($records[7] eq
> $PHONE_PRE) && ($records[8] eq $PHONE_SUFF)) {
> $sorry = 1;
> }
> }
> close VARIFY;
> return $sorry;
> }
> ##################### EOF ###############################
Michael
--
Administrator www.shoebox.net
Programmer, System Administrator www.gallanttech.com
--
