[24634] in Perl-Users-Digest
Perl-Users Digest, Issue: 6798 Volume: 10
daemon@ATHENA.MIT.EDU (Perl-Users Digest)
Tue Aug 3 12:21:04 2004
Date: Tue, 3 Aug 2004 09:20:31 -0700 (PDT)
From: Perl-Users Digest <Perl-Users-Request@ruby.OCE.ORST.EDU>
To: Perl-Users@ruby.OCE.ORST.EDU (Perl-Users Digest)
Perl-Users Digest Tue, 3 Aug 2004 Volume: 10 Number: 6798
Today's topics:
any pointers please? combine words script <me@example.com>
Re: any pointers please? combine words script <jurgenex@hotmail.com>
Re: any pointers please? combine words script <me@example.com>
Re: any pointers please? combine words script <nobull@mail.com>
Re: any pointers please? combine words script <jurgenex@hotmail.com>
Re: any pointers please? combine words script <me@example.com>
Re: any pointers please? combine words script <uri@stemsystems.com>
Re: any pointers please? combine words script <nobull@mail.com>
Re: any pointers please? combine words script <nobull@mail.com>
Re: any pointers please? combine words script <me@example.com>
Re: any pointers please? combine words script <uri@stemsystems.com>
Re: any pointers please? combine words script (Anno Siegel)
Re: any pointers please? combine words script (Anno Siegel)
Re: any pointers please? combine words script <me@example.com>
Re: any pointers please? combine words script <me@example.com>
Re: any pointers please? combine words script nobull@mail.com
Re: any pointers please? combine words script <uri@stemsystems.com>
Re: any pointers please? combine words script <me@example.com>
Re: any pointers please? combine words script <me@example.com>
Re: any pointers please? combine words script <me@example.com>
Re: any pointers please? combine words script <uri@stemsystems.com>
Digest Administrivia (Last modified: 6 Apr 01) (Perl-Users-Digest Admin)
----------------------------------------------------------------------
Date: Sat, 31 Jul 2004 12:31:21 -0400
From: steve_f <me@example.com>
Subject: any pointers please? combine words script
Message-Id: <g2ing01cvi4ffb7tkosflc2anlpuj7udbr@4ax.com>
Hey, if you got a minute have fun with this script and give me some pointers.
The combine_hash idea was probably wrong, I had it before to run a few
scripts off of main.cgi but it got out of hand.
Oh, you can also find it online at http://www.hudsonscripting.com ;-)
#!/usr/bin/perl -Tw
use strict;
#use CGI::Carp qw(fatalsToBrowser);
my $query = $ENV{QUERY_STRING};
my %config = combine_hash($query);
my %kv = get_input($config{cgi});
$config{user_input} = $kv{words};
print header($config{title});
for my $key (keys %kv) {
for my $option (@{ $config{options}}) {
if ($key eq $option) {push @{ $config{user_config}}, $key}
}
}
print table($config{intro},form(%config, %kv));
if ($kv{got_input} eq "yes") { print format_output($config{action}(%kv)); }
exit(0);
sub combine_hash {
my $query = shift;
my ($language, $num);
if ($query =~ /mkt=(\w+)/) { $language = $1 }
if ($query =~ /boxes=(\w+)/) { $num = $1 }
if (!$num) { $num = 2 }
my $intro =
"<b>Combine words</b>";
$intro .= "<small> - choose number of boxes: ";
$intro .= menu($num, 1, "combine.cgi?boxes=1",
2, "combine.cgi?boxes=2",
3, "combine.cgi?boxes=3");
$intro .= "</small><br><br>";
my @options = get_options($num);
my %page = (script => "combine.cgi?boxes=$num",
cgi => [get_textareas($num), @options],
title => 'Combine two lists',
intro => $intro,
textarea => [ get_textareas($num) ],
options => [@options],
defaults => ['no quotes', 'reverse'],
submit => get_submit($num),
action => sub { my %kv = @_;
my @array = combine_lists(%kv);
return @array;
},
);
}
sub get_submit {
my $num = shift;
if ($num == 1) { return 'combine one' }
if ($num == 2) { return 'combine two' }
if ($num == 3) { return 'combine three' }
}
sub menu {
my $source = shift;
my %hash = @_;
my @temp;
for my $element (sort keys %hash) {
if ($element eq $source) { push(@temp, "<b>$source</b>") }
else { push(@temp, "<a href=\"./$hash{$element}\">$element</a>") }
}
my $output .= join(" | \n", @temp);
return $output;
}
sub get_options {
my $num = shift;
if ($num == 1) { return 'no_quotes', 'quotes', 'brackets', 'skip_space'}
elsif ($num == 2) { return 'no_quotes', 'quotes', 'brackets',
'reverse_words', 'skip_space'}
elsif ($num == 3) { return 'no_quotes', 'quotes', 'brackets',
'reverse_words', 'skip_space'}
else { return 'no_quotes', 'no_quotes', 'quotes', 'brackets', 'skip_space'}
}
sub get_textareas {
my $num = shift;
if ($num == 1) { return 'kw_1'}
elsif ($num == 2) { return 'kw_1', 'kw_2'}
elsif ($num == 3) { return 'kw_1', 'kw_2', 'kw_3'}
else { return 'kw_1'}
}
sub combine_lists {
my %kv = @_;
my @array;
if ($kv{kw_1} && !$kv{kw_2} && !$kv{kw_3}) {
@array = split /\n/, $kv{kw_1};
if ($kv{skip_space}) { push @array, simple_merge(@array) }
}
if ($kv{kw_1} && $kv{kw_2} && !$kv{kw_3}) {
@array = combine_two($kv{kw_1}, $kv{kw_2});
if ($kv{reverse_words}) {
push @array, combine_two($kv{kw_2}, $kv{kw_1});
}
if ($kv{skip_space}) { push @array, simple_merge(@array) }
}
if ($kv{kw_1} && $kv{kw_2} && $kv{kw_3}) {
@array = combine_three($kv{kw_1}, $kv{kw_2}, $kv{kw_3});
if ($kv{reverse_words}) {
push @array, combine_three($kv{kw_3}, $kv{kw_2}, $kv{kw_1});
}
if ($kv{skip_space}) { push @array, simple_merge(@array) }
}
my @return_array;
if ($kv{no_quotes}) { @return_array = @array }
if ($kv{quotes}) {
for my $element (@array) { push @return_array, qq("$element") }
}
if ($kv{brackets}) {
for my $element (@array) { push @return_array, qq([$element]) }
}
if (!$kv{no_quotes} &&!$kv{quotes} && !$kv{brackets}) { @return_array = @array }
return @return_array;
}
sub simple_merge {
my @words = @_;
my @return_array;
for my $word (@words) {
my @array = split(//, $word);
my $i = 0;
my @location;
for my $character (@array) {
if ($character =~ / /) {
push @location, $i;
}
$i++
}
for my $num (@location) {
my $temp;
for my $i (0..$#array) {
if ($num == $i) { next }
else { $temp .= $array[$i] }
}
push @return_array, $temp;
$temp = ();
}
}
return @return_array;
}
sub combine_two {
my $words_1 = shift;
my $words_2 = shift;
my @array;
for my $word_1 (split /\n/, $words_1) {
for my $word_2 (split /\n/, $words_2) {
push @array, "$word_1 $word_2"
}
}
return @array;
}
sub combine_three {
my $words_1 = shift;
my $words_2 = shift;
my $words_3 = shift;
my @array;
for my $word_1 (split /\n/, $words_1) {
for my $word_2 (split /\n/, $words_2) {
for my $word_3 (split /\n/, $words_3) {
push @array, "$word_1 $word_2 $word_3"
}
}
}
return @array;
}
sub get_input {
my $ref_array = shift;
my @form_variables = @{ $ref_array};
my %kv = ();
read (STDIN, my $input, $ENV{'CONTENT_LENGTH'});
if ($input) { $kv{got_input} = "yes" }
my @kv = split (/&/, $input);
for my $kv (@kv) {
(my $key, my $value) = split (/=/, $kv);
$value =~ tr/+/ /;
$value =~ s/%([\dA-Fa-f][\dA-Fa-f])/pack ("C", hex ($1))/eg;
$value =~ s/\r//g;
$value =~ s/^\s+//;
$value =~ s/\s+$//;
$value =~ s/\n\s+/\n/;
$value =~ s/\s+\n/\n/;
for my $variable (@form_variables) {
if ($key eq "$variable") { $kv{$key} = $value }
}
}
return %kv;
}
sub header {
my $title = shift;
my $string = qq(Content-type: text/html\n\n);
$string .= qq(<html>\n<head><title>$title</title></head>\n);
$string .= qq(<body bgcolor="#ffffff" text="#000000" link="#000000" vlink="#800000" alink="#ff00ff">\n\n);
return $string;
}
sub table {
my $cell_1 = shift;
my $cell_2 = shift;
my $string = qq(
<table align="center" border=0 cellspacing=0 cellpadding=0 width="90%">
<tr align="left" valign="middle">
<td valign="top" width="35%">
_cell_1_
</td>
</tr>
<tr>
<td valign="top" width="35%">
_cell_2_
</td>
</tr>
</table>);
$string =~ s/_cell_1_/$cell_1/;
$string =~ s/_cell_2_/$cell_2/;
return $string;
}
sub form {
my %page = @_;
my $string = qq(<form action="$page{script}" method=post>\n);
if ($page{textarea}) { $string .= textarea(%page) }
if ($page{text}) { $string .= text(%page) }
if ($page{options}) { $string .= checkbox(%page) }
$string .= qq(<br><input type=submit value="$page{submit}">\n);
return $string;
}
sub textarea {
my %page = @_;
my $string = qq(<small><small>Enter one word or phrase per line</small></small><br>\n);
for my $num (0..$#{ $page{textarea}}) {
$string .= qq(<textarea name="$page{textarea}[$num]" rows=9 cols=33>);
$string .= qq($page{$page{textarea}[$num]}</textarea> \n);
}
$string .= '<br>';
return $string;
}
sub checkbox {
my %page = @_;
my $string = qq(<small>);
for my $num (0..@{ $page{options}} - 1) {
my $checked;
for my $key (@{ $page{user_config}}) {
if ($page{options}[$num] eq $key) { $checked = 'checked' }
}
(my $label = $page{options}[$num]) =~ s/_/ /g;
$string .= qq(<input type="checkbox" );
$string .= qq(name="$page{options}[$num]" );
$string .= qq(value="$page{options}[$num]" );
$string .= qq($checked>);
$string .= qq(\u$label<br>\n);
}
$string .= qq(</small>);
return $string;
}
sub format_output {
my @array = @_;
my $string = qq(<br>\n
<table align="center" border=0 cellspacing=0 cellpadding=0 width="90%">
<tr align="left" valign=middle"><td><tt>);
$string .= join "<br>\n", @array;
$string .= qq(</td></tr></table>);
return $string;
}
------------------------------
Date: Sat, 31 Jul 2004 16:56:03 GMT
From: "Jürgen Exner" <jurgenex@hotmail.com>
Subject: Re: any pointers please? combine words script
Message-Id: <DaQOc.2883$Je5.629@nwrddc03.gnilink.net>
steve_f wrote:
> Hey, if you got a minute have fun with this script and give me some
> pointers. The combine_hash idea was probably wrong, I had it before
> to run a few scripts off of main.cgi but it got out of hand.
[about 300 lines of code snipped]
And how do you expect anyone to figure out what this script is supposed to
do versus what it is actually doing? In particular with about 300(!) lines
of code?
Post a _minimal_ piece of sample code that demonstrates the problem you are
seeing, describe your expectation (i.e. what the code is supposed to do),
describe your actual observation (i.e. what the code is actually doing),
describe briefly what you already tried to isolate/fix the problem, and
chances are much, much greater that people can actually help.
But a "please fix my program" without even telling anyone what is wrong with
the program doesn't do any good.
jue
------------------------------
Date: Sat, 31 Jul 2004 15:11:37 -0400
From: steve_f <me@example.com>
Subject: Re: any pointers please? combine words script
Message-Id: <3krng05ij56k7k7enkaje1uek156ghus3q@4ax.com>
On Sat, 31 Jul 2004 16:56:03 GMT, "Jürgen Exner" <jurgenex@hotmail.com> wrote:
>steve_f wrote:
>> Hey, if you got a minute have fun with this script and give me some
>> pointers. The combine_hash idea was probably wrong, I had it before
>> to run a few scripts off of main.cgi but it got out of hand.
>
>[about 300 lines of code snipped]
>
[ snip ]
>
>But a "please fix my program" without even telling anyone what is wrong with
>the program doesn't do any good.
>
>jue
>
>
aw sorry... it was just a simple little webmaster tool and I was
quite happy with it, but I didn't realize it was off topic to show
off and share code here ;-)
------------------------------
Date: 31 Jul 2004 21:38:18 +0100
From: Brian McCauley <nobull@mail.com>
Subject: Re: any pointers please? combine words script
Message-Id: <u9wu0j52v9.fsf@wcl-l.bham.ac.uk>
"Jürgen Exner" <jurgenex@hotmail.com> writes:
> steve_f wrote:
> > Hey, if you got a minute have fun with this script and give me some
> > pointers. The combine_hash idea was probably wrong, I had it before
> > to run a few scripts off of main.cgi but it got out of hand.
>
> [about 300 lines of code snipped]
> But a "please fix my program" without even telling anyone what is wrong with
> the program doesn't do any good.
Jürgen, this is, I think a request for a critique of the coding style
of a working program rather that a request to fix a broken one.
--
\\ ( )
. _\\__[oo
.__/ \\ /\@
. l___\\
# ll l\\
###LL LL\\
------------------------------
Date: Sat, 31 Jul 2004 21:01:42 GMT
From: "Jürgen Exner" <jurgenex@hotmail.com>
Subject: Re: any pointers please? combine words script
Message-Id: <WMTOc.3838$Je5.2188@nwrddc03.gnilink.net>
steve_f wrote:
> On Sat, 31 Jul 2004 16:56:03 GMT, "Jürgen Exner"
> <jurgenex@hotmail.com> wrote:
>
>> steve_f wrote:
>>> Hey, if you got a minute have fun with this script and give me some
>>> pointers. The combine_hash idea was probably wrong, I had it before
>>> to run a few scripts off of main.cgi but it got out of hand.
>>
>> [about 300 lines of code snipped]
>>
>
> [ snip ]
>
>>
>> But a "please fix my program" without even telling anyone what is
>> wrong with the program doesn't do any good.
>>
>> jue
>>
>>
> aw sorry... it was just a simple little webmaster tool and I was
> quite happy with it, but I didn't realize it was off topic to show
> off and share code here ;-)
Sorry, I may have misunderstood your intention.
jue
------------------------------
Date: Sat, 31 Jul 2004 19:22:12 -0400
From: steve_f <me@example.com>
Subject: Re: any pointers please? combine words script
Message-Id: <56aog0lamgli2far5vu06idtfdgigosb8p@4ax.com>
On Sat, 31 Jul 2004 21:01:42 GMT, "Jürgen Exner" <jurgenex@hotmail.com> wrote:
>steve_f wrote:
>> On Sat, 31 Jul 2004 16:56:03 GMT, "Jürgen Exner"
>> <jurgenex@hotmail.com> wrote:
>>
>>> steve_f wrote:
>>>> Hey, if you got a minute have fun with this script and give me some
>>>> pointers. The combine_hash idea was probably wrong, I had it before
>>>> to run a few scripts off of main.cgi but it got out of hand.
>>>
>>> [about 300 lines of code snipped]
>>>
>>
>> [ snip ]
>>
>>>
>>> But a "please fix my program" without even telling anyone what is
>>> wrong with the program doesn't do any good.
>>>
>>> jue
>>>
>>>
>> aw sorry... it was just a simple little webmaster tool and I was
>> quite happy with it, but I didn't realize it was off topic to show
>> off and share code here ;-)
>
>Sorry, I may have misunderstood your intention.
>
>jue
>
I think I always get over excited about scripts. Also I didn't really
even explain what the script is. It is a webmaster tool, mostly
for AdWords, but is usefully for general search engine optimization.
I guess the reason why I posted it is I always have a feeling
I am not programming simply enough and thought maybe
someone could give me a few tips. And, on the other hand
I kind of like it and have a few neat tricks going on I thought
other people would like.
But, maybe you are right that posting so much code is rude,
so sorry about that.
------------------------------
Date: Sun, 01 Aug 2004 05:17:48 GMT
From: Uri Guttman <uri@stemsystems.com>
Subject: Re: any pointers please? combine words script
Message-Id: <x77jsjcu8l.fsf@mail.sysarch.com>
>>>>> "sf" == steve f <me@example.com> writes:
sf> #!/usr/bin/perl -Tw
sf> use strict;
sf> #use CGI::Carp qw(fatalsToBrowser);
sf> my $query = $ENV{QUERY_STRING};
sf> my %config = combine_hash($query);
use CGI.pm. never parse your queries on your own unless you have a good
reason.
ever heard of comments?
sf> my %kv = get_input($config{cgi});
don't use short cryptic variable names like kv. and you have too many
file lexicals IMO (but i can tell for sure unless i do a full analysis).
sf> $config{user_input} = $kv{words};
sf> print header($config{title});
sf> for my $key (keys %kv) {
sf> for my $option (@{ $config{options}}) {
sf> if ($key eq $option) {push @{ $config{user_config}}, $key}
don't put blocks on one line like that.
sf> }
sf> }
sf> print table($config{intro},form(%config, %kv));
sf> if ($kv{got_input} eq "yes") { print format_output($config{action}(%kv)); }
sf> exit(0);
sf> sub combine_hash {
sf> my $query = shift;
sf> my ($language, $num);
sf> if ($query =~ /mkt=(\w+)/) { $language = $1 }
sf> if ($query =~ /boxes=(\w+)/) { $num = $1 }
don't parse a query like that. gack! what if someone sent you an encoded
value for mkt which had %20 like hex in it?
sf> if (!$num) { $num = 2 }
$num ||= 2 ;
sf> my $intro =
sf> "<b>Combine words</b>";
sf> my %page = (script => "combine.cgi?boxes=$num",
sf> cgi => [get_textareas($num), @options],
sf> title => 'Combine two lists',
sf> intro => $intro,
sf> textarea => [ get_textareas($num) ],
sf> options => [@options],
sf> defaults => ['no quotes', 'reverse'],
sf> submit => get_submit($num),
sf> action => sub { my %kv = @_;
sf> my @array = combine_lists(%kv);
sf> return @array;
sf> },
sf> );
why assign to %page when it is the last thing in a sub? just return the
list.
sf> }
sf> sub get_submit {
sf> my $num = shift;
sf> if ($num == 1) { return 'combine one' }
sf> if ($num == 2) { return 'combine two' }
sf> if ($num == 3) { return 'combine three' }
return( 'combine' . qw( one two three )[ shift - 1 ] ) ;
ugly but i hate repeated code like that sub has.
sf> }
sf> sub menu {
sf> my $source = shift;
sf> my %hash = @_;
my ($source, %hash) = @_;
assign from @_ is more common and better IMO than shift
sf> my @temp;
never name a temp, temp. name it for what is it a temp FOR. and i can't
recall the last time i needed a temp var. it is a flag that the code
isn't designed well.
sf> for my $element (sort keys %hash) {
sf> if ($element eq $source) { push(@temp, "<b>$source</b>") }
sf> else { push(@temp, "<a href=\"./$hash{$element}\">$element</a>") }
sf> }
push( @better_name, $element eq $source ?
"<b>$source</b>" :
qq{<a href="./$hash{$element}">$element</a>} ;
also use qq{} when you have quotes in your text
sf> my $output .= join(" | \n", @temp);
sf> return $output;
let's combine all of that:
return join " | \n", map {
$_ eq $source ?
"<b>$source</b>" :
qq{<a href="./$hash{$_}">$element</a>}
} sort keys %hash ;
not too cluttered, i think. :)
sf> }
sf> sub get_options {
sf> my $num = shift;
sf> if ($num == 1) { return 'no_quotes', 'quotes', 'brackets', 'skip_space'}
sf> elsif ($num == 2) { return 'no_quotes', 'quotes', 'brackets',
sf> 'reverse_words', 'skip_space'}
sf> elsif ($num == 3) { return 'no_quotes', 'quotes', 'brackets',
sf> 'reverse_words', 'skip_space'}
sf> else { return 'no_quotes', 'no_quotes', 'quotes', 'brackets', 'skip_space'}
sf> }
GACK!!!!!
whay do you do a elsif if you RETURN in the previous block? you don't in
the combine one/two/three sub earlier. do the same thing here. in fact
do the same thing i did above. factor out the common code and just index
by $num into a qw.
sf> sub get_textareas {
sf> my $num = shift;
sf> if ($num == 1) { return 'kw_1'}
sf> elsif ($num == 2) { return 'kw_1', 'kw_2'}
sf> elsif ($num == 3) { return 'kw_1', 'kw_2', 'kw_3'}
sf> else { return 'kw_1'}
sf> }
same as above.
sf> sub combine_lists {
sf> my %kv = @_;
better names please!!
sf> my @array;
sf> if ($kv{kw_1} && !$kv{kw_2} && !$kv{kw_3}) {
sf> @array = split /\n/, $kv{kw_1};
sf> if ($kv{skip_space}) { push @array, simple_merge(@array) }
sf> }
sf> if ($kv{kw_1} && $kv{kw_2} && !$kv{kw_3}) {
sf> @array = combine_two($kv{kw_1}, $kv{kw_2});
sf> if ($kv{reverse_words}) {
sf> push @array, combine_two($kv{kw_2}, $kv{kw_1});
sf> }
sf> if ($kv{skip_space}) { push @array, simple_merge(@array) }
sf> }
sf> if ($kv{kw_1} && $kv{kw_2} && $kv{kw_3}) {
sf> @array = combine_three($kv{kw_1}, $kv{kw_2}, $kv{kw_3});
sf> if ($kv{reverse_words}) {
sf> push @array, combine_three($kv{kw_3}, $kv{kw_2}, $kv{kw_1});
sf> }
sf> if ($kv{skip_space}) { push @array, simple_merge(@array) }
sf> }
sf> my @return_array;
sf> if ($kv{no_quotes}) { @return_array = @array }
sf> if ($kv{quotes}) {
sf> for my $element (@array) { push @return_array, qq("$element") }
sf> }
sf> if ($kv{brackets}) {
sf> for my $element (@array) { push @return_array, qq([$element]) }
sf> }
sf> if (!$kv{no_quotes} &&!$kv{quotes} && !$kv{brackets}) { @return_array = @array }
sf> return @return_array;
sf> }
that sub is totally incomprehensible. it looks like it had redundant and
repeated code and calls those odd little subs you have written. no idea
what it wants to do.
sf> sub simple_merge {
sf> my @words = @_;
sf> my @return_array;
sf> for my $word (@words) {
sf> my @array = split(//, $word);
sf> my $i = 0;
sf> my @location;
sf> for my $character (@array) {
you rarely need to loop over characters in perl. again, i have no clue
as to what you are trying to do so i can't fix it. the sub's name is
meaningless as well.
sf> if ($character =~ / /) {
WHAT!! you have a single char and you use a slow regex to see if it is a
blank? eq is the correct operator.
sf> push @location, $i;
ever heard of index? it does this location of character searching for you.
sf> }
sf> $i++
sf> }
sf> for my $num (@location) {
sf> my $temp;
again with temp. bad boy! no cookie for you!
sf> for my $i (0..$#array) {
sf> if ($num == $i) { next }
sf> else { $temp .= $array[$i] }
sf> }
sf> push @return_array, $temp;
sf> $temp = ();
sf> }
sf> }
sf> return @return_array;
sf> }
sf> sub combine_two {
sf> my $words_1 = shift;
sf> my $words_2 = shift;
sf> my @array;
sf> for my $word_1 (split /\n/, $words_1) {
sf> for my $word_2 (split /\n/, $words_2) {
sf> push @array, "$word_1 $word_2"
sf> }
sf> }
sf> return @array;
sf> }
you have a fetish for combine. it must mean something to you but not to
anyone else.
sf> sub combine_three {
sf> my $words_1 = shift;
sf> my $words_2 = shift;
sf> my $words_3 = shift;
sf> my @array;
sf> for my $word_1 (split /\n/, $words_1) {
sf> for my $word_2 (split /\n/, $words_2) {
sf> for my $word_3 (split /\n/, $words_3) {
sf> push @array, "$word_1 $word_2 $word_3"
sf> }
sf> }
sf> }
sf> return @array;
sf> }
sf> sub get_input {
sf> my $ref_array = shift;
sf> my @form_variables = @{ $ref_array};
sf> my %kv = ();
sf> read (STDIN, my $input, $ENV{'CONTENT_LENGTH'});
sf> if ($input) { $kv{got_input} = "yes" }
sf> my @kv = split (/&/, $input);
sf> for my $kv (@kv) {
sf> (my $key, my $value) = split (/=/, $kv);
sf> $value =~ tr/+/ /;
sf> $value =~ s/%([\dA-Fa-f][\dA-Fa-f])/pack ("C", hex ($1))/eg;
sf> $value =~ s/\r//g;
sf> $value =~ s/^\s+//;
sf> $value =~ s/\s+$//;
sf> $value =~ s/\n\s+/\n/;
sf> $value =~ s/\s+\n/\n/;
sf> for my $variable (@form_variables) {
sf> if ($key eq "$variable") { $kv{$key} = $value }
sf> }
sf> }
sf> return %kv;
sf> }
CGI.pm or you die! BLECHHH!!!!!!
sf> sub header {
sf> my $title = shift;
sf> my $string = qq(Content-type: text/html\n\n);
sf> $string .= qq(<html>\n<head><title>$title</title></head>\n);
sf> $string .= qq(<body bgcolor="#ffffff" text="#000000" link="#000000" vlink="#800000" alink="#ff00ff">\n\n);
sf> return $string;
sf> }
cgi.pm again.
enough for me. i should bill you for this posting!
uri
--
Uri Guttman ------ uri@stemsystems.com -------- http://www.stemsystems.com
--Perl Consulting, Stem Development, Systems Architecture, Design and Coding-
Search or Offer Perl Jobs ---------------------------- http://jobs.perl.org
------------------------------
Date: 01 Aug 2004 12:22:24 +0100
From: Brian McCauley <nobull@mail.com>
Subject: Re: any pointers please? combine words script
Message-Id: <u94qnnnlvz.fsf@wcl-l.bham.ac.uk>
Uri has given an excellent critique of the OP's code. I am in total
agreement with most of what Uri says (and where I've not commented
this should be assumed to be the case). In some places I'll present
alternatives the TIMTOWDI standpoint. In a few places my opinion
actually differs from Uri's.
Uri Guttman <uri@stemsystems.com> writes:
> >>>>> "sf" == steve f <me@example.com> writes:
>
> sf> my %kv = get_input($config{cgi});
>
> don't use short cryptic variable names like kv.
Certainly not for widely scoped lexicals. The length of a variable
name should be in proportion to its scope. There's nothing wrong
with do { \my $o = 1 } !
Another thing to point out is, think twice before passing or returning
a hash to/from a function as a list. Sometimes it makes sense but it
should not be the norm.
> sf> if ($key eq $option) {push @{ $config{user_config}}, $key}
>
> don't put blocks on one line like that.
I don't see any problem with single statement blocks on one line.
After all the bit in side the @{...} is actually a block too!
> sf> if ($kv{got_input} eq "yes") { print format_output($config{action}(%kv)); }
Always use the natural representation for things. If somthing is
naturally a boolean (as I suspect $kv{got_input} is) use the
language's natural representation of boolean.
if ($kv{got_input}) { print format_output($config{action}(%kv)); }
>
> sf> exit(0);
>
> sf> sub combine_hash {
>
> sf> my $query = shift;
>
> sf> my ($language, $num);
>
> sf> if ($query =~ /mkt=(\w+)/) { $language = $1 }
> sf> if ($query =~ /boxes=(\w+)/) { $num = $1 }
>
> don't parse a query like that. gack! what if someone sent you an encoded
> value for mkt which had %20 like hex in it?
Yes, don't parse QUERY_STRING yourself.
However ignoring the fact that you shouldn't be doing this in this way
at all, I'd like to point out that there's a more elegant way to
extract bits from a pattern match into variables.
I dislike any constuct that involves $1 et al and I only get involved
with them when I have to (ie when iterating using a regex with a /g).
my ($language) = $query =~ /mkt=(\w+)/;
my ($num) = $query =~ /boxes=(\w+)/;
> sf> my %page = (script => "combine.cgi?boxes=$num",
> sf> cgi => [get_textareas($num), @options],
> sf> title => 'Combine two lists',
> sf> intro => $intro,
> sf> textarea => [ get_textareas($num) ],
> sf> options => [@options],
> sf> defaults => ['no quotes', 'reverse'],
> sf> submit => get_submit($num),
> sf> action => sub { my %kv = @_;
> sf> my @array = combine_lists(%kv);
> sf> return @array;
> sf> },
> sf> );
>
> why assign to %page when it is the last thing in a sub? just return the
> list.
Or maybe a hashref.
>
> sf> sub get_submit {
> sf> my $num = shift;
> sf> if ($num == 1) { return 'combine one' }
> sf> if ($num == 2) { return 'combine two' }
> sf> if ($num == 3) { return 'combine three' }
>
> return( 'combine' . qw( one two three )[ shift - 1 ] ) ;
Hey, Uri, you should _always_ enable warnings when developing Perl
code. :-)
> ugly but i hate repeated code like that sub has.
Me too. But I'd probably define a variable, maybe even a hash. Yeah,
I know that a list with the subscripts offset by one may be the most
efficient representation but I would argue its not the most natural.
{
my @submit = ( undef, 'combine one', 'combine two', 'combine three' );
sub get_submit {
$submit[shift];
}
}
You should only factor out common code when the fact that the code is
common is in the nature of the problem. When the fact that there is
common code is not part of the natural way of thinking about the
problem then you shouldn't do it. Without understanding more about
the OP's problem I can't say for sure by I'd guess it as inappropriate
to factor out the constant substring. Or rather if it were
appropriate to factor it out it should probably be factored out
elswhere in the code and not here.
> sf> sub menu {
> sf> my $source = shift;
> sf> my %hash = @_;
>
> my ($source, %hash) = @_;
>
> assign from @_ is more common and better IMO than shift
I'm affraid I have to say I'm inclined to leave this upto personal
taste. My personal taste most of the time is to use list assignment
except when pulling the class/object argument off the front of a
method's @_. But that's only most of the time.
> sf> my @temp;
>
> never name a temp, temp. name it for what is it a temp FOR. and i can't
> recall the last time i needed a temp var. it is a flag that the code
> isn't designed well.
Again this comes back to the idea that the length of a variable name
should be proportional the the scope. If a varialble's entire scope
will fit in ones mental parsing buffer then it's OK for it to have a 1
character name. I would avoid names like temp because they add
neither calrity nor brevity.
>
> sf> for my $element (sort keys %hash) {
> sf> if ($element eq $source) { push(@temp, "<b>$source</b>") }
> sf> else { push(@temp, "<a href=\"./$hash{$element}\">$element</a>") }
> sf> }
>
> push( @better_name, $element eq $source ?
> "<b>$source</b>" :
> qq{<a href="./$hash{$element}">$element</a>} ;
I disagree just because you can replace if-else with ?: this doesn't
mean you should.
> also use qq{} when you have quotes in your text
I've never liked qq{}. I think it adds complexity. I'm used to the
idea that within an interpolative context '$' '@' and '"' need to be
quoted. I know I can choose an interpolative context where instead '$'
'@' and '}' need quoting but I don't think this aids readability.
> sf> my $output .= join(" | \n", @temp);
> sf> return $output;
>
> let's combine all of that:
>
> return join " | \n", map {
> $_ eq $source ?
> "<b>$source</b>" :
> qq{<a href="./$hash{$_}">$element</a>}
> } sort keys %hash ;
>
> not too cluttered, i think. :)
I think differently, I actually prefer the OPs code (although I
wouldn't use @temp as a variable name and I'd make some semantically
neutral changes to the punctuional and whitespace.
Anyhow, I find the tertiary operator split over multiple lines looks
messy and hard to follow. The ':' is just too small for my
subconcious pre-parser to lock onto it. I like a cuddled else.
return join " | \n", map {
if ($_ eq $source) {
"<b>$source</b>";
} else {
qq{<a href="./$hash{$_}">$element</a>};
}
} sort keys %hash ;
It amazes me that the same people who propound the evils of permitting
nested statement qualifiers thus for forbidding innocent little
constucts like...
print unless /^#/ for @text;
...will happily combine lines of clear and readable code by using ?:
and map{} to produce something less readable.
> sf> }
>
> sf> sub get_options {
> sf> my $num = shift;
> sf> if ($num == 1) { return 'no_quotes', 'quotes', 'brackets', 'skip_space'}
> sf> elsif ($num == 2) { return 'no_quotes', 'quotes', 'brackets',
> sf> 'reverse_words', 'skip_space'}
> sf> elsif ($num == 3) { return 'no_quotes', 'quotes', 'brackets',
> sf> 'reverse_words', 'skip_space'}
> sf> else { return 'no_quotes', 'no_quotes', 'quotes', 'brackets', 'skip_space'}
> sf> }
>
> GACK!!!!!
>
> whay do you do a elsif if you RETURN in the previous block? you don't in
> the combine one/two/three sub earlier. do the same thing here. in fact
> do the same thing i did above. factor out the common code and just index
> by $num into a qw.
Once again I would look to an array.
> sf> sub get_textareas {
> sf> my $num = shift;
> sf> if ($num == 1) { return 'kw_1'}
> sf> elsif ($num == 2) { return 'kw_1', 'kw_2'}
> sf> elsif ($num == 3) { return 'kw_1', 'kw_2', 'kw_3'}
> sf> else { return 'kw_1'}
> sf> }
>
> same as above.
Ah, but this is the third time. One of my favourate maxims in
programming is: "If you are doing it three times you
are (probably) doing it wrong".
Now it the time to consider a radical refactoring.
Leave it until you get to the seventh time and it's to much effort.
Do it the second time and you'll be refactoring too often. I find
three is a good number as a rule of thumb.
Perhaps you should ditch get_options(), get_textareas() and
get_submit() and replace them with a simple file-scoped HoH.
my %modes = (
1 => {
textareas => [ 'kw_1'],
options => [ 'no_quotes', 'quotes', 'brackets', 'skip_space' ],
submit => 'combine one',
},
2 => {
textareas => [ 'kw_1', 'kw_2' ],
options => [ 'no_quotes', 'quotes', 'brackets', 'reverse_words',
'skip_space' ],
submit => 'combine two',
},
# and so on
);
> sf> for my $num (@location) {
> sf> my $temp;
> sf> for my $i (0..$#array) {
> sf> if ($num == $i) { next }
> sf> else { $temp .= $array[$i] }
> sf> }
> sf> push @return_array, $temp;
> sf> $temp = ();
Seting a scalar to an empty list? WTF? Better to say undef. But
since $temp is a string accumulator it should really be initialised to
an empty string not to undef anyhow.
> sf> }
Anyhow, any time you explicitly clear an accumulator variable ask
yourself - would I need to do this if I'd declared the variable in the
right scope?
In this case, amuzingly, not only is the answer 'no' but the OP _did_
actually declare the accumulator in the correct scope so this
statement is simply redundant.
Anyhow, an array slice would make more sense.
join '' => @array[ 0 .. $num-1, $num+1 .. $#array ]
Or maybe
join '' => @array[ grep { $_ != $num } 0 .. $#array ]
But then looking further afield this looks like the OP is really doing
a splice() or maybe a serch and replace - I'll address simple_merge()
in a separate post.
> sf> sub combine_two {
> sf> my $words_1 = shift;
> sf> my $words_2 = shift;
> sf> my @array;
> sf> for my $word_1 (split /\n/, $words_1) {
> sf> for my $word_2 (split /\n/, $words_2) {
> sf> push @array, "$word_1 $word_2"
> sf> }
> sf> }
> sf> return @array;
> sf> }
>
> sf> sub combine_three {
> sf> my $words_1 = shift;
> sf> my $words_2 = shift;
> sf> my $words_3 = shift;
> sf> my @array;
> sf> for my $word_1 (split /\n/, $words_1) {
> sf> for my $word_2 (split /\n/, $words_2) {
> sf> for my $word_3 (split /\n/, $words_3) {
> sf> push @array, "$word_1 $word_2 $word_3"
> sf> }
> sf> }
> sf> }
> sf> return @array;
> sf> }
Remember the rule of three? Well you've just hit it again. Actually
maybe it's more like five. You should refactor combine_two and
combine_one into a single recursive function.
sub combine {
my @words = split /\n/, shift;
return @words unless @_;
my @combinations;
for my $word (@words) {
push @combinations => map { "$word $_" } combine(@_);
}
@combinations;
}
Of course you could eliminate @combinations by using map{} but I
don't think it aids readability to do so.
--
\\ ( )
. _\\__[oo
.__/ \\ /\@
. l___\\
# ll l\\
###LL LL\\
------------------------------
Date: 01 Aug 2004 12:44:19 +0100
From: Brian McCauley <nobull@mail.com>
Subject: Re: any pointers please? combine words script
Message-Id: <u9zn5fm6b0.fsf@wcl-l.bham.ac.uk>
steve_f <me@example.com> writes:
> Hey, if you got a minute have fun with this script and give me some pointers.
> The combine_hash idea was probably wrong, I had it before to run a few
> scripts off of main.cgi but it got out of hand.
This is way too long to address all in one place. I'll just look at
one isolated part in this branch of the thread.
> sub simple_merge {
> my @words = @_;
> my @return_array;
> for my $word (@words) {
> my @array = split(//, $word);
> my $i = 0;
> my @location;
> for my $character (@array) {
> if ($character =~ / /) {
> push @location, $i;
> }
> $i++
> }
> for my $num (@location) {
> my $temp;
> for my $i (0..$#array) {
> if ($num == $i) { next }
> else { $temp .= $array[$i] }
> }
> push @return_array, $temp;
> $temp = ();
> }
> }
> return @return_array;
> }
You are rolling your own string manipultions. Perl has very good
string manipulations.
You are using the variable name $word for something that will contain
spaces. In most languages words do not contain spaces.
If you want to do some thing for all the spaces in a string then
simply use while (/ /g).
sub simple_merge {
my @merged;
for (@_) {
while (/ /g) {
push @merged => substr($_,0,pos()-1).substr($_,pos);
}
}
@merged;
}
Actually this is one of the few places I'd like to use "$`$'" but
there's a nasty overhead using $` and $' so I won't.
--
\\ ( )
. _\\__[oo
.__/ \\ /\@
. l___\\
# ll l\\
###LL LL\\
------------------------------
Date: Sun, 01 Aug 2004 15:06:29 -0400
From: steve_f <me@example.com>
Subject: Re: any pointers please? combine words script
Message-Id: <kofqg0pt2hq9uuusq0kmibu2542u16hsrp@4ax.com>
[ snip - one huge reply ]
wow...............!! thanks uri...it will take me maybe a week or
two to go through this, so I just want to say thanks and
I will use CGI.pm...sorry about that ;-)
------------------------------
Date: Mon, 02 Aug 2004 00:29:31 GMT
From: Uri Guttman <uri@stemsystems.com>
Subject: Re: any pointers please? combine words script
Message-Id: <x7isc2mlgn.fsf@mail.sysarch.com>
>>>>> "BM" == Brian McCauley <nobull@mail.com> writes:
BM> Uri Guttman <uri@stemsystems.com> writes:
>> >>>>> "sf" == steve f <me@example.com> writes:
>>
sf> my %kv = get_input($config{cgi});
>>
>> don't use short cryptic variable names like kv.
BM> Certainly not for widely scoped lexicals. The length of a variable
BM> name should be in proportion to its scope. There's nothing wrong
BM> with do { \my $o = 1 } !
i still wouldn't use $o there but i agree with the proportion idea. one
major negative with single char names is that they are hard/impossible
to search/replace.
sf> if ($key eq $option) {push @{ $config{user_config}}, $key}
>>
>> don't put blocks on one line like that.
BM> I don't see any problem with single statement blocks on one line.
BM> After all the bit in side the @{...} is actually a block too!
true but not the same as an if/while block. a deref block is usually
part of an expression so spreading it out over multiple lines is
silly. but flow control blocks should (most) always be formatted in
normal ways.
BM> I dislike any constuct that involves $1 et al and I only get
BM> involved with them when I have to (ie when iterating using a regex
BM> with a /g).
why do you need $1 even there? assign the grabbed stuff to a list of
vars or an array and it will still work fine in a while /g loop.
>> return( 'combine' . qw( one two three )[ shift - 1 ] ) ;
BM> Hey, Uri, you should _always_ enable warnings when developing Perl
BM> code. :-)
UNTESTED! :)
>> ugly but i hate repeated code like that sub has.
BM> Me too. But I'd probably define a variable, maybe even a hash. Yeah,
BM> I know that a list with the subscripts offset by one may be the most
BM> efficient representation but I would argue its not the most natural.
sure. i was just so annoyed with the OP's code. :) i might use a hash
instead of an array so i wouldn't have the -1 problem. also i would
declare/assign it externally to the sub. but i bet there are better
solutions that remove the whole need for this and many of the other
subs. as i said, i won't analyze this enough to figure out how to clean
it up but my gut says it can be done in much less code. i smell
redundant code all over this mess.
sf> sub menu {
sf> my $source = shift;
sf> my %hash = @_;
>>
>> my ($source, %hash) = @_;
>>
>> assign from @_ is more common and better IMO than shift
BM> I'm affraid I have to say I'm inclined to leave this upto personal
BM> taste. My personal taste most of the time is to use list assignment
BM> except when pulling the class/object argument off the front of a
BM> method's @_. But that's only most of the time.
i agree with most of the time. damian showed a wacko new module of his
at the sufficiently advanced technologies talk that assumes assignment
from @_ (to allow for use of $_[0] put there by a source filter!).
sf> my @temp;
>>
>> never name a temp, temp. name it for what is it a temp FOR. and i can't
>> recall the last time i needed a temp var. it is a flag that the code
>> isn't designed well.
BM> Again this comes back to the idea that the length of a variable name
BM> should be proportional the the scope. If a varialble's entire scope
BM> will fit in ones mental parsing buffer then it's OK for it to have a 1
BM> character name. I would avoid names like temp because they add
BM> neither calrity nor brevity.
that was my main point. naming somthing $temp is like the 'add 1'
comment for $i++.
>>
sf> for my $element (sort keys %hash) {
sf> if ($element eq $source) { push(@temp, "<b>$source</b>") }
sf> else { push(@temp, "<a href=\"./$hash{$element}\">$element</a>") }
sf> }
>>
>> push( @better_name, $element eq $source ?
>> "<b>$source</b>" :
>> qq{<a href="./$hash{$element}">$element</a>} ;
BM> I disagree just because you can replace if-else with ?: this doesn't
BM> mean you should.
i like it :)
>> also use qq{} when you have quotes in your text
BM> I've never liked qq{}. I think it adds complexity. I'm used to the
BM> idea that within an interpolative context '$' '@' and '"' need to be
BM> quoted. I know I can choose an interpolative context where instead '$'
BM> '@' and '}' need quoting but I don't think this aids readability.
i just hate seeing \ in strings more than it needs to be there. qq is
there for a reason so use it. i know someone who hates here docs but i
love them. different (key)strokes! :)
sf> my $output .= join(" | \n", @temp);
sf> return $output;
>>
>> let's combine all of that:
>>
>> return join " | \n", map {
>> $_ eq $source ?
>> "<b>$source</b>" :
>> qq{<a href="./$hash{$_}">$element</a>}
>> } sort keys %hash ;
>>
>> not too cluttered, i think. :)
BM> I think differently, I actually prefer the OPs code (although I
BM> wouldn't use @temp as a variable name and I'd make some semantically
BM> neutral changes to the punctuional and whitespace.
BM> Anyhow, I find the tertiary operator split over multiple lines looks
BM> messy and hard to follow. The ':' is just too small for my
BM> subconcious pre-parser to lock onto it. I like a cuddled else.
i was having fun there. again, my code isn't real as i would need to do
a full redesign. i have generated html crap with much cleaner code than
the OP so i know the kind of code needed.
BM> return join " | \n", map {
BM> if ($_ eq $source) {
BM> "<b>$source</b>";
BM> } else {
BM> qq{<a href="./$hash{$_}">$element</a>};
BM> }
BM> } sort keys %hash ;
using the return value of if/else is evil IMO. it may work, it may be
documented but it is not the intent of it. ?: is designed for this very
thing.
BM> Now it the time to consider a radical refactoring.
which i said i won't do. :)
BM> Perhaps you should ditch get_options(), get_textareas() and
BM> get_submit() and replace them with a simple file-scoped HoH.
BM> my %modes = (
BM> 1 => {
BM> textareas => [ 'kw_1'],
BM> options => [ 'no_quotes', 'quotes', 'brackets', 'skip_space' ],
BM> submit => 'combine one',
BM> },
BM> 2 => {
BM> textareas => [ 'kw_1', 'kw_2' ],
BM> options => [ 'no_quotes', 'quotes', 'brackets', 'reverse_words',
BM> 'skip_space' ],
BM> submit => 'combine two',
BM> },
BM> # and so on
BM> );
looks better already. tables are good!!
uri
--
Uri Guttman ------ uri@stemsystems.com -------- http://www.stemsystems.com
--Perl Consulting, Stem Development, Systems Architecture, Design and Coding-
Search or Offer Perl Jobs ---------------------------- http://jobs.perl.org
------------------------------
Date: 2 Aug 2004 11:16:35 GMT
From: anno4000@lublin.zrz.tu-berlin.de (Anno Siegel)
Subject: Re: any pointers please? combine words script
Message-Id: <cel7qj$6v2$1@mamenchi.zrz.TU-Berlin.DE>
steve_f <me@example.com> wrote in comp.lang.perl.misc:
> Hey, if you got a minute have fun with this script and give me some pointers.
s/minute/few hours/
300 lines of code, and not a word of explanation, except that it is
supposed to "combine words"? Not my idea of fun.
Anno
------------------------------
Date: 2 Aug 2004 12:06:44 GMT
From: anno4000@lublin.zrz.tu-berlin.de (Anno Siegel)
Subject: Re: any pointers please? combine words script
Message-Id: <celaok$9ak$1@mamenchi.zrz.TU-Berlin.DE>
Uri Guttman <uri@stemsystems.com> wrote in comp.lang.perl.misc:
> >>>>> "BM" == Brian McCauley <nobull@mail.com> writes:
> BM> Uri Guttman <uri@stemsystems.com> writes:
[good stuff snipped to add two comments]
> >> don't use short cryptic variable names like kv.
>
> BM> Certainly not for widely scoped lexicals. The length of a variable
> BM> name should be in proportion to its scope. There's nothing wrong
> BM> with do { \my $o = 1 } !
>
> i still wouldn't use $o there but i agree with the proportion idea. one
> major negative with single char names is that they are hard/impossible
> to search/replace.
That's no big deal when the scope is small. Also, there's rarely a need
to rename a variable whose name means almost nothing.
> BM> I dislike any constuct that involves $1 et al and I only get
> BM> involved with them when I have to (ie when iterating using a regex
> BM> with a /g).
>
> why do you need $1 even there? assign the grabbed stuff to a list of
> vars or an array and it will still work fine in a while /g loop.
Well, that's the crux. You can't have //g in scalar context and collect
submatches in a list. That's why $1 etc. tend to crop up in the body of
a while ( /.../g ).
Anno
------------------------------
Date: Mon, 02 Aug 2004 11:57:48 -0400
From: steve_f <me@example.com>
Subject: Re: any pointers please? combine words script
Message-Id: <amosg09jk32pdi7ls3usqdu83egno15373@4ax.com>
On 2 Aug 2004 11:16:35 GMT, anno4000@lublin.zrz.tu-berlin.de (Anno Siegel) wrote:
>steve_f <me@example.com> wrote in comp.lang.perl.misc:
>> Hey, if you got a minute have fun with this script and give me some pointers.
>
>s/minute/few hours/
>
>300 lines of code, and not a word of explanation, except that it is
>supposed to "combine words"? Not my idea of fun.
>
>Anno
s/sloppy_old_code/great_new_ideas/g
works very well for me!
------------------------------
Date: Mon, 02 Aug 2004 12:01:00 -0400
From: steve_f <me@example.com>
Subject: Re: any pointers please? combine words script
Message-Id: <grosg0pm67adu2sf2hfp7530kmkhtk9klc@4ax.com>
Thanks Brian! You and Uri have really given me some beautiful
ideas...you can do so much with simple statements in Perl...just
print, open, some loops, etc. But I know there are better ways
of expressing things that I need to learn...I'm probably pretty
lazy about it or just don't know.
Then again, there is also the issue of complexity...if you try
hiding complexity somewhere it pops out at you somewhere
else. So I tried making everything into subs, but then I got
a million subs...etc. It must be an art to know when and how
to generalize.
All this great info is going to take a while to sink in...so I
want to say thanks and not launch into asking more questions
before I really go through everything...it is great stuff and
a lot to think about!
------------------------------
Date: 2 Aug 2004 09:55:36 -0700
From: nobull@mail.com
Subject: Re: any pointers please? combine words script
Message-Id: <4dafc536.0408020855.6dcb5a28@posting.google.com>
Brian McCauley <nobull@mail.com> wrote in message news:<u94qnnnlvz.fsf@wcl-l.bham.ac.uk>...
> There's nothing wrong with do { \my $o = 1 } !
Except, of course "Can't modify single ref constructor in scalar assignment".
Should have said
\do{ my $o = 1 }
------------------------------
Date: Tue, 03 Aug 2004 04:26:52 GMT
From: Uri Guttman <uri@stemsystems.com>
Subject: Re: any pointers please? combine words script
Message-Id: <x7wu0gc0ef.fsf@mail.sysarch.com>
>>>>> "sf" == steve f <me@example.com> writes:
sf> On 2 Aug 2004 11:16:35 GMT, anno4000@lublin.zrz.tu-berlin.de (Anno
sf> Siegel) wrote:
>> 300 lines of code, and not a word of explanation, except that it is
>> supposed to "combine words"? Not my idea of fun.
>>
>> Anno
sf> s/sloppy_old_code/great_new_ideas/g
sf> works very well for me!
but not for anyone else. that is called selfish coding. here are a
couple of coding rules for you to learn:
code is for people, not computers
code is for OTHER people, not yourself
if you would ever want to be hired by a clueful employer, learn how to
analyze a problem, design and code it cleanly and to use comments. your
code earned an F from me (and you thanked me for my feedback).
uri
--
Uri Guttman ------ uri@stemsystems.com -------- http://www.stemsystems.com
--Perl Consulting, Stem Development, Systems Architecture, Design and Coding-
Search or Offer Perl Jobs ---------------------------- http://jobs.perl.org
------------------------------
Date: Tue, 03 Aug 2004 10:04:58 -0400
From: steve_f <me@example.com>
Subject: Re: any pointers please? combine words script
Message-Id: <1j6vg01irlrcf4mmvsnrvucadhgpbbq2pd@4ax.com>
On Tue, 03 Aug 2004 04:26:52 GMT, Uri Guttman <uri@stemsystems.com> wrote:
>>>>>> "sf" == steve f <me@example.com> writes:
>
> sf> On 2 Aug 2004 11:16:35 GMT, anno4000@lublin.zrz.tu-berlin.de (Anno
> sf> Siegel) wrote:
>
> >> 300 lines of code, and not a word of explanation, except that it is
> >> supposed to "combine words"? Not my idea of fun.
> >>
> >> Anno
>
> sf> s/sloppy_old_code/great_new_ideas/g
>
> sf> works very well for me!
>
>but not for anyone else. that is called selfish coding. here are a
>couple of coding rules for you to learn:
>
> code is for people, not computers
> code is for OTHER people, not yourself
>
>if you would ever want to be hired by a clueful employer, learn how to
>analyze a problem, design and code it cleanly and to use comments. your
>code earned an F from me (and you thanked me for my feedback).
>
>uri
oh yeah, I think you were very helpful, so that's great. But I put some
thought into it and I think the script works beautifully, so that is why
I like it...it does exactly what I want. But the code is ugly...I would
add similar functions without generalizing, etc. So that is why you
give it an F.
------------------------------
Date: Tue, 03 Aug 2004 10:30:39 -0400
From: steve_f <me@example.com>
Subject: Re: any pointers please? combine words script
Message-Id: <gk7vg019alimikg45l0m04jnfbto1vaogs@4ax.com>
On Sun, 01 Aug 2004 05:17:48 GMT, Uri Guttman <uri@stemsystems.com> wrote:
[ snip - don't parse environmental variables yourself ]
>ever heard of comments?
ok...I was being selfish...I should comment more.
>
> sf> my %kv = get_input($config{cgi});
>
>don't use short cryptic variable names like kv. and you have too many
>file lexicals IMO (but i can tell for sure unless i do a full analysis).
good point...using %kv and %config drives me up the wall...I forget
what is what.
file lexicals = global variables?
>
> sf> $config{user_input} = $kv{words};
> sf> print header($config{title});
> sf> for my $key (keys %kv) {
> sf> for my $option (@{ $config{options}}) {
> sf> if ($key eq $option) {push @{ $config{user_config}}, $key}
>
>don't put blocks on one line like that.
this I do not think is so critical
>
> sf> }
> sf> }
> sf> print table($config{intro},form(%config, %kv));
> sf> if ($kv{got_input} eq "yes") { print format_output($config{action}(%kv)); }
>
> sf> exit(0);
>
> sf> sub combine_hash {
>
> sf> my $query = shift;
>
> sf> my ($language, $num);
>
> sf> if ($query =~ /mkt=(\w+)/) { $language = $1 }
> sf> if ($query =~ /boxes=(\w+)/) { $num = $1 }
>
>don't parse a query like that. gack! what if someone sent you an encoded
>value for mkt which had %20 like hex in it?
in the code, the query can only be boxes=1, boxes=2 and boxes=3,
so I didn't think it mattered...what is a better way though?
>
> sf> if (!$num) { $num = 2 }
>
>$num ||= 2 ;
cute, but I really don't think it reads well...it means if $num doesn't
exist then $num = 2?
>
>
> sf> my $intro =
> sf> "<b>Combine words</b>";
>
>
> sf> my %page = (script => "combine.cgi?boxes=$num",
> sf> cgi => [get_textareas($num), @options],
> sf> title => 'Combine two lists',
> sf> intro => $intro,
> sf> textarea => [ get_textareas($num) ],
> sf> options => [@options],
> sf> defaults => ['no quotes', 'reverse'],
> sf> submit => get_submit($num),
> sf> action => sub { my %kv = @_;
> sf> my @array = combine_lists(%kv);
> sf> return @array;
> sf> },
> sf> );
>
>why assign to %page when it is the last thing in a sub? just return the
>list.
good idea!....this whole concept of returning what you want I like a
lot...works great ;-)
>
> sf> }
>
> sf> sub get_submit {
> sf> my $num = shift;
> sf> if ($num == 1) { return 'combine one' }
> sf> if ($num == 2) { return 'combine two' }
> sf> if ($num == 3) { return 'combine three' }
>
>return( 'combine' . qw( one two three )[ shift - 1 ] ) ;
is this construct: qw( one two three )[ shift - 1 ] an
anonymous array? wow...I have never seen this
before, but it looks great!
>
>ugly but i hate repeated code like that sub has.
>
> sf> }
>
> sf> sub menu {
> sf> my $source = shift;
> sf> my %hash = @_;
>
>my ($source, %hash) = @_;
>
>assign from @_ is more common and better IMO than shift
ok...this is cute too, I like it.
>
> sf> my @temp;
>
>never name a temp, temp. name it for what is it a temp FOR. and i can't
>recall the last time i needed a temp var. it is a flag that the code
>isn't designed well.
>
> sf> for my $element (sort keys %hash) {
> sf> if ($element eq $source) { push(@temp, "<b>$source</b>") }
> sf> else { push(@temp, "<a href=\"./$hash{$element}\">$element</a>") }
> sf> }
>
> push( @better_name, $element eq $source ?
> "<b>$source</b>" :
> qq{<a href="./$hash{$element}">$element</a>} ;
>
>also use qq{} when you have quotes in your text
>
> sf> my $output .= join(" | \n", @temp);
> sf> return $output;
>
>let's combine all of that:
>
> return join " | \n", map {
> $_ eq $source ?
> "<b>$source</b>" :
> qq{<a href="./$hash{$_}">$element</a>}
> } sort keys %hash ;
>
>not too cluttered, i think. :)
ok...I need some work here...I don't really understand map
at all...I like the test ? case_one : case_two construct, but
I never use it
[ snip - same mistakes in code discussed ]
>
> sf> my @array;
> sf> if ($kv{kw_1} && !$kv{kw_2} && !$kv{kw_3}) {
> sf> @array = split /\n/, $kv{kw_1};
> sf> if ($kv{skip_space}) { push @array, simple_merge(@array) }
> sf> }
> sf> if ($kv{kw_1} && $kv{kw_2} && !$kv{kw_3}) {
> sf> @array = combine_two($kv{kw_1}, $kv{kw_2});
> sf> if ($kv{reverse_words}) {
> sf> push @array, combine_two($kv{kw_2}, $kv{kw_1});
> sf> }
> sf> if ($kv{skip_space}) { push @array, simple_merge(@array) }
> sf> }
> sf> if ($kv{kw_1} && $kv{kw_2} && $kv{kw_3}) {
> sf> @array = combine_three($kv{kw_1}, $kv{kw_2}, $kv{kw_3});
> sf> if ($kv{reverse_words}) {
> sf> push @array, combine_three($kv{kw_3}, $kv{kw_2}, $kv{kw_1});
> sf> }
> sf> if ($kv{skip_space}) { push @array, simple_merge(@array) }
> sf> }
> sf> my @return_array;
> sf> if ($kv{no_quotes}) { @return_array = @array }
> sf> if ($kv{quotes}) {
> sf> for my $element (@array) { push @return_array, qq("$element") }
> sf> }
> sf> if ($kv{brackets}) {
> sf> for my $element (@array) { push @return_array, qq([$element]) }
> sf> }
> sf> if (!$kv{no_quotes} &&!$kv{quotes} && !$kv{brackets}) { @return_array = @array }
> sf> return @return_array;
> sf> }
>
>that sub is totally incomprehensible. it looks like it had redundant and
>repeated code and calls those odd little subs you have written. no idea
>what it wants to do.
what the hell is that? did I write that? geez...that should be a thread in
itself...sorry about it ;-)
[ snip - a few more points and then more about cgi.pm ]
well, thanks again...OK, I will rewrite my selfish code and post it again
to see if it is more social
------------------------------
Date: Tue, 03 Aug 2004 10:42:44 -0400
From: steve_f <me@example.com>
Subject: Re: any pointers please? combine words script
Message-Id: <6s8vg0h01aidqpsvijf5qpeefp3prj3vpt@4ax.com>
On 01 Aug 2004 12:44:19 +0100, Brian McCauley <nobull@mail.com> wrote:
>steve_f <me@example.com> writes:
>
>> Hey, if you got a minute have fun with this script and give me some pointers.
>> The combine_hash idea was probably wrong, I had it before to run a few
>> scripts off of main.cgi but it got out of hand.
>
>This is way too long to address all in one place. I'll just look at
>one isolated part in this branch of the thread.
>
>> sub simple_merge {
>> my @words = @_;
>> my @return_array;
>> for my $word (@words) {
>> my @array = split(//, $word);
>> my $i = 0;
>> my @location;
>> for my $character (@array) {
>> if ($character =~ / /) {
>> push @location, $i;
>> }
>> $i++
>> }
>> for my $num (@location) {
>> my $temp;
>> for my $i (0..$#array) {
>> if ($num == $i) { next }
>> else { $temp .= $array[$i] }
>> }
>> push @return_array, $temp;
>> $temp = ();
>> }
>> }
>> return @return_array;
>> }
>
>You are rolling your own string manipultions. Perl has very good
>string manipulations.
>
>You are using the variable name $word for something that will contain
>spaces. In most languages words do not contain spaces.
Yes, thanks...I should use $string instead. As Uri was telling me, these kinds
of thinks get noticed when private code becomes public.
>
>If you want to do some thing for all the spaces in a string then
>simply use while (/ /g).
>
>sub simple_merge {
> my @merged;
> for (@_) {
> while (/ /g) {
> push @merged => substr($_,0,pos()-1).substr($_,pos);
> }
> }
> @merged;
>}
>
>Actually this is one of the few places I'd like to use "$`$'" but
>there's a nasty overhead using $` and $' so I won't.
ok...this looks neat, but I need to walk through each combination:
one two three four
onetwo three four
one twothree four
one two threefour
so I build an index into an array of the location of spaces, and then
loop through the strings a few times and skip each space in turn.
------------------------------
Date: Tue, 03 Aug 2004 14:52:27 GMT
From: Uri Guttman <uri@stemsystems.com>
Subject: Re: any pointers please? combine words script
Message-Id: <x7u0vkb7fr.fsf@mail.sysarch.com>
>>>>> "sf" == steve f <me@example.com> writes:
sf> On Sun, 01 Aug 2004 05:17:48 GMT, Uri Guttman <uri@stemsystems.com> wrote:
sf> my %kv = get_input($config{cgi});
>>
>> don't use short cryptic variable names like kv. and you have too many
>> file lexicals IMO (but i can tell for sure unless i do a full analysis).
sf> good point...using %kv and %config drives me up the wall...I forget
sf> what is what.
sf> file lexicals = global variables?
global to the file.
>> don't put blocks on one line like that.
sf> this I do not think is so critical
it is not a standard style. you need to read more perl code and you will
rarely see that done.
>> don't parse a query like that. gack! what if someone sent you an encoded
>> value for mkt which had %20 like hex in it?
sf> in the code, the query can only be boxes=1, boxes=2 and boxes=3,
sf> so I didn't think it mattered...what is a better way though?
use a general query parser like cgi.pm. if you ever need to change the
query, it will still work but your home grown code may not. also it is
easier to understand for the reader who will know what cgi.pm does and
not quickly grasp your code.
>>
sf> if (!$num) { $num = 2 }
>>
>> $num ||= 2 ;
sf> cute, but I really don't think it reads well...it means if $num doesn't
sf> exist then $num = 2?
yes. it is a very common perl idiom so use it. most all perl hackers
will know it.
>> my ($source, %hash) = @_;
>>
>> assign from @_ is more common and better IMO than shift
sf> ok...this is cute too, I like it.
cute isn't the best word for it. it is just more common and easier to
follow.
>> return join " | \n", map {
>> $_ eq $source ?
>> "<b>$source</b>" :
>> qq{<a href="./$hash{$_}">$element</a>}
>> } sort keys %hash ;
>>
>> not too cluttered, i think. :)
sf> ok...I need some work here...I don't really understand map
sf> at all...I like the test ? case_one : case_two construct, but
sf> I never use it
i wonder why so many perl newbies have trouble with map. the ?: operator
is very useful and should be part of your vocabulary (as should
map). seach google news for many threads on map.
sf> well, thanks again...OK, I will rewrite my selfish code and post it again
sf> to see if it is more social
good that you are grasping that concept. it is critical to your growth
as a coder.
uri
--
Uri Guttman ------ uri@stemsystems.com -------- http://www.stemsystems.com
--Perl Consulting, Stem Development, Systems Architecture, Design and Coding-
Search or Offer Perl Jobs ---------------------------- http://jobs.perl.org
------------------------------
Date: 6 Apr 2001 21:33:47 GMT (Last modified)
From: Perl-Users-Request@ruby.oce.orst.edu (Perl-Users-Digest Admin)
Subject: Digest Administrivia (Last modified: 6 Apr 01)
Message-Id: <null>
Administrivia:
#The Perl-Users Digest is a retransmission of the USENET newsgroup
#comp.lang.perl.misc. For subscription or unsubscription requests, send
#the single line:
#
# subscribe perl-users
#or:
# unsubscribe perl-users
#
#to almanac@ruby.oce.orst.edu.
NOTE: due to the current flood of worm email banging on ruby, the smtp
server on ruby has been shut off until further notice.
To submit articles to comp.lang.perl.announce, send your article to
clpa@perl.com.
#To request back copies (available for a week or so), send your request
#to almanac@ruby.oce.orst.edu with the command "send perl-users x.y",
#where x is the volume number and y is the issue number.
#For other requests pertaining to the digest, send mail to
#perl-users-request@ruby.oce.orst.edu. Do not waste your time or mine
#sending perl questions to the -request address, I don't have time to
#answer them even if I did know the answer.
------------------------------
End of Perl-Users Digest V10 Issue 6798
***************************************
