Beefy Boxes and Bandwidth Generously Provided by pair Networks
Pathologically Eclectic Rubbish Lister

Re: "Correct" program style questions

by BrowserUk (Pope)
on Oct 23, 2002 at 10:12 UTC ( #207344=note: print w/replies, xml ) Need Help??

in reply to "Correct" program style questions

Not so much a suggestion as a question arising... Is there any particular reason for using temporary vars as opposed to say

#!/usr/bin/perl -wT use strict; use CGI qw/:standard/; use HTML::Entities; my ( $name ) = (param('name') || '') =~ /^([[:alpha:][:punct:][:s +pace:]]+)$/; my ( $color ) = (param('color') || '') =~ /^([[:alpha:]]+)$/; encode_entities( $name ); print header, <<"END_HTML"; <html> <head><title>Test page</title></head> <body> <p>Your name is "$name" and the color you listed was "$color"</p> </body> </html> END_HTML

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

Replies are listed 'Best First'.
Re: Re: "Correct" program style questions
by Ovid (Cardinal) on Oct 23, 2002 at 14:44 UTC
    my ( $color ) = (param('color') || '') =~ /^([[:alpha:]]+)$/;

    That's an interesting line. My only immediate problem with it is that we come closer to obfuscating the code. People will likely understand the intent, but not necessarily see the subtleties. For instance, $color will be undef and not the empty string, if param('color') doesn't return anything. However, if the regular expression was stripped from the end of that, then $color would indeed default to the emptry string. Of course, I can't say that mine is much clearer in that regard.


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

      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.)

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

        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:/', $/, $/ 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:/', $/, $/ 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!

      it's worse than obfustication, it's out right betrayal worthy of the machiavellian-est of perl programers. if param('color') returns a false value (be it undef or the empty string) the regex against '' is going to always return undef.

      I assume you simply want to provide a default value for $color if a) it's not present (param('color') returns empty string or undef) or b) it isn't a sequence of only [:alpha:] chars.

      use locale; my $color = param('color') =~ /^\w+$/ ? param('color') : "default color";
      or you could write it as:
      my $color = "default color"; $color = param('color') if param('color') =~ /^\w+$/;

      of course, the real solution to this is:

      use locale; sub is_alpha { my ($datum) = @_; # return "" so we don't get unitialized value errors $datum =~ /^(?:\w+)$/ ? $datum : ""; } my $color = is_alpha(param('color')) || "default value";

      Or why not sub class CGI and add these checks to CGI::param? Have MyCGI::param return an object with color, name, etc methods so you could just write $cgi->param->color;

Log In?

What's my password?
Create A New User
Node Status?
node history
Node Type: note [id://207344]
and all is quiet...

How do I use this? | Other CB clients
Other Users?
Others chilling in the Monastery: (10)
As of 2018-06-25 12:55 GMT
Find Nodes?
    Voting Booth?
    Should cpanminus be part of the standard Perl release?

    Results (126 votes). Check out past polls.