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.

At the suggestion of a few people I've converted the program into text files to make things easier for reviewers. Here are the four files:

http://www.redsquirreldesign.com/soapbox/Soapbox.pm.txt
http://www.redsquirreldesign.com/soapbox/main.txt
http://www.redsquirreldesign.com/soapbox/soap.txt
http://www.redsquirreldesign.com/soapbox/soapbox.conf.txt

I am particularly interested in feedback about:

• Any common newbie blunders you may find
• Security issues
• Specific areas where the code could be more efficient
• A critique on my use of object-oriented Perl
The feedback I have received thus far has been very helpful. It's been a learning experience...particularly about taint checking!

Thanks,

--Dave

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
    }
[download]

might be rewritten:

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

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.

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 ],
        . . .
[download]


HTH,
Charles K. Clarkson

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