A Matter of Style in CGI

Spenser has asked for the wisdom of the Perl Monks concerning the following question:

Replies are listed 'Best First'.
Re: A Matter of Style in CGI by dws (Chancellor) on Sep 12, 2002 at 18:19 UTC
No criticism will be considered too minor. Around here, that's a challenge. :) A couple of comments: Get into the habit of taint-checking, via -T. You'll find straightway that you need to de-taint `param("sort")`, since somebody could sneak in something that would corrupt the SQL statement you're building. ~~`use strict;` will keep you out of trouble, as well. (And it would have pointed out one of the problems below.)~~ `use strict;` sooner. Insteading of saying "Perl (vs. 5)" in a comment, write `require 5.0;` Then Perl will enforce the version requirement for you. You need to think through your error handling. In particular, what do you want the user experience to be if either you fail to connect to the database or if your query fails to execute? Add error checking to `$sth->execute();` ~~What do you intend to do with `$style`?~~ Never mind. I looked, but didn't at first see where you were using it. The rest looks O.K. at a casual glance. You might want to look into using one of the available templating mechanisms (e.g., HTML::Template) so that you can edit your HTML separate from program logic.	[reply] [d/l] [select]
Re^2: A Matter of Style in CGI by Aristotle (Chancellor) on Sep 12, 2002 at 18:30 UTC
He does `use strict;` - it just comes after the comments.. (I was bitten by that in my first glance too as I tend to put the strict line up top below the shebang. I'd put it on the shebang line if that were possible..) I also much prefer Template Toolkit 2 to HTML::Template - to each his own, so I thought I'd point out the alternative. Makeshifts last the longest.	[reply]
Re: Re^2: A Matter of Style in CGI by PodMaster (Abbot) on Sep 13, 2002 at 06:57 UTC
I'd put it on the shebang line if that were possible.. perldoc perlrun `#!/usr/bin/perl -Mstrict -wT` [download] update: Ooooooooooooooooooooh .... crashnburn ;)(i'd say i was about due for one of these ~ didn't try it) `____________________________________________________` ** The Third rule of perl club is a statement of fact: pod is sexy.	[reply] [d/l]
Re: Re: Re^2: A Matter of Style in CGI by rob_au (Abbot) on Sep 13, 2002 at 11:26 UTC
Re: A Matter of Style in CGI by Kanji (Parson) on Sep 12, 2002 at 19:31 UTC
Rather than mix n' matching hardcoded HTML and CGI's HTML shortcuts, consider using just the latter for consistency ... `# <link rel='stylesheet' href='/styles/hr.css' type='text/css' /> $q->start_html( -title => 'Employee Table', # etc.. -style => { src => '/styles/hr.css' }, ), # <span class=section-heading>Employee Table</span> $q->span({-class=>"col-heading"},"Employee Table"),` [download] You may also want to remove quotes from strings made up solely of a lone scalar (ie, `$emp_id` vs. `"$emp_id"`), as the interpolation of such serves no purpose. Lastly, to expand on comments by dws and Zaxo, take a look at using placeholders in your SQL. --k.	[reply] [d/l] [select]
Re^2: A Matter of Style in CGI by Aristotle (Chancellor) on Sep 12, 2002 at 20:20 UTC
.. remove quotes from strings made up solely of a lone scalar (..), as the interpolation of such serves no purpose. Good catch; I'd go even further with that advice and say you should never do it unless you know why not. It can potentially be very harmful since it stringifies anything: references are flattened into useless text. That's very unlikely to be something you are intending. In the case of other kinds of scalar values, it's no harm, however neither does it do anything useful. So while it doesn't necessarily hurt, it doesn't gain you anything either. And occasionally it can hurt big time. Bottom line, don't do it. Makeshifts last the longest.	[reply]
Re: A Matter of Style in CGI by fglock (Vicar) on Sep 12, 2002 at 18:07 UTC
maybe `$sort = $q->param("sort")`	[reply] [d/l]
Re: Re: A Matter of Style in CGI by twerq (Deacon) on Sep 12, 2002 at 18:15 UTC
Good point. Seems the OP is mixing Function-Oriented with Object-oriented use of CGI.pm Have a look at http://stein.cshl.org/WWW/software/CGI/cgi_docs.html#functionvsoo, which gives a pretty good run down of the difference. Mostly, you're using the $q object, which is great. It just means that you don't need to bother importing the standard functions into your namespace with `use CGI qw/:standard/;` . . . `my $q = new CGI;` should be enough. --twerq	[reply] [d/l] [select]
Re: A Matter of Style in CGI by Zaxo (Archbishop) on Sep 12, 2002 at 18:20 UTC
You use object notation throughout, so you don't need to import any functions. Line 14 becomes: `use CGI;` What is supposed to happen if some rascal sends this in a form? `sort=valid';delete from sysmain.humans where '1` After Compline, Zaxo	[reply] [d/l]
Re: Spiking the mySQL parameter by Spenser (Friar) on Sep 12, 2002 at 20:04 UTC
All of the comments here have been useful and I'm absorbing them fully--thanks to everyone. However, this one by Zaxo about a user spiking a param with an extra mySQL statement to be inserted at the end of my mySQL statement, I can't figure out how to resolve. Talking it over with cerberus who works with me, we figured out that we can do something like this: `my $sort = $q->param("sort"); if($sort ne "emp_id" \|\| $sort ne "emp_last" \|\| $sort ne "dept") { $sort = "emp_last"; }` [download] This would get rid of any destructive mySQL statements a hacker might throw into the CGI parameter. However, what about scripts where we're taking in a search parameter? I thined out some pieces of the script above, emp-list.cgi, for a shorter and more concise post. One piece I left out is a search feature which I feel I should now post for this side question: `my $search_text = param("search_text") \|\| ""; if($search_text ne "") { $sql_stmnt = "SELECT emp_id, CONCAT(emp_first, ' ', emp_last) FROM sys_main.humans WHERE emp_first LIKE '%$search_text%' OR emp_last LIKE '%$search_text%'"; $sth = $dbh->prepare($sql_stmnt); $sth->execute(); while(@emp_matches = $sth->fetchrow_array()) { $emp_matches{$emp_matches[0]} = $emp_matches[1]; } }` [download] Here I'm basically getting a list of matching names and putting them in a hash for the user to choose the specific employee she wants to view details on. In this case, we wouldn't know all of the acceptable answers and couldn't filter out hacking attempts so easily. Any thoughts? -Spenser Update To answer my own question for future reference by others, I believe I've figured out how to stop a user from appending a CGI/mySQL query statement with the following as Zaxo suggested: `...;delete from sysmain.humans where '1'` You just change the user permissions in mySQL not to allow deletion of records by the CGI script user.	[reply] [d/l] [select]
Re: Re: Spiking the mySQL parameter by dws (Chancellor) on Sep 12, 2002 at 20:14 UTC
You can rewrite `$sql_stmnt = "SELECT emp_id, CONCAT(emp_first, ' ', emp_last) FROM sys_main.humans WHERE emp_first LIKE '%$search_text%' OR emp_last LIKE '%$search_text%'"; $sth = $dbh->prepare($sql_stmnt); $sth->execute();` [download] as `$sql_stmnt = "SELECT emp_id, CONCAT(emp_first, ' ', emp_last) FROM sys_main.humans WHERE emp_first LIKE ? OR emp_last LIKE ?"; $sth = $dbh->prepare($sql_stmnt); $sth->execute("%$search_text%", "%$search_text%");` [download] and get the benefit of having the values automatically quoted for you.	[reply] [d/l] [select]
Re: Re: Spiking the mySQL parameter by Hero Zzyzzx (Curate) on Sep 13, 2002 at 16:45 UTC
This is how I deal with user-defined sort orders, which I (personally) think is a little more readable. . .This is taken straight from production code. . .Plus, to add more sorts you only need to add them to the drop-down and the hash. my %sorts=( form=>'form_list.form_title', rform=>'form_list.form_title desc', name=>'stored_forms.client_name', rname=>'stored_forms.client_name desc', prepared=>'stored_forms.advocate_name', rprepared=>'stored_forms.advocate_name desc', date=>'stored_forms.created', rdate=>'stored_forms.created desc' ); my $sort=$sorts{$q->param('sort')} \|\| $sorts{'form'}; my $forms=$dbh->prepare("select stored_forms.client_name, form_lis +t.form_title, stored_forms.id, stored_forms.advocate_name,date_format(stored_forms.created,'%c/%e/%Y' +) as save_date, stored_forms.client_accountnum,form_list.form_location from form_list, stored_forms where form_list.form_type = stored_forms.form_type and stored_forms.client_accountnum like ? order by stored_forms.client_accountnum,".$sort); [download] HTH -Any sufficiently advanced technology is indistinguishable from doubletalk.	[reply] [d/l]
Re: A Matter of Style in CGI by hmerrill (Friar) on Sep 12, 2002 at 20:01 UTC
DBI things: 1. Check out the database handle attributes RaiseError, PrintError, and AutoCommit. For error checking(and catching) your DBI statements, especially in CGI scripts where you may not want to die(you might want to give a nice Error screen) when you have a bad problem, you can't beat RaiseError and "eval". 2. Get into the habit of using placeholders(?) - although they won't really buy you much here, you're bound to run into a situation where they will help with quoting issues and/or performance. Is use them all the time. 3. while (@_ = $sth->fetchrow_array()) { $emp_id = $_[0]; $emp_name = $_[1]; $dept = $_[2]; can be written to be more easily understandable this way: while (($emp_id, $emp_name, $dept) = $sth->fetchrow_array()) { no sense using array subscripts when it's not necessary. You might also have a look at fetchrow_hashref. [download] I'm also going to throw this one out there, just in case you're not familiar with the excellent documentation that comes with Perl - I worked for quite a few years with Perl myself before someone clued me in. If you're not familiar with the "perldocs", start by doing "perldoc perldoc" at a command prompt. You can see the perldocs for most any Perl module by doing "perldoc module name" at a command prompt. For example, to view the DBI perldocs, do "perldoc DBI". My appologies if you already knew about this stuff. HTH.	[reply] [d/l]
Re: A Matter of Style in CGI by TStanley (Canon) on Sep 12, 2002 at 18:12 UTC
And since you are using the OO method, you don't need to import the 'standard' tags in the use statement, so this: `use CGI qw/:standard/;` [download] becomes this: `use CGI;` [download] TStanley -------- It is God's job to forgive Osama Bin Laden. It is our job to arrange the meeting -- General Norman Schwartzkopf	[reply] [d/l] [select]
Re: A Matter of Style in CGI by Flexx (Pilgrim) on Sep 12, 2002 at 20:07 UTC
Hi! I got some thoughts on your script, I'd like to share with you. You dared to ask, so you deserve no better. HARHAR!! ;) Not all of it is CGI-related, but nonetheless I hope it's interesting. `my $sql_stmnt = "SELECT emp_id, CONCAT(emp_last, ', ', emp_first), dep +t_name FROM sys_main.humans, sys_main.depts WHERE status='AV' AND dept_id=dept ORDER BY '$sort'"; my $sth = $dbh->prepare($sql_stmnt); $sth->execute();` [download] This is fine. I do not know mySQL (our DB folks like Oracle ;), but if it supports bind variables in the ORDER BY clause, you could say: `my $sth = $dbh->prepare_cached(<<"+++end_of_sql_statement"); SELECT emp_id, CONCAT(emp_last, ', ', emp_first), dept_name FROM sys_main.humans, sys_main.depts WHERE status='AV' AND dept_id=dept ORDER BY ? +++end_of_sql_statement $sth->execute($sort);` [download] Using prepare_cached might save you some milliseconds, if mySQL allows for it. If cached, using a placeholder in the ORDER BY clause will allow to have the statement cached independent from the sort column. Unless you need the value of `$sort` elsewhere you could even say: `$sth->execute($q->param('sort') \|\| 'emp_last')` to leave things in place. Also, having sort double quoted is a waste of time, at least of optimizer time, but that's pernickety. ;) Just as a matter of taste, I like to put my SQL statments in here docs, as above. `$q->start_html(-title=>'Employee Table', -bgcolor=>'#FFFFFF', -link=>'#4682B4', -vlink=>'#5F9EA0', -alink=>'#1E90FF') $q->start_table({-width=>'450', -border=>'0', -cellpadding=>'2', -cellspacing=>'0'})` [download] Wouldn't you wan't have all those style attributes defined in the stylesheet you link to? Even if the imported stylesheet is not in your hands, you could define your styles in a dedicated style tag in the head section of the generated output: `my $stylesheet = <<'++end_of_stylesheet'; .emp_table { width: 450 pt; border-width: 0 pt; padding: 2 pt; } ++end_of_stylesheet print $q->start_html(-title => 'Employee Table', -style => { -code =>$stylesheet }), $q->start_table({ -class => 'emp_table', -width => 450 });` [download] Note that CGI.pm also provides a `span()` function allowing you to apply a class / style definition to multiple elements (which would save some of your HTML span tags). I am not sure, however, if that works in the OO interface, too. You hard-code the (own) script name several times: `-href=>"emp-list.cgi"` [download] Don't. Either use `my $SCRIPT_NAME = 'emp_list.cgi'` at file scope near the top of your file, so you'd have to change that only once if it changes. Even better is to use `$q->script_name()` for that. That way, you can rename the script file without having to change anything. Use as few literals as possible. Early in your script use: `my $STYLESHEET_DIR = '/stylesheets'; my $IMAGE_DIR = '/images';` [download] Actually, I like have such definitions in a CONF.pm and `use` that. Running under mod_perl, this will even save a bit memory making these variables shared. `while (@_ = $sth->fetchrow_array()) { $emp_id = $_[0]; $emp_name = $_[1]; $dept = $_[2];` [download] Consider: `while (my ($emp_id, $emp_name, $dept) = $sth->fetchrow_array) { ... }` [download] Hope this helps/is interesting, so long Flexx	[reply] [d/l] [select]
Re: A Matter of Style in CGI by sauoq (Abbot) on Sep 12, 2002 at 20:21 UTC
In keeping with the title, this is very much a matter of style. The one while loop in that code indicates that you seem to like blocks that look like: `while () { ...; }` [download] Blocks structured like that are difficult to read. The eye has to search for the ending curly. Perl isn't Python. In Perl, the curlies define the blocks and you should make them easy to spot. Some people insist that blocks should look like: `while () { ...; }` [download] I prefer blocks that look like: `while () { ...; }` [download] but I'm guessing that neither of those will appeal to you as much as `while () { ...; }` [download] or maybe `while () { ...; }` [download] or even `while () { ...; }` [download] Another reason not to use your current style is that by putting the curly on a separate line, you can add a line to the end of your block without moving the curly. The real benefit of that will be noticeable when you want to move lines of code around. You couldn't just cut that whole line and paste it elsewhere, for instance. That style can really ruin the day for someone that uses vi. Consistency is probably more important than the actual style you choose but you should still pick one that makes coding easier and your code eaier to read. -sauoq "My two cents aren't worth a dime.";	[reply] [d/l] [select]
Re: A Matter of Style in CGI by Ryszard (Priest) on Sep 13, 2002 at 07:42 UTC
As a matter of style I'd move your html generation out in templates and use something like HTML::Template or Template::Toolkit. You have the advantage of editing the look and feel of your HTML without the risk of accidently modifying your code. $q->a({-href=>"emp-list.cgi?sort=emp_id"} Consider something like `$query->url(-full=>1);` this way you can change the name of your CGI without having to hunt thru' your code to find the references to your hardcoded (tm) script name. Personally, if i were producing the above i would abstract the database routines into a seperate module make the call, return the data, then turn it into html. You have the advantage of reusable code, and a single maintaince point.	[reply] [d/l]


laziness, impatience, and hubris
	PerlMonks