Re: any pointers please? combine words script
From: steve_f (me_at_example.com)
Date: 08/03/04
- Next message: steve_f: "Re: any pointers please? combine words script"
- Previous message: Ala Qumsieh: "Re: Variable interpolation on STDIN ?"
- In reply to: Uri Guttman: "Re: any pointers please? combine words script"
- Next in thread: Uri Guttman: "Re: any pointers please? combine words script"
- Reply: Uri Guttman: "Re: any pointers please? combine words script"
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
Date: Tue, 03 Aug 2004 10:30:39 -0400
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{$element} ;
>
>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{$element}
> } 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
- Next message: steve_f: "Re: any pointers please? combine words script"
- Previous message: Ala Qumsieh: "Re: Variable interpolation on STDIN ?"
- In reply to: Uri Guttman: "Re: any pointers please? combine words script"
- Next in thread: Uri Guttman: "Re: any pointers please? combine words script"
- Reply: Uri Guttman: "Re: any pointers please? combine words script"
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
Relevant Pages
|
