Right now, I'm running into a lot of legacy code like this:
#------------------------#
sub GetValues
#------------------------#
{
my $dbh = get_connection('databasename');
my $sth = $dbh->prepare('Select * from table_name');
$sth->execute();
my $r;
my @a;
while ($r = $sth->fetchrow_hashref) {
push @a, $r;
}
return (\@a);
}
my "Improved" version would look more like this:
#------------------------#
sub GetValues
#------------------------#
{
my $dbh = get_connection('databasename');
my $ok = 1;
my $msg = "Success";
my $sth;
if (!$dbh) {
$ok = 0;
$msg = "Failed to get connection to databasename in GetValues";
} else {
$sth = $dbh->prepare('Select * from table_name');
}
if ($ok && (!$sth)) {
$ok = 0;
$msg = "Call to prepare failed in GetValues";
}
my $r;
my @a;
if ($ok) {
$ok = $sth->execute();
if (!$ok) {
$msg = "Call to excecute failed in GetValues";
} else {
my $r;
my @a;
while ($r = $sth->fetchrow_hashref) {
push @a, $r;
}
}
return ($ok, $msg, \@a);
}
#------------------------#
Note that my new code is literally TWICE as long as the old code. Often, it's longer.
Is this really advantageous? In the long run, it feels like I'm trying the same things over and over again. I actually made a module to abstract out the if-ish ness of the this routine, but the functions got to the point where they were taking 6+ paramters, some of which were optional. It was just ugly. So I've stopped using it until I can do it over again with anonymous hashrefs and "named params."
The net result is that plenty of folks are saying "error checking is over-rated", and I can see where they are coming from. What's the "right" answer?
#------------------------#
sub GetValues
#------------------------#
{
my $dbh = get_connection('databasename')
or return( 0, "Failed to get connection to databasename in GetValues" );
my $sth = $dbh->prepare('Select * from table_name')
or return( 0, "Call to prepare failed in GetValues" );
my @a = $sth->execute()
or return( 0, "Call to excecute failed in GetValues" );
my $r;
my @a;
push @a, $r
while $r = $sth->fetchrow_hashref;
return (1, \@a);
}
#------------------------#
I find code like this pretty cringe-worthy. If you set RaiseError and HandleError, you can have something this simple:
sub get_values {
my $dbh = get_connection('databasename');
return $dbh->selectall_arrayref(
'SELECT * FROM table_name', { Slice => {} }
);
}
Isn't that much nicer than 25 lines of code? I'm also not keen on returning flags for errors, you'll just have to write lots of code to check, just die if something bad happened and trap it with eval.
Re:ick
htoug on 2003-09-17T19:46:34
Yes, and most of the time, the correct reaction to something bad happening is to die, so you can even drop the eval.
Re:{ RaiseError = 1 }
tobert on 2003-09-26T14:56:30
Hmm, I didn't look and see that the pre tag isn't allowed. bummer.
The best way I've found is to use RaiseError => 1, which tells DBI to always throw an exception when there is an error. This way, you never have errors go unnoticed. If you want an error to not be fatal, use eval.
eval {
$dbh->do("DROP TABLE users");
};
if ( $@ ) { my_error_handler( $@ ) }
Another method I use is a function that does a little more digging for me before printing it's error message.
sub check_dbi_errors ($) {
my $sth = shift;
return if (!$sth->err);
warn $sth->errstr;
if ( $debug ) {
print "SQL for statement was: \"",
$sth->{Statement}, "\"\n";...
}
}
my $sth = $dbh->prepare(
execute( $value_for_d );
check_dbi_errors( $sth );...
