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

for a bigger cms i use frequently the following subroutine, which handles different sorts of sql queries, as my database days are still young and this part of code is essential for the cms i would like to your opionions about these lines.

sql_query handles different queries ranging from smaller and bigger select statements to update and insert like

my $chk_arid = "SELECT MAX( arid ) FROM $table where flag = ?"; my ($arid) = sql_query( $chk_arid , 1 ); # or my $cmd = "select aid, code from authors where name = ? "; ( $aid, $code ) = sql_query( $cmd, $name ); # or even my $cmd_up = "UPDATE SET flag = 0 where flag = 1 and arid = ?"; sql_query( $cmd_up, $pub{arid} ) or die("Error update flag"); my $cmd_ins = "INSERT INTO $table ( arid, catid, filename, title, aid, teaser, text, links, comment, changeby, newsid, keywords, description, flag) VALUES ( ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)"; sql_query( $cmd_ins, $arid, $file, $pub{title}, $pub{aid}, $pub{teaser}, $pub{text}, $pub{links}, $comment, $aid, $pub{newsid}, $pub{keywords}, $pub{description}, 1 ) or die("Failed to insert new changed sql data :( \n");
the subroutine itself :))
sub sql_query { my $cmd = shift; my @place_holders; if ( defined $_[0] ) { foreach my $i ( 0 .. $_ ) { $place_holders[$i] = "$_[$i]"; } } die("Received no sql cmd for database query") unless ( defined +$cmd); my $dbh = DBI->connect("DBI:mysql:$dbname","$username","$passw +d"); die "ERR: Couldn't open connection: ".$DBI::errstr."\n" unless + $dbh; my $sth = $dbh->prepare( $cmd ); die "Couldn't prepare statement: $DBI::errstr stopped\n" unles +s $sth; $sth->execute( @place_holders ) or die "Couldn't execute statement: $DBI::errstr\n"; my @row = $sth->fetchrow_array() if ( $cmd =~ /SELECT/i ); $sth->finish; $dbh->disconnect; return @row if ( $cmd =~ /SELECT/i); return 1; }

Well everything is working so far, but are there any (small or big) relevant contraindication speeking against the use of this snippet?
thx in advance for your opinion and interessting pointers or cleanups maksl
(ps speed is not so a big limitation as this piece of code is only use for the authors .. users see plain htm)

Replies are listed 'Best First'.
Re: sql query subroutine check
by chromatic (Archbishop) on Mar 01, 2003 at 21:35 UTC

    You can simplify parameter fetching substantially.

    my ($cmd, @place_holders) = @_;

    That doesn't explicitly stringify the place holders, but I'm not sure it's necessary in your code. You don't need the double quotes around variables-as-arguments in connect(), either, unless you're doing something really complicated. (My guess is that you're not.)

    Be cautious with your return. In scalar context, since it's an array, you'll get the number of elements in the array. This may or may not be what you want.

    Connecting and disconnecting on every pass through the sub can be expensive.

    You may want to use Carp instead of die for error messages. It can be a bit friendlier.

      thanks for your thoughts chromatic i'll take them in account :))
      i have already fallen under the trap you mentioned (getting the numbers of the return array and not the expected values) .. son now i'm more cautious
      the above is not under heavy load .. it's for a friend .. doing it in freetime
Re: sql query subroutine check
by Ovid (Cardinal) on Mar 01, 2003 at 23:20 UTC

    I've recently become a convert to object-oriented persistence modules. Specifically, I use Class::DBI, though there are others which are good. One of the many benefits of these modules is, when used properly, they allow for greater decoupling of code from the specific database implementation. For example, consider the following SQL:

    my $cmd_ins = <<"END_SQL"; INSERT INTO product_metadata (desc, abbrv, prdct_id) VALUES (?, ?, ?) END_SQL

    Later, as the programmer realizes that the field names in the database are not very desriptive, she thinks to change them to description, abbreviation, and product_id. This makes it easier to see what's happening in the database, but also forces her to change her code in every spot where this table is referenced.

    As an alternative, I do something like the following:

    my %column_map = ( description => 'desc', abbreviation => 'abbrv', product_id => 'prdct_id' );

    Then, when I create an object that is persistent in the database, I use this mapping to map my desired accessor names to column names. Outside of the object, code relies on the accessor names and if I need to change the database column names, I just change them in %column_map and my code still works.

    While you are probably well into your project, this could save you much grief in the future, particularly in projects where you are developing your database at the same time as you develop the code. For a more complete explanation of this, see my response at


    New address of my CGI Course.
    Silence is Evil (feel free to copy and distribute widely - note copyright text)

      i'm using hashes quite frequently to map changing file_names but never thought about using it for my database column. so sometimes, i carry with me strange names for field names .. their use has changed in between .. one that was thought for news_id .. is used for position in the referenced cat ;)

      my %column_map = ( position => 'news_id', );

      makes this code easier to read for shure
      /me is taking a serious look at Class::DBI
      thx ovid for your pointer .. :-))) maksl