No, that's not bad practice IMO. It evidently works for you, and I
think it's clear enough what's going on with that code. For the sake
of code reuse, though, you might want to consider generalizing the
code and sticking it into a module.

Didn't see anything about 'do' being deprecated in the perldoc -f do or
perldoc perlsyn docs. Though do SUBROUTINE was stated as deprecated.
The problem (IMHO) with the above is that you can't return a positive
value from the eval and you haven't error checked whether there was an
exception thrown, which is one of the more common reasons to use an eval
in the first place. So I guess my question is, why are you using the
eval to begin with?

my $return = eval { some stuff };
if ($@) {
rethrow or handle "Eval failed with: $@";
}
unless ($return) {
some multiline error handling;
}

The problem is that eval returning a false/undefined value shouldn't
necessarily have to indicate failure. And if the 'eval' isn't catching
an exception then there is really no reason to use it in the first place.

http://danconia.org

That's not entirely true: eval is also useful for delaying execution
(module inclusion, etc.) until runtime.

Checking for $@, though, is a must no matter what you're using it for.

-- jay
--------------------
daggerquill [at] gmail [dot] com
http://www.tuaw.com
http://www.dpguru.com
http://www.engatiki.org

+1 despite my completely missing that omission earlier!

I am sorry, I wasn't clear enough. I am aware of eval returning the last
statements return value, thus the possibility of having an undef result.
I am using it exclusively for control of "do or die" tasks, as DBI
operations for example, where even if it returns 0 it still evalutes
true. The reason I do () an entire block is because I specify additional
error messages. Like this:

my $dsn = join (':', 'DBI',
$cfg->{engine},
join (';', "database=$cfg->{database_name}",
"host=$cfg->{host}",
"port=$cfg->{port}",
),
);

my $dbh;
eval { $dbh = DBI->connect ($dsn, $auth->{user}, $auth->{pass}, {
AutoCommit => 0,
RaiseError => 1,
PrintError => 0,
}
)
}
or do {
$ERROR = "Unable to connect to '$dsn':n" . ($DBI::errstr || 'Unknown error');
return undef;
};

return $dbh;

Thanks for the feedback

Peter

[snip previous round]

But again, you have defeated the purpose of using 'RaiseError' because
you are catching the exception, not doing anything with it, and then
doing your error handling manually. In this case why not just,

my $dbh = DBI->connect($dsn, $auth->{user}, $auth->{pass},
{ AutoCommit => 0,
RaiseError => 0,
PrintError => 0,
}
);
unless ($dbh) {
# error handling....
}
return $dbh;

Why use the eval idiom when you can just shut off DBI's exception throwing?

Or use the eval and raise errors but catch them with the normal $@
variable. aka...

my $dbh = eval { connection stuff, with raise error on };
if ($@) {
# error handling.... error is in $@
return undef;
}
return $dbh;

Either way it seems like an odd idiom to use an eval just to get to a
do, I think you can do that with just a regular blocks (though I didn't
test so maybe not)....

{ my $dbh = .... # with RaiseError off } or { # error handling };

or at least

{ my $dbh = .... # with RaiseError off } or do { # error handling };

Either way there shouldn't be a need for eval unless you have raise
error on, but in that case you should catch the exception with $@.

http://danconia.org

Just as a side note... this is really a stylistic or idiomatic argument.
Syntactically or execution wise there is not anything technically wrong
with the code, that I saw. My argument stems completely from the "*it
seems* this is what The Perl Community/Tim Bunce intended" but that is
merely a guess based on the API, and the docs:

http://search.cpan.org/~timb/DBI-1.48/DBI.pm#RaiseError

And I am really only posing it as "feedback" as you requested rather
than a disagreement. I suspect from a readability/maintenance standpoint
as a fellow developer I would prefer the normal eval/if $@ idiom simply
because I would recognize it more readily than how you have it coded.

Having said that, I use my own wrapper module that throws my own brand
of exception, so I really use none of the idioms :-). But that is merely
a preference and I don't expect other developers to like/dislike it :-).

http://danconia.org

<snip>

Precisely right. That once again is a bad example I guess :) It is just
dictated by the way all the rest of the error handling is done. For
example:

sub check_table {
my ($dbh, $table) = @_;
$table = lc $table;

my $tab_ref;

eval { my $tb_h = $dbh->table_info ();
$tab_ref = $tb_h->fetchall_arrayref ();
}
or do {
$ERROR = "Execution of DBI->table_info() failed:n" . $dbh->errstr;
return undef;
};

unless (grep { ($_->[2] eq $table) and ($_->[3] eq 'TABLE') } @{$tab_ref} ) {
$ERROR = "Table '$table' does not exist";
return undef;
}

my $col_ref;

eval { my $cl_h = $dbh->column_info (undef, undef, $table, '%');
$col_ref = ($cl_h->fetchall_hashref ('COLUMN_NAME') );
}
or do {
$ERROR = "Execution of DBI->column_info() for '$table' failed:n" . $dbh->errstr;
return undef;
};

my @expected_columns = describe_table ($table);

if ( (keys %{$col_ref} != scalar @expected_columns)
or
(grep { ! defined $col_ref->{$_} } @expected_columns) ) {

$ERROR = "Layout of '$table' is invalid";
return undef;
}

return 1;
}

So the connect() subroutine which you saw in the previous mail simply
follows this model, although it is a different animal on the inside.

Peter