QC: good function?

[Ken Sington]

Here's a function I wrote.
The function is incomplete at the moment because I'm still writing it.
But from what I have now, what do you think?

If you just look at the code and the comments; if I suddently became rich and
quit my job and you just took over this contract (we never meet), and you see code like the function
below, will you have any trouble?




#---------------------------------.---------------------------------#
# ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ . ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ #
# ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ PASSING PARAMETERS ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ #
# ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ . ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ #
#===============================--.--===============================#
# to use:
# &paramPass('option','name=value',$ENV{'QUERY_STRING'});
# arg 1: options
# add <-- add new name=value pairs
# change <-- changes values for each name given
# explicit <-- return only these name=value pairs
# delete <-- delete name=value pairs
# arg 2: items
# if option=add <-- provide new name=value&name2=value2 pairs to be added
# if option=change <-- provide name=newValue&name2=NewValue2& etc...
# if option=explicit <-- provide varName&varName2 of pairs to be passed
# if option=delete <-- provide varName&varName2 of pairs to be deleted
# arg 3: $ENV{QUERY_STRING}
sub paramPass {
my ($option,$nameValue,$env)=@_;
my $returnIt;
my $work = $env;
if ($option eq "add"){
$returnIt = $env . "&" . $nameValue;
} else {
my @env = split /&/, $env;
my @nameValue = split /&/, $nameValue;
for my $line (@env){
my ($name, $value) = split /=/,$line,2; # solves problem: name=value=3445, where
value \
has "=" in it
my $blnMatched="false";
my ($nHold,$vHold);
for my $nv (@nameValue){
my ($n,$v) = split /=/,$nv, 2;
($nHold, $vHold, $blnMatched) = ($n, $v, "true") if ($name eq $n);
}
$work =~ s/$name/$nHold=$vHold/ if ($blnMatched eq "true" && $option eq "change");
# placeholder...up to here
}
$returnIt = $work;
}

return $returnIt;
}

 

[Ken Sington]

PS:

This function is called by another perlpage.cgi

so we have page1.cgi with these values:
login=joe
password=schmoe98
bankaccount=345678
bankpin=1234

and a "GET" link
page2.cgi?login=joe&password=schmoe98&bankaccount=345678&bankpin=1234

but we want to encrypt the password, so we call the function to change the name=value pair:
$password is the param(password) which is schmoe98.
$url = paramPass ('change','password=$crypticpassword',$ENV{'QUERY_STRING'});

now, page2.cgi wants to add a new name=value pair:
$url = paramPass ('add','mycolor=blue',$ENV{'QUERY_STRING'});


so, now we can:
<a href="?$url">click here</a>

 

[A. Sinan Unur]

Here's a function I wrote.
The function is incomplete at the moment because I'm still writing it.
But from what I have now, what do you think?

If you just look at the code and the comments; if I suddently became
rich and quit my job and you just took over this contract (we never
meet), and you see code like the function below, will you have any
trouble?

Probably yes.

#---------------------------------.---------------------------------#
# ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ . ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ #
# ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ PASSING PARAMETERS ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ #
# ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ . ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ #
#===============================--.--===============================#

Instead of this cutesy block, I would prefer to see a plain English
description of what problem this sub solves, what context it is intended
to be used etc.

# to use:
# &paramPass('option','name=value',$ENV{'QUERY_STRING'});

There are implications to using & before sub invocations. See perldoc
perlsub. Do you need those features? If not, prefer the plain form:
paramPass( ... );

# arg 1: options
# add <-- add new name=value pairs
# change <-- changes values for each name given
# explicit <-- return only these name=value pairs
# delete <-- delete name=value pairs
# arg 2: items
# if option=add <-- provide new name=value&name2=value2 pairs
to be added # if option=change <-- provide
name=newValue&name2=NewValue2& etc... # if option=explicit <--
provide varName&varName2 of pairs to be passed # if option=delete
<-- provide varName&varName2 of pairs to be deleted # arg 3:
$ENV{QUERY_STRING}

Your message is badly formatted. Are you aware that ; is a valid
separator?

sub paramPass {
my ($option,$nameValue,$env)=@_;
my $returnIt;
my $work = $env;

You have already passed the actual query string, then created a copy of
it in $env (which is a bad name, if you are processing the query string,
then name your variable to reflect that). Why make another copy?

if ($option eq "add"){
$returnIt = $env . "&" . $nameValue;

$returnIt = "$env&$nameValue";

} else {
my @env = split /&/, $env;

Do you realize that ; is a valid separator?

my @nameValue = split /&/, $nameValue;

I thought $nameValue held a string like 'name=value'. Why split on &?

for my $line (@env){
my ($name, $value) = split /=/,$line,2; # solves problem:
name=value=3445, where value has "=" in it
my $blnMatched="false";
my ($nHold,$vHold);
for my $nv (@nameValue){
my ($n,$v) = split /=/,$nv, 2;
($nHold, $vHold, $blnMatched) = ($n, $v, "true") if
($name eq $n);
}
$work =~ s/$name/$nHold=$vHold/ if ($blnMatched eq "true"
&& $option eq "change"); # placeholder...up to here
}
$returnIt = $work;
}

I lost track of what you are doing here.

Why not take a look at CGI.pm? If not to use it, at least to learn from
it.

 

[Sherm Pendley]

If you just look at the code and the comments; if I suddently became rich
and quit my job and you just took over this contract (we never meet), and
you see code like the function below, will you have any trouble?

First, I would see the "cute" banner comment and nearly total lack of
comments elsewhere. Strike one.

Then, I would see the use of & to call a subroutine when it isn't needed.
Strike two.

And finally, I'd see that it's a poorly-reinvented wheel, written by someone
who is either unaware that CGI.pm already has methods for this, or is aware
of it but afflicted with NIH syndrome. Strike three.

After three easy strikes, I'd delete this subroutine from my programs and
from my mind, and update any code that used it to use the standard CGI.pm
instead.

I've rescued disastrous projects and cleaned up dodgy code like this many
times. It's no trouble at all.

sherm--

 

[Tad McClellan]

Here's a function I wrote.

Why did you write it?

Because you need to do what it does in Real Programs or as
a learning exercise?

If you need CGI param processing in a Real Program, then you
should use a module that knows how to do it.

It is tricky to get right, and you've gotten it very wrong...

But from what I have now, what do you think?

I'd give it a grade of "D".

It would not stay on my hard disk long.

my ($option,$nameValue,$env)=@_;

Whitespace is not a scarce resource, feel free to use as much as
you like to make your code easier to read and understand:

my($option, $nameValue, $env) = @_;

See also:

perldoc perlstyle

my $returnIt;
my $work = $env;
if ($option eq "add"){
$returnIt = $env . "&" . $nameValue;

I'd replace that with:

return "$env&$nameValue" if $option eq 'add';

and save a level of indentation below.

(Not really. I would really use a URL-encoding module to do URL-encoding
correctly for me.
)

my @env = split /&/, $env;

for my $line (@env){

You do not need the @env temporary variable:

foreach my $line ( split /&/, $env ){

my $blnMatched="false";

The string "false" is a TRUE value in Perl.

Are you trying to confuse folks?

 

[krakle]

PS:

This function is called by another perlpage.cgi

so we have page1.cgi with these values:
login=joe
password=schmoe98
bankaccount=345678
bankpin=1234

and a "GET" link
page2.cgi?login=joe&password=schmoe98&bankaccount=345678&bankpin=1234

I see Username, Password and Bank Account and Pin in the URL... If I
was your boss I would fire you... Not because you plan to encrypt the
password but because you felt the need to write it this way to begin
with....

 

[Ken Sington]

Why did you write it?

Because you need to do what it does in Real Programs or as
a learning exercise?

I may use it. or I may use something else.
It's also for exercise.

If you need CGI param processing in a Real Program, then you
should use a module that knows how to do it.

I guess I'll use something else.

It is tricky to get right, and you've gotten it very wrong...
ok. I'll use something else.
....




The string "false" is a TRUE value in Perl.

Are you trying to confuse folks?

No sir.
"", undef = false
"blah" = true
just feels strange to me.

 

[Tad McClellan]

No sir.
"", undef = false
"blah" = true
just feels strange to me.

How do you feel about

0 = false
1 = true

??


That would be less likely to confuse a maintenance programmer.

 

[Sherm Pendley]

I see Username, Password and Bank Account and Pin in the URL... If I
was your boss I would fire you...

Agreed. Ken, do you realize that by using GET, you are storing every one of
these parameters in a clear-text, world-readable log file?

So, how did someone with *zero* knowledge of writing secure code manage to
land a job at a bank, anyway? And what bank do you work at - I'd like to
know so I can make sure not to open an account there.

sherm--

 

[Ken Sington]

krakle wrote:


....


Agreed. Ken, do you realize that by using GET, you are storing every one of
these parameters in a clear-text, world-readable log file?

POST can also be intercepted.
alright, so for now, I'll use GET for testing purposes.

So, how did someone with *zero* knowledge of writing secure code manage to I don't claim that.
land a job at a bank, anyway? And what bank do you work at - I'd like to

I work at a bank?

 

[Ken Sington]

....




How do you feel about

0 = false
1 = true

??

feels better.

so what's the normal way to pass name/values from one cgi page to another

That would be less likely to confuse a maintenance programmer.

one day, I'll fool people into thinking I'm a programmer.

 

[Sherm Pendley]

POST can also be intercepted.

I'm not referring to interception. GET, by default and without the
slightest bit of "hacking" required, stores the full URL - including the
query string - in server logs.

Server logs are usually world-readable, so if you're on a shared server,
anyone with an account on that server can read them.

sherm--

 

[krakle]

I'm not referring to interception. GET, by default and without the
slightest bit of "hacking" required, stores the full URL - including the
query string - in server logs.

Server logs are usually world-readable, so if you're on a shared server,
anyone with an account on that server can read them.

sherm--

Im sure GET & POST can be intercepted... Ken, why don't you store the
vital information in a database and use some sort of random or
encryted string as the key and pass that through GET/POST/COOKIES to
retrieve the vitals...