Re: Code Critique
by tachyon (Chancellor) on Dec 19, 2001 at 17:56 UTC
|
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
| [reply] [d/l] |
Re: Code Critique
by stefan k (Curate) on Dec 19, 2001 at 16:02 UTC
|
You should:
- make your links real links
- note the license somewhere on your website.
- not demand that downloaders must give name and email adress; this will prevent many people (like me) to actually download your programm and you need that to get the community involved
- be aware that one probably can't have a quick look at the source of a project the size of a helpdesk
Regards... |
Stefan
|
you begin bashing the string with a +42 regexp of confusion
|
| [reply] |
|
Hi Stefan,
I put a form there just out of interest of who's using the program, its nice to know how many people etc - but i'll remove it if people are not d/l because of it.
I will note the license on the site, I forgot to do that, thanks for pointing it out
The SOURCE file is here
The demo is here
| [reply] |
|
| [reply] |
|
Re: Code Critique
by gmax (Abbot) on Dec 19, 2001 at 16:44 UTC
|
To be fair, I could download the
source code without leaving any e-mail address.
And, indeed, every script says that it is under GPL.
However, one thing that is definitely missing is documentation. By looking at the code itself you can't get the feeling of how things are organized; and the demo can't tell you what's under the hood.
So, I would add some (a lot!) of POD to those scripts.
Finally, one last note. No time to have a look at your code, but, just a shot in the dark. Running:
grep -r "use strict" *
from the root of your snapshot, I get an empty list, meaning that none of your scripts are using it.
The same command inquiring for "-w" and "use CGI" returns a significant list.
gmax
| [reply] [d/l] |
Re: Code Critique
by IOrdy (Friar) on Dec 19, 2001 at 17:34 UTC
|
Why dont people embrace the concept of templates? I'm the first to admit my perl knowledge is still very limited but one of the first modules I ever used was HTML::Template. Seperating your code/content/style just plain makes sense. (i.e what css is to html).
Apart from the use strict comment (I got use strict and -w bitch slapped into me when I first started sratching out code in perl) I think that templates should be the next thing you implement.
That's after any coding changes the monks suggest though.
Forgive my english, I'm Australian. | [reply] |
|
| [reply] |
|
You still miss the bigger picture, it's not about making the
generation of HTML from Perl code easier or abstract - it's
about moving the HTML out of the Perl code. Trust me, this
is a good thing. Also, understand that I didn't get it at
first either - the whole 'seperation of presentation from
logic' seemed like a complete waste of time. Remember, my
first job out of school was developing in Cold Fusion. I
literally had to take over the job of the UI designer,
because they were too afraid to touch the site once all
of their HTML had been moved into the Cold Fusion code.
(you have read my tut on HTML::Template, right? ;))
I have heard you mention in the CB, why use a templating
system when CSS is available. Because CSS isn't powerful
enough, it only defines markup. Templating systems allow
you to package up all dynamic content for a page in a
convenient (well, convenient for a Perl programmer) data
structure and apply it to HTML. Very similar to CSS, but
instead it offers the power to control large chunks of
presentation - only display this for users logged in, only
display that for anonymous users. How do you think this
site does what it does? ;)
You mentioned HTML::Template wouldn't be powerful enough
for you most of the time. I can't agree with that, but i
can agree that
a templating system might be 'over-kill' most
of the time. If you are working with a team of developers
and a team of UI designers, then you will see the power
of templating systems.
I am not saying that you should rush out and port all of
your existing sites to a templating system, i am just saying
please don't knock it till you have fully experienced it.
jeffa
L-LL-L--L-LL-L--L-LL-L--
-R--R-RR-R--R-RR-R--R-RR
F--F--F--F--F--F--F--F--
(the triplet paradiddle)
| [reply] |
|
I agree... discounting a few things that HTML::Template may be useful for, I always found it hard to actually 'wrap' my code around a template. I know that if separating code from content is an issue, then wouldn't it be simpler to move on to Mason? However, I'm not particular fan of this either since many Mason scripts I looked at appeared to be quite a mess.
Having said that, I am actually trying to move more towards splitting display logic from program logic. One way to do this, I think, would be to have your program generate and store all of its 'working' data mean to be displayed in some fashion in a hash or some complex enough structure which yet could be looped through/accessed via a templating script (such as <TMPL_LOOP ... > </TMPL_LOOP> implemented in HTML::Template).
Since I'm yet not a strong believer in Perl templating utilities, would anyone with some related experience share in his/her joys of using them? I'd be interested in hearing from someone who was in a position similar to I'm in now (that is 'hardly decided') and how they managed to integrate this concept in their code.
PS: my native language is Russian so don't mind my English! ;-)
"There is no system but GNU, and Linux is one of its kernels." -- Confession of Faith
|
| [reply] |
|
|
|
|
|
|
|
Re: Code Critique
by tachyon (Chancellor) on Dec 19, 2001 at 16:44 UTC
|
Seems fairly functional. One small aesthetic note - some of your banners have an outline <table border="1"... and others do not.
Unless you want anyone to be able to have fairly free and full access to your site you should make your web dirs non-world readable. Have a look http://www.perldesk.com/dl/. If this is what you want the world to see then feel free to ignore....
cheers
tachyon
s&&rsenoyhcatreve&&&s&n.+t&"$'$`$\"$\&"&ee&&y&srve&&d&&print
| [reply] |
|
Hi Tachyon
Thanks for the reply, I have the dirs world readable so people can go in an greab the release they want, the most recent will be in the current directory though
Thanks
John
| [reply] |