Beefy Boxes and Bandwidth Generously Provided by pair Networks
go ahead... be a heretic
 
PerlMonks  

Re: Code Critique

by tachyon (Chancellor)
on Dec 19, 2001 at 17:56 UTC ( #133115=note: print w/replies, xml ) Need Help??


in reply to Code Critique

I have had a look at your code. This is a big project but you have made it far more complex than it needs to be. The major and completely overriding comment is that you use *global variables* extensively. This not only makes code error and bug prone but also makes it a pain in the arse to follow. I commend Use strict warnings and diagnostics or die to you.

-T You really need to read perlman:perlsec and use taint checking.

You are interpolating scalar varaibles directly into SQL instead of using placeholders ? Worse still, you don't escape or untaint these values - some of which are raw user input. For details on why this is really bad and may make you sad see This Short DBI Tutorial by MJD It is possible for a malicious user to destroy your database as it currently stands. Even a well meaning O'Deary may wreak havoc.

A next comment is that you have hand rolled an HTML parser when HTML::Template is sitting there waiting for you.

You use die extensively which is rather silly in CGI programs. A die_nice() sub is usually a good idea. In this sub you print a cover all BS message to the user so they don't get a 500 internal server error. You can also email yourself, log the date/time/error etc. A typical structure might be:

open FILE, $file or die_nice("Can't open $file: $!"); sub die_nice { $real_problem = shift; $user_fob_off = "Sorry, our server can not respond to your request + due to "; $user_fob_off .= "routine maintenance ;-) Please try again later." +; tell_user( Title=>"Sorry!", Content=>$user_fob_off, Link=>$home ); email( To=>"admin", From=>"die_nice()", Subject=>"die_nice() error +", Content=>$real_problem ); exit; } sub email { my %mail = @_; *MAIL = *STDOUT; # open mailprog simulation my $message = unindent(" To: $mail{'To'} From: $mail{'From'} Subject: $mail{'Subject'} $mail{'Content'} "); print MAIL $message; } # remove leading whitespace indentation to allow # nice code formatting with strings indented inline sub unindent { $data = shift; $data =~ s/^\s+//; $data =~ s/^ +//gm; return $data; } sub tell_user{}

You might like to investigate the use of here pages, pod and modules

You have far too many scripts that are to all intents one or two subroutines long. This makes following your code tedious. It is better IMHO to bundle related functions into a module or 10 and then call these from your main script(s). Logically (to me) the main script just calls one of either the user/staff/admin interface modues. These then do their things by calling other modules such as say the Password module that does all the password validation/modification or the Job module that adds/modifies/deletes jobs.

cheers

tachyon

s&&rsenoyhcatreve&&&s&n.+t&"$'$`$\"$\&"&ee&&y&srve&&d&&print

Log In?
Username:
Password:

What's my password?
Create A New User
Node Status?
node history
Node Type: note [id://133115]
help
Chatterbox?
and the web crawler heard nothing...

How do I use this? | Other CB clients
Other Users?
Others rifling through the Monastery: (5)
As of 2019-12-08 21:54 GMT
Sections?
Information?
Find Nodes?
Leftovers?
    Voting Booth?

    No recent polls found

    Notices?