Re: Re: Re: "Correct" program style questions

[sauoq]

Of course, I can't say that mine is much clearer in that regard.

It isn't. I felt ickier the more I looked at that code for handling params too.

There is no reason to try to differentiate between undef and the empty string in this case. Interpolated, printed, or otherwise stringified, undef is an empty string. So,

# Cargo-cultish version with a temp variable.
my $_name  = param('name') || '';
my ($name) = $_name =~ /^([[:alpha:]]+)$/;

is equivalent to

# BrowserUk's cargo-cultish only version.
my ($name) = (param('name') || '') =~ /^([[:alpha:]]+)$/;

which is also equivalent to

# My preferred version.
my ($name) = param('name') =~ /^([[:alpha:]]+)$/;

Using the construct param('name') || '' at all is questionable. It throws away information. CGI returns an empty string in the case that the parameter was submitted with no value and undef in the case that it wasn't submitted at all. If you need to differentiate between those cases then you can check for undef. If you don't need to, you can ignore the difference (as above.)

-sauoq
"My two cents aren't worth a dime.";

[BrowserUk]

Hmmm. My post, as stated, was simply asking why Ovid chose to use a temporary variable where it wasn't necessary. I'm beginning to hate the sight of that phrase 'cargo-cultish'. Especially when it is wrongly used to mean 'not the way I would do it'.

Using your version, if the 'name' param is completely omitted, then param('name') as you state will return undef. Which means that the attempt to apply the m// to results in

Use of uninitialized value in pattern match (m//) at ....

Which is exactly why the ||'' is there in the first place.

Your correct that this only delays the need to use defined to test for this, but I think that that delay is beneficial. Vis. Instead of,

my $_name = param('name');
my ($name) = $_name =~ /^([[:alpha:]]+)$/ if defined $_name;
my $_color = param('color');
my ($color) = $_color =~ /^([[:alpha:]]+)$/ if defined $_color;
my $_otherReqParam = param('otherReqParam');
my ($otherReqParam) = $_otherReqParam =~ /^([[:alpha:]]+)$/ if defined
+ $_otherReqParam;
# ... and so on for all the other params, and then
print 'location: http:/www.somesite.com/error.htm', $/, $/
     unless defined $name
     and    defined $color
     and    defined $otherReqParameter;
....

You get the simpler, cleaner

my ($name) =          (param('name')         ||'') =~ /^([[:alpha:][:p
+unct:][:space:]]+)$/;
my ($color) =         (param('color')        ||'') =~ /^([[:alpha:]]+)
+$/;
my ($otherReqParam) = (param('otherReqParam')||'') =~ /^([[:alpha:]]+)
+$/;

#deal with all other params in a similar fashion.
print 'location: http:/www.somesite.com/error.htm', $/, $/
     unless defined $name
     and    defined $color
     and    defined $otherReqParam;
     
# If we got here we have everything we need, untainted.
...

This allows all the parameter validation to be done at the top of the script without needing temporary variables or to test at least twice for definedness, nicely grouped and with a consistant style, and allows us to bottle out early if we haven't got everything we need.

Not the only way to do it, maybe not the best way to do it, but it is a valid way and not without its merits. And its far from what I understand the definition of "cargo-cultish" to mean.

---

Cor! Like yer ring! ... HALO dammit! ... 'Ave it yer way! Hal-lo, Mister la-de-da. ... Like yer ring!

[sauoq]

I'm beginning to hate the sight of that phrase 'cargo-cultish'.

I'm sick of the term myself. I believe, however, that this was the first time I've ever actually used it. I used it with its proper meaning in mind too, although it may not have fit as well as I thought it did. I honestly thought the or-empty construct was being included more by habit than for any real purpose. Of course, it does avoid a possible warning and I suppose that's reason enough for it. Maybe. It still bugs me.

I'm of the opinion that you shouldn't process parameters unless you expect them to be there. If you expect them but they aren't there, then the warnings are a good thing. If, however, you really don't want the warnings then simply turn them off.

-sauoq
"My two cents aren't worth a dime.";

[BrowserUk]

One reason for processing parameters that "aren't there" is to check that someone isn't modifying a URI in an attempt to break the code?

This seems like an appropriate way to do that and bottle out early to an error page, before going on to validate correctly passed parameters. That way you get a single log entry for each mal-formed query string, noting all the information you need rather than (potentially) several dozen essentially similar warnings, one for each missing parameter.

---

Cor! Like yer ring! ... HALO dammit! ... 'Ave it yer way! Hal-lo, Mister la-de-da. ... Like yer ring!

[Ovid]

I'd like to plead 'no contest', your honor.

I build some fairly decent sized Web sites (well, the code for them, anyway) and for one of them, because of the way the pages are constructed, sometimes I have a parameter or two that simply doesn't appear, due to the user's permissions. As a result, I have similar pages, generated by the same templates, handled by the same Perl code, but some parameters simply will not be there. That's why I started using constructs such as I listed. However, I realize that mine is an exceptional case and shouldn't be used by most.

Because the site is rather large, pouring over all of the code just to find those few parameters that I am surpressing warnings for seems like overkill (and I doubt I could get the client to pay for it :)

Cheers,
Ovid

Join the Perlmonks Setiathome Group or just click on the the link and check out our stats.