redsquirrel has asked for the wisdom of the Perl Monks concerning the following question:

I would greatly appreciate ANY feedback anyone could provide. The following page will provide details and a link to download the tarball.

http://www.redsquirreldesign.com/soapbox

TIA

Dave Hoover
"Twice blessed is help unlooked for." --Tolkien
http://www.redsquirreldesign.com/dave

Replies are listed 'Best First'.
Re: Code Review Needed!
by petdance (Parson) on Jun 30, 2001 at 02:09 UTC
    I suspect that you'd get more responses (at least from me) if:
    1. We didn't have to go to another site, pull down a tarball, unzip it, examine it, etc etc.
    2. We had more specifics of what you're looking at that's potentially problematic. 1300 lines is quite a bit of code to just glance over.

    xoxo,
    Andy
    --
    I was dreaming when I wrote this, so sue me if I go too fast.

Re: Code Review Needed!
by CharlesClarkson (Curate) on Jun 30, 2001 at 03:56 UTC

    If I understand what your attempting in main.cgi this snippet:

    # Load the keywords used my ($terms, @terms); if ($s->{terms}) { @terms = @{$s->{terms}}; for (@terms) { $terms .= "$_ " } chomp($terms); # for some reason chomp wasn't working here }

    might be rewritten:

    # Load the keywords used my $terms; if ($s->{terms}) { $terms = join ' ', @{$s->{terms}}; }

    but more important is what the code should do if if $s->{terms}is false? Currently strict should throw an error when $terms is used later.


    HTH,
    Charles K. Clarkson
      Charles,

      Thanks for the tip on using join. Tad McClellan on comp.lang.perl.misc suggested the same thing.

      Regarding the issue with if ($s->{terms}), I'm not getting any errors. my $terms is declared outside of the if statement block so I think that is keeping strict happy.

      Thank you for taking the time to look at my code.

      --
      Dave Hoover
      "Twice blessed is help unlooked for." --Tolkien
      http://www.redsquirreldesign.com/dave

        Your not getting any errors because $s->{terms} always tests true. What if one day it tests false? That is why your testing it.

        If it does test false one day, $terms will be undef and strict will throw an 'unitialized' error here:

        # Assign the inserted values $s->{insert} = { all => [ "%All%", $all ], any => [ "%Any%", $any ], terms => [ "%Terms%", $terms ], . . .

        HTH,
        Charles K. Clarkson

        Biologists have a word for something that doesn’t change: dead.