|We don't bite newbies here... much|
Re: Code review, good 'ol CGI.by tachyon (Chancellor)
|on Jul 29, 2001 at 11:26 UTC||Need Help??|
A few points. When locking a file I use this little snippet to allow a script to wait for its lock:
Also closing a filehandle removes the underlying file lock so you only need to close them, Perl will unlock them for you thus all the LOCK_UN lines are redundant.
This line does nothing useful, it does not untaint $mode
This bit uses $mode in a regex and the value in $1 assigned to $page is untainted.
The or die makes no logical sense as the assignment $page = $1 will always succeed thus this can never execute.
You have a number of or die "blah" statements. You should include the $! special var as this stores Perl's explanation of the error that has triggered the die ie "File does not exist". The standard synatax goes like:
This code is a very obtuse way to do something quite simple, namely go to one of two subs depending on the user input.
You create a hash of sub references and then eventually call them. This is much easier to follow:
The logic is the same with the exception of not checking for 'incorrect' action params. This is the typical logic I use as it is simple - either you ask for something with the correct name or you get the default page. The use of warn and die as you do is also redundant. Both warn and die print to STDERR. Warn just does this whereas die throws an expception afterwards. Logically you are just doing a die here.
Your open files in two different places in the script then lock them all in the one place. This makes no logical sense to me and makes it hard to follow your logic train.
Finally in CGI die is oftem suboptimal. You should not run a production script with use CGI::Carp qw( fatalsToBrowser ) active as it makes it easier to hack a script as the exact errors are reported in the browser. Without carp die will give the user a 500 error when it gets called. For these reasons amongst others most developers write a die_nice routine. Here is a die_nice routine:
The unindent sub just lets me indent the herepage stuff with the sub and drop this indentation off the final output. Hope this helps, all up looks good with -Tw, use strict, setting a secure path etc.