Hi David,
I hope you don't take this stuff personally. A code critique is a critique, after all
:)
I'll skip the lines of code that I'm not commenting on.
my $input = shift;
my @variables = split(/\&/, $input);
First, we know that the split is incorrect. A semi-colon is the new, preferred
delimiter for form data, beginning with the HTML 4.0 specification. See
http://www.w3.org/TR/REC-html40/appendix/notes.html#ampersands-in-uris for more
information. And yes, I *have* seen user agents using semicolons.
However, that's not my biggest concern. You're forcing the use to do something
like this:
my $form_hash = parse_form_input( $ENV{QUERY_STRING} );
Now, if I have 20 programs, I have 20 places to mistype that. This will
increase the chance of bugs and is a serious problem. The user needs to be
shielded from such implementation details.
Further, while you acknowledge that you don't currently handle POST requests, I
consider this to be a bug, not a limitation. GET and POST requests are only
similar on the surface. Their actual behavior is different enough that leaving
off POST cripples this code.
foreach $variable (@variables) {
Hmm... you didn't predeclare $variable. It looks like you're not using strict.
Tsk, tsk. If someone else using this code has a global named $variable in the
same namespace, you've just clobbered it.
next if ($variable !~ /\=/);
Skip if no '='? I guess this code doesn't handle ISINDEX, either, but I won't
cry over that. Who uses it anyway? (I'm sure I'm going to hear from two of
the three people who still do :) Oh, and you don't need to escape the equals
sign.
if ($value ne ""){
Not sure if I like that last line. If the value evaluates as false, you throw
away the name. I'm not sure the end-user is going to appreciate the data loss.
This could become a problem is they're doing something with the names and they
have them hard-coded into their code.
if (!defined $form_vars{$name}) { # if this is the first of this form input
name.
You want "exists", not defined.
$var_count{$name} = 0; # set the count of the array to 0 incase there is
more than one
$form_vars{$name} = $value; # seting the variable the hash with the name to
the value
} else { # for repeated times in with the same form input name
$form_vars{$name} = [($form_vars{$name})] if ($var_count{$name} == 0); # set
the first
element to the first input if this is only the second input with this name
First, there's no reason to track the var count since you just throw it away.
Second, you can skip the entire if/else structure and just give people a array
references for the values. Some people aren't familiar with them, so you could
provide nice, transparent accessors to them. Then, you have plenty of control
over access to your data structure.
This is one of the nice benefits of CGI.pm's param() method. All of the form
values are stored internally as array refs, but param() uses 'wantarray' to see
if you just want a scalar value or all of them. This means that internally
there is only one data structure type for these values and less change of bugs.
Okay, that's my general comments on the first subroutine. I'll tackle
parse_mform_input() later. However, I'll go ahead and make some general
comments on the code.
First, your lines are waaaaaaaaaaaaaay too long. Different editors and
different resolutions allow for different lines lengths to be comfortably
displayed. In every single editor that I have, your lines of code wrap around
the screen. This makes it very difficult for me to trace the execution of your
program. By limiting your line lengths to, say, 80 characters or so, you can
ensure that others can easily read through your code. Further, if you need to
work on your code on someone else's computer (which can be common in a
workplace), then you are less likely to be cursing wrapping lines.
If you added POST handling to your code, you might think it complete. I would
disagree because of the limitations in your code. For example, how many times
do we see stuff like the following:
print "Content-type:text/html\n\n";
print "ContentType: text/plain\n\n";
print "Set-Cookie: name=Ovid; expires=Friday,31-Dec-02 23:59:59 GMT;
domain=ovidinexile.com;\n";
Every one of those three lines has an error. Some of them, the browser (or
perhaps the Web server) will correct for. Others will not be corrected.
Further, many programmers, even experienced Web programmers, will not spot the
errors right away. Now, if most Web programs only parsed form input, your code
would be fine, but that's not the case. These programs print headers and
generate cookies. They *have* to. As a result, any module that is going to
handle form data will have to handle headers and cookies for the user rather
than allow users to keep making typos like I did in the above code.
Naturally, not all of the errors that a user can make will be eliminated by
code that handles this for them, but anything that reduces their error rate for
known repetitive actions (headers and cookies) is pretty much mandatory for
code that is to benefit Web programming.
Cheers,
Curtis "Ovid" Poe
=====
"Ovid" on http://www.perlmonks.org/
Someone asked me how to count to 10 in Perl:
push@A,$_ for reverse q.e...q.n.;for(@A){$_=unpack(q|c|,$_);@a=split//;
shift@a;shift@a if $a[$[]eq$[;$_=join q||,@a};print $_,$/for reverse @A
__________________________________________________
Do You Yahoo!?
Sign up for SBC Yahoo! Dial - First Month Free
http://sbc.yahoo.com
--
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
