2

[EDIT] - with the benefit of hindsight, this question was misdirected. I have not deleted it because it is a good example of the incorrect use of eval and correct criticism by Perl::Critic.

Perl Critic raises the following criticism for the code below:

Return value of eval not tested. You can't depend upon the value of $@/$EVAL_ERROR to tell whether an eval failed

my $Jet = Win32::OLE->CreateObject('DAO.DBEngine.36')
    or croak "Can't create Jet database engine.";
my $DB = $Jet->OpenDatabase($DBFile)

# code omitted for the sake of brevity
# perl script writes results to Access db via an append query
$DB->Execute( $SQLquery, 128 );                       #128=DBFailOnError

eval {$err = Win32::OLE->LastError()} ; #<<<< PROBLEM LINE SEE FEEDBACK BELOW  
if ( $err){
     print $ERROR "WIN32::OLE raised an exception: $err\n";
     Win32::OLE->LastError(0);  # this clears your error
}

My thinking is that I am using eval to detect the existence of the error object and on the Win32:OLE module to detects the error and reports it.

Am I safe to ignore the criticism?

heferav
  • 759
  • 5
  • 11
  • 2
    I have no idea what the point of the "eval" is. –  Oct 16 '09 at 09:40
  • Specifically, `LastError` is not going to throw. – Sinan Ünür Oct 16 '09 at 11:19
  • 1
    If you're doing something that Perl::Critic complains about, you're probably doing it wrong. In this case, since you don't do what it says, it's tellin gyou that your eval is pointless. – brian d foy Oct 16 '09 at 13:43
  • brian, I have edited my example to make it clear "what I am doing". My experience is that Win32::OLE has some problems in trapping all WIN32 errors, hence the eval statement is trying to catch these exceptions. – heferav Oct 16 '09 at 14:12
  • 1
    @heferav My suspicion is that you haven't read the docs and you are not aware of `$Win32::OLE::Warn`. Second, even if you are correct, you do not need eval when calling `LastError` because `LastError` does not throw. – Sinan Ünür Oct 16 '09 at 14:25
  • Sinan, I have read the documentation again and now understand what you are saying. I have accepted your answer below. Thank you for your advice. – heferav Oct 16 '09 at 15:59
  • I see no utility in attaching the ms-access tag to this question, as the db engine is completely peripheral to the matter at hand. DAO/COM or Jet/ACE might be appropriate tags, but ms-access is not involved in any way, shape or form. – David-W-Fenton Oct 17 '09 at 01:52
  • David, I have removed the ms-access tag – heferav Oct 17 '09 at 08:30

2 Answers2

3

Leaving aside the perl-critic issuse, your code does not make much sense.

The Win32::OLE docs explain when exceptions will be thrown (and how you can automatically catch them).

LastError just gives you information about an error after it has occurred assuming your program has not died. Wrapping it in eval is pointless.

Update: I would have written something along the following lines (untested because I am on Linux with no access to Windows right now):

use strict;
use warnings;

use Carp;

use Win32;
use Win32::OLE;

$Win32::OLE::Warn = 3;

# No need for this eval if you are OK with the default error message    
my $Jet = eval {
    Win32::OLE->CreateObject('DAO.DBEngine.36')
} or croak sprintf(
    "Can't create Jet database engine: %s", 
    win32_error_message(Win32::OLE->LastError)
);

# No need for this eval if you are OK with the default error message
my $DB = eval {
    $Jet->OpenDatabase($DBFile)
} or croak sprintf(
    "Can't open database '$DBFile': %s",
    win32_error_message(Win32::OLE->LastError)
);

my $result = eval {
    $DB->Execute( $SQLquery, 128 )
};

unless (defined $result) {
    print $ERROR win32_error_message(Win32::OLE->LastError);
    Win32::OLE->LastError(0);
}
Sinan Ünür
  • 116,958
  • 15
  • 196
  • 339
  • Sinan, I read the Win32::OLE docs when I wrote the code. The error trapping is problematic because of time delays in errors being reported by MS Access to the Win32 API, hence deficiencies in the Win32 API are also reflected in the Win32::OLE perl module – heferav Oct 16 '09 at 14:21
  • How does that change the fact that `LastError` does not throw? There is no need to wrap it in `eval`! Just report it if it returns something. Of course, you need to make sure to clear it before you call any routines because I think the value is only valid following an actual failure and is not cleared in case of success. Incidentally, your down vote is unjustified. – Sinan Ünür Oct 16 '09 at 14:28
  • Sinan, I accept that your solution above is superior. On a small number of occations $DB->Execute caused an exception after a long delay i.e after the error object had been tested and found to be OK. I will try your code. Thanks again. – heferav Oct 16 '09 at 16:06
  • @heferav That might be. I do not have much experience with accessing Access using `Win32::OLE`. However, I do not see if there is anything you can do about that. In any case, wrapping a method that does not throw in an `eval` block doesn't really do anything. – Sinan Ünür Oct 16 '09 at 16:53
2

The meaning of that message is detailed in the documentation. In short, it tells you to not rely on $@ alone after an eval, but to also check the return value of eval.

However, in your case, the problem is that you aren't checking either the return value of eval nor are you checking $@ and moreover it seems that your use of eval is completely superfluous because the method you are calling shouldn't be throwing any exceptions.

innaM
  • 47,505
  • 4
  • 67
  • 87