Re: Critque my second script for me?

I agree with everything that Hero Zzyzzx and chromatic noted above. I'll just throw in my 2¢ worth of suggestions, for what it's worth :)

No spaces between subs and opening parens.

It may make your sub behave unexpectedly. For more info, check out this node.

Use CGI.pm for everything you can (especially if you're already using CGI.pm).

You've imported all of the standard fuctions, but are handrolling your HTML. For example, your bad_hacker_no_cookie has:

sub bad_hacker_no_cookie {
    print "Content-type: text/html\n\n";
     print "<HTML> \n";
     print "<body bgcolor=000000 text=99CCCC> \n";
     print "<CENTER><H1><font face=Arial>!Unauthorized Access!\n";
     print "<BR>";
     print "<font color = red>Hey bub, stop messin' with my News Page!
+\n";
     print "<BR>You are at IP address $user !";
     print "</H1></CENTER></font></body>";
     exit 0;
}
[download]

But you can tidy it up, use the functions you've already imported, and make sure it's valid html/xhtml code at the same time by using:

sub bad_hacker_no_cookie {

    print
        header,
        start_html( -bgcolor => '#000000', -text => '#99ccc'),
        h1( { -align => 'center' },
            '!Unauthorized Access!',
            br(),
            font( { -color => 'red' },
                "Hey bub, stop messin' with my News Page!",
                br(),
                "You are at IP address $user !",
            ),
        ),
        end_html;

}
[download]

Variable naming and grouping

I like to set my config vars (using mnemonic names) in either a hash or href. It helps me to identify them at a glance, especially when there are a lot of config settings.

$mail, in a large program with lots of settings may be confusing (was that the contents of a message, the path to the mail program, the type of mailer to use, an email address, etc.), especially when you pick up the script to modify it in 6 months. But if you had <NOBR>$cfg->{'email_notices_to'}</NOBR>, you would know exactly was it was for and the data it contains :)

Print from filehandle instead

If you're just opening a file to be output to the browser, you don't need to toss the whole file into memory. Here's the code from your show_news sub:

if ($topic eq "" && $words eq "") {
open (FTXT, "$path_to_text") or die "where's the text file? : $!";
my  @text_file = <FTXT>;
open (FHTML, "$path_to_header") or die "where's the html file? : $!";
my  @html_file = <FHTML>;
print "Content-type: text/html\n\n";
print @html_file;
print @text_file;
print "</table></div></body></html> \n";
exit;
[download]

Instead of this, you can use:

print header;

open( FHTML, $path_to_header ) or die "where's the html file? $path_to
+_header : $!\n";
print while <FHTML>;
close FHTML or die "Blahblah $path_to_header $!\n";  #you forgot to cl
+ose these filehandles

open( FTXT, $path_to_text ) or die "where's the text file? $path_to_te
+xt : $!\n";
print while <FTXT>;
close FTXT or die "Blahblah $path_to_text $!\n";

print '</TABLE></DIV></BODY></HTML>';
[download]

Hope at least some of this helps,
Jasmine

Comment on Re: Critque my second script for me? Select or Download Code