Around here, that's a challenge. :)

A couple of comments:

1. 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.
2. use strict; will keep you out of trouble, as well. (And it would have pointed out one of the problems below.) use strict; sooner.
3. Insteading of saying "Perl (vs. 5)" in a comment, write

   require 5.0;

   Then Perl will enforce the version requirement for you.
4. 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?
5. Add error checking to $sth->execute();
6. 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.

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.

I'd put it on the shebang line if that were possible..

perldoc perlrun

#!/usr/bin/perl -Mstrict -wT
[download]

update:

Ooooooooooooooooooooh .... *crash*n*burn* ;)(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.

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.

.. 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.

maybe  $sort = $q->param("sort")

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

You use object notation throughout, so you don't need to import any functions. Line 14 becomes:

What is supposed to happen if some rascal sends this in a form?
sort=valid';delete from sysmain.humans where '1

After Compline,
Zaxo

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.

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.

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.

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]

use CGI qw/:standard/;
[download]

use CGI;
[download]

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:

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

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]

while () { 
    ...;
    }
[download]

while () 
    {
    ...; 
    }
[download]

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.";

$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.