Curtis,
Of course I'm not going to take this personally, this is the very reason I
submitted the code.
I appreciate your feedback. The reason I was frustrated with Todd is because it was
strictly
criticism. Basically the tone was "Your wrong, and you shouldn't be making your own
lib anyway".
Ok, I'm going to reply to your comments, then I have attached the full lib so you
can see how
the whole thing works. Granted the web site for the lib is blank because I lost that
page and
didn't have a back up for it. So, I have to rebuild the home page for it.
> 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.
Any suggestions on how to do this? Should I check to see if there are &'s and if not
attempt to
split on ;'s?
> my $form_hash = parse_form_input( $ENV{QUERY_STRING} );
> Now, if I have 20 programs, I have 20 places to mistype that.
If you will look at the full lib attached, you simply use:
use Form.pm
my %vars = Form();
Sorry for the confusion, I thought I had cleared up that those two functions were only
the guts of
the lib.
> next if ($variable !~ /\=/);
> Oh, and you don't need to escape the equals sign.
I know, but I usually escape all special characters so that I don't have to worry
about which ones I
do and don't have to escape. I know, dirty habit from when I first began learning
perl.
> if ($value ne ""){
> Not sure if I like that last line.
Yes, I know. I have earlier in on this group suggested that I would probably give the
option of
receiving empty value pairs. (or maybe it was in a different perl group, hmm, not sure)
> if (!defined $form_vars{$name}) { # if this is the first of this form input name.
> You want "exists", not defined.
Ok, any reasons why? They both seem to accomplish exactly the same result.
> if (!defined $form_vars{$name}) { # if this is the first of this form input name.
> $var_count{$name} = 0; # set the count of the array to 0 incase there is more than
>one
> $form_vars{$name} = $value; # setting 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
> $var_count{$name}++; # increase the count of the array for this form input name
> push(@{$form_vars{$name}}, $value); # adding the next element to the list
> }
> 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
Your right, beyond the first I don't need to track the count. I could have said
$var_count{$name}=1; instead of $var_count{$name}++; and I would have accomplished the
same thing.
I do need to track it that far to accomplish my desired result. That is that single
value pairs
only have hash element name = value. But multiple value same name sets would be name
= array_ref.
Actually, come to think of it, instead of keeping a count, I could simply say:
$form_vars{$name} = [($form_vars{$name})] if (ref($var_count{$name}) ne "ARRAY");
and that would accomplish what I wanted. Hmm, that's a line of code I haven't touched
for a good
year or two (I began programming in Commodore basic back in 1983 or 1984, some habits
never die,
counters for example).
Ok, let me finish up here.
Yes, my lines of code are long. For this I apologize. I work on 1600x1200 with
Homesite which
doesn't wrap lines unless I ask it to. You know, if you have the same circumstances
as I do, it
sure makes for easy reading (less vertical lines, and all same command on the same
line and so on).
I do handle POST, I guess I didn't make that clear before. Read through the
attached lib and
you will see that is true. That is what parse_mform_input is about.
Yes, everybody does need to print the header and other such things. CGI.pm does
provide
functions for this, but that is not what this lib was intended for. I wrote this to
accomplish one
specific task quickly, efficiently, and the way I wanted it done, and that task was to
receive data
and put it into a very useable format. I didn't intend to write a CGI.pm replacement,
that makes no
sense at all (there are way too many functions there that I for one will never use,
and am not
interested in, though I understand others are). Most scripts that I have seen do
their own print
headers and cookies without regard to CGI.pm as stands. Infact, most that I have seen
only use
CGI.pm for the retrieval of data. You know, I didn't even write this lib with intent
to give CGI.pm
competition. I simply wanted a lib that did as I described above, and once I had
created it,
wondered if I was the only one who would be interested in such a library. I don't
believe that I
am, so I figured I would share it with the rest of the world as well.
I appreciate the feedback, and hope with the full lib in hand I will get more.
Also remember that I have a few options I already plan to add to it that are currently
not there.
- Option to disable upload
- Option to limit upload file size
- Option to receive all name value sets regardless of if the values are empty
- Option to receive get along with post
I think there is one more that keeps eluding me, but anyway, that's the jist of it.
Thanks,
David
----- Original Message -----
From: "Ovid" <[EMAIL PROTECTED]>
To: <[EMAIL PROTECTED]>; <[EMAIL PROTECTED]>
Sent: Wednesday, July 03, 2002 9:35 AM
Subject: Re: Form.pm
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]
Form.pm
Description: Binary data
-- To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
