Just another Perl shrine | |
PerlMonks |
Re: Code Critiqueby tachyon (Chancellor) |
on Dec 19, 2001 at 17:56 UTC ( [id://133115]=note: print w/replies, xml ) | Need Help?? |
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:
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
In Section
Seekers of Perl Wisdom
|
|