Looking for a critique

[samwyse]

This code is working, but I'd like to make it "better". In particular,
I'm dissatisfied with my method of deciding if the end-user has
ActiveState Perl installed. I'd also like to suppress the "BEGIN
failed--compilation aborted" message when the "die" executes. Any
suggestions? Thanks!

#!/usr/bin/perl

BEGIN {
eval "use DBI";
if ($@) {
my @cf_email;
eval "use Config";
@cf_email = Config::config_re("cf_email") unless $@;
if (join(' ', @cf_email) =~ /\bActiveState.com\b/) {
die <<__ActiveState__;

You need to install the Perl DBI and DBD::ODBC modules.
To do so, try using these commands while connected to the Internet:
ppm install DBI
ppm install DBD-ODBC

__ActiveState__
} else {
die <<__CPAN__;

You need to install the Perl DBI and DBD::ODBC modules.
To do so, try using this command while connected to the Internet:
cpan DBI DBD::ODBC
Note that the CPAN command frequently requires that a C compiler be
installed on your computer.

__CPAN__
}
}
}

__END__

 

[Uri Guttman]

s> This code is working, but I'd like to make it "better". In particular,
s> I'm dissatisfied with my method of deciding if the end-user has
s> ActiveState Perl installed. I'd also like to suppress the "BEGIN
s> failed--compilation aborted" message when the "die" executes. Any
s> suggestions? Thanks!

s> #!/usr/bin/perl

s> BEGIN {

why are you doing this in a begin block? the only reason you would need
it in a begin is if this code did some exporting. but the dbi module is
pure OO so you can simplify this greatly.
s> eval "use DBI";

don't use string eval when you really just need this:

eval { require DBI ; } ;

s> if ($@) {

you can merge the last two into:

unless( eval { require DBI ; } ) {


s> my @cf_email;

why the predeclare? actually, it isn't even needed.

s> eval "use Config";

config is core module. if it dies, it is more important to tell them
than the dbi stuff. so just require it here and let it die on its
own. if you want to trap it, use eval block as before

require Config ;

s> @cf_email = Config::config_re("cf_email") unless $@;
s> if (join(' ', @cf_email) =~ /\bActiveState.com\b/) {

why the join and then the regex? just grep the list or use first() from
Utils::List. and you can merge the die as well:

die <<DIE if grep /\bActiveState.com\b/, Config::config_re("cf_email") ;
blah

DIE

another variation is

grep /\bActiveState.com\b/, Config::config_re("cf_email") or
die <<DIE ;
blah

DIE

i might have flipped the booleans but i leave that as an exercise for
the OP.

s> die <<__ActiveState__;

use a simpler here doc token and it should be all caps and without the __

s> } else {

why do an else after you just died in the then? and since we removed the
whole then block by using simple boolean stuff, we don't need this else
block either. just die, dammit!

s> die <<__CPAN__;

drop the __. and my style is to use DIE rather then a message specific
token. it is a die string. and i often use here docs in die/warn calls.

uri

 

[Samwyse]

s> This code is working, but I'd like to make it "better". In particular,
s> I'm dissatisfied with my method of deciding if the end-user has
s> ActiveState Perl installed. I'd also like to suppress the "BEGIN
s> failed--compilation aborted" message when the "die" executes. Any
s> suggestions? Thanks!

s> #!/usr/bin/perl

s> BEGIN {

why are you doing this in a begin block? the only reason you would need
it in a begin is if this code did some exporting. but the dbi module is
pure OO so you can simplify this greatly.

Much of my code was borrowed from recipe 12.2 of O'Reilly's "Perl
Cookbook", which I blindly trusted. It advises "You don't want to use
eval { BLOCK }, because this only traps run-time exceptions and use is a
compile-time event. Instead, you must use eval "string", to catch
compile-time problems as well." and "We wrap the eval in a BEGIN block
to ensure the module-loading happens at compile time instead of run time."

[...]

I've elided a big bit for which the above note also applies, however in
the middle was some useful advice about using grep. Thanks!

s> } else {

why do an else after you just died in the then? and since we removed the
whole then block by using simple boolean stuff, we don't need this else
block either. just die, dammit!

In case it isn't obvious, this code is going to be handed out to people
without much Perl experience. What isn't obvious is that some of them
will be in the process of learning Perl and might be using my script as
example code. For that reason, I tried to write it in an "anti-golf"
style. Most of the time I do use the "open or die" and "die unless"
idioms, but in this case I wanted to emphasize the fact that the script
would die whichever path was taken; thus the symmetric "if (...) {die
'this'}else{die 'that'}" approach.

s> die <<__CPAN__;

drop the __. and my style is to use DIE rather then a message specific
token. it is a die string. and i often use here docs in die/warn calls.

I've gotten burned in the past using a single token for multiple here
docs, so I tend to make them relevant to the task at hand. The __ is a
bit of an affectation which I adopted because of it's similarity to
__END__ and __DATA__. Both of those signal the end of a section of a
file and the end of a here doc is definitely encompassed by that definition.

 

[Uri Guttman]

s> I'm dissatisfied with my method of deciding if the end-user has
s> ActiveState Perl installed. I'd also like to suppress the "BEGIN
s> failed--compilation aborted" message when the "die" executes. Any
s> suggestions? Thanks!
s> #!/usr/bin/perl
s> BEGIN {
S> Much of my code was borrowed from recipe 12.2 of O'Reilly's "Perl
S> Cookbook", which I blindly trusted. It advises "You don't want to use
S> eval { BLOCK }, because this only traps run-time exceptions and use is
S> a compile-time event. Instead, you must use eval "string", to catch
S> compile-time problems as well." and "We wrap the eval in a BEGIN block
S> to ensure the module-loading happens at compile time instead of run
S> time."

that seems to not address the real issue. the main difference between
require and use is calling the import method of the loaded class so it
can install stuff into the current namespace. if you aren't doing any
importing (as is typical with using pure OO module), there is no benefit
of use over require. and the compiletime vs runtime issue is also moot
in that case. in this case there are only runtime issues (did the module
load correctly or not) so block eval is fine for that. even with their
view, string eval of "use foo" executed inside a BEGIN block is no
different than block eval of the same code. the 'foo' part is hardwired
in both cases so there is no win for the string eval. now if the module
name is determined at runtime, then calling eval "require $mod" is the
winner since it will work with a namespace bareword (Foo::Bar) and be
portable to all platforms. the runtime form of require only works with
real file names and so you would have to convert Foo::Bar to Foo/Bar.pm
and do that portably which is a pain.

hopefully that clears things up.

and your comment on blindly trusting the cookbook bothers me. never copy
code that you don't understand. the cookbook is great for ideas and some
code examples. i am not overly fond of the actual code in there (but i
am very picky about code quality) and when i have used ideas from there,
i always write my code from scratch.

s> } else {
S> In case it isn't obvious, this code is going to be handed out to
S> people without much Perl experience. What isn't obvious is that some
S> of them will be in the process of learning Perl and might be using my
S> script as example code. For that reason, I tried to write it in an
S> "anti-golf" style. Most of the time I do use the "open or die" and
S> "die unless" idioms, but in this case I wanted to emphasize the fact
S> that the script would die whichever path was taken; thus the symmetric
S> "if (...) {die 'this'}else{die 'that'}" approach.

we aren't talking golf here. concise perl can be very readable. overly
verbose code can actually be harder to read.


s> die <<__CPAN__;
S> I've gotten burned in the past using a single token for multiple here
S> docs, so I tend to make them relevant to the task at hand. The __ is
S> a bit of an affectation which I adopted because of it's similarity to
S> __END__ and __DATA__. Both of those signal the end of a section of a
S> file and the end of a here doc is definitely encompassed by that
S> definition.

the similarity to __DATA__ is actually a reason to not use __. as for
getting burned with reused here doc tokens, that is more a lesson for
you on coding discipline and not one on picking harder to find/scan here
doc tokens. you can change the token around but that is also
inconsistant. you have to choose the place where you want consistancy
and choosing to reuse token names is a better place IMO. but what do i
know?

uri

 

[Samwyse]

s> I'm dissatisfied with my method of deciding if the end-user has
s> ActiveState Perl installed. I'd also like to suppress the "BEGIN
s> failed--compilation aborted" message when the "die" executes. Any
s> suggestions? Thanks!
s> #!/usr/bin/perl
s> BEGIN {

S> Much of my code was borrowed from recipe 12.2 of O'Reilly's "Perl
S> Cookbook", which I blindly trusted. It advises "You don't want to use
S> eval { BLOCK }, because this only traps run-time exceptions and use is
S> a compile-time event. Instead, you must use eval "string", to catch
S> compile-time problems as well." and "We wrap the eval in a BEGIN block
S> to ensure the module-loading happens at compile time instead of run
S> time."

that seems to not address the real issue. the main difference between
require and use is calling the import method of the loaded class so it
can install stuff into the current namespace. if you aren't doing any
importing (as is typical with using pure OO module), there is no benefit
of use over require. and the compiletime vs runtime issue is also moot
in that case. in this case there are only runtime issues (did the module
load correctly or not) so block eval is fine for that. even with their
view, string eval of "use foo" executed inside a BEGIN block is no
different than block eval of the same code. the 'foo' part is hardwired
in both cases so there is no win for the string eval.

There's another big difference between require and use. The former is a
run-time event, the latter is a compile-time event. (Although what with
BEGIN blocks and AUTOLOADER, the difference is becoming blurier all the
time.) I suppose that we'll both have to write test cases, but the
point that the book seemed to be making is that in this code:
eval { use DBI };
the "use" will apparently be executed when the eval is compiled, not
when it is executed, defeating the purpose of using an eval. (I suppose
that this behavior could be version dependent, and may have changed
since the book was written.)

hopefully that clears things up.

Clear as mud.

 

[samwyse]

There's another big difference between require and use. The former is a
run-time event, the latter is a compile-time event. (Although what with
BEGIN blocks and AUTOLOADER, the difference is becoming blurier all the
time.) I suppose that we'll both have to write test cases, but the
point that the book seemed to be making is that in this code:
eval { use DBI };
the "use" will apparently be executed when the eval is compiled, not
when it is executed, defeating the purpose of using an eval.

Now that I'm at work, I've checked.

eval {use DBI}; => fails before eval executes, my handler never runs
eval 'use DBI'; => fails at runtime, my handler does run
eval {require DBI}; => also works, but doesn't run import

Based on these results (and a few of your other comments), I'm now
using this:

use Config;
eval { require DBI } or
die (scalar(grep /\bActiveState\b/, Config::config_re("cf_email")) ?
<<DIE : <<DIE);

You need to install the Perl DBI and DBD::Oracle modules.
To do so, try using these commands while connected to the Internet:
ppm install DBI
ppm install DBD-Oracle

DIE

You need to install the Perl DBI and DBD::Oracle modules.
To do so, try using this command while connected to the Internet:
cpan DBI DBD::Oracle
Note that the CPAN command frequently requires that a C compiler be
installed on your computer.

DIE


I'm still looking for a better test than scanning the cf_email config
variable.

 

[Uri Guttman]

S> There's another big difference between require and use. The former is
S> a run-time event, the latter is a compile-time event. (Although what
S> with BEGIN blocks and AUTOLOADER, the difference is becoming blurier
S> all the time.) I suppose that we'll both have to write test cases,

the diff is always blurry as you can control when they get called. i
think i didn't miss that point at all but you could read it differently
than what i wrote. anyhow, my main point is that the cookbook is very
misleading in that code and text about block/string eval and use.

S> but the point that the book seemed to be making is that in this code:
S> eval { use DBI };
S> the "use" will apparently be executed when the eval is compiled, not
S> when it is executed, defeating the purpose of using an eval. (I
S> suppose that this behavior could be version dependent, and may have
S> changed since the book was written.)

ok, let's try one more time. block and string eval have NOTHING to do
with each other except for the poorly overloaded name (which larry
regrets and is fixed in p6). block eval is a try/catch thing and string
eval is a compile and run this code string. so your comment about
defeating the purpose of eval by choosing block eval is nonsense. they
aren't replacements for each other at all. they are completely different
functions with very different behavior. they just have a shared name.

the behavior of both has not changed since they were put into perl so
that point is moot too.

S> Clear as mud.

you have to learn more about why the evals are different and why you use
one and not the other. string eval is very dangerous and can cause all
sorts of nasty hard to find bugs. it is also too powerful for most of
the silly little things newbies use it for. and it is slower as it
compiles code each time it is called. block eval is very simple and can't
cause tricky bugs. it is just used to catch die's and fatal errors in
the code in its block.

the use DBI thing was just to catch the error of not finding DBI and
nothing more. so using string eval for that is a waste of cpu,
misleading, and just wrong code IMO. it is something i can see tom
christiansen doing and not caring about what others think about it. i do
care and i consider that code to be very poor. block eval is the correct
thing to do there as you just are trying to catch the exception of use
failing. and since the DBI module is *OO*, then require is a better
choice than use. and the whole thing doesn't need to be in a BEGIN block
since no importing (has to be done at compile time) is needed. so why do
string eval when it isn't needed for all those reasons? don't argue what
the book said, think of reasons yourself and defend your position of
using string eval in that exact case (with DBI).

uri

 

[Uri Guttman]

s> Now that I'm at work, I've checked.

s> eval {use DBI}; => fails before eval executes, my handler never runs

that fails because the use line executes immediately before the block eval is
even run so it can't catch the exception.

s> eval 'use DBI'; => fails at runtime, my handler does run

pretty much the same as the book code.

s> eval {require DBI}; => also works, but doesn't run import

but you don't need import! and you can always call it yourself (but only
if you need it):

eval {require DBI ; DBI->import() };


s> Based on these results (and a few of your other comments), I'm now
s> using this:

s> use Config;
s> eval { require DBI } or
s> die (scalar(grep /\bActiveState\b/, Config::config_re("cf_email")) ?
s> <<DIE : <<DIE);

s> You need to install the Perl DBI and DBD::Oracle modules.
s> To do so, try using these commands while connected to the Internet:
s> ppm install DBI
s> ppm install DBD-Oracle

s> DIE

s> You need to install the Perl DBI and DBD::Oracle modules.
s> To do so, try using this command while connected to the Internet:
s> cpan DBI DBD::Oracle
s> Note that the CPAN command frequently requires that a C compiler be
s> installed on your computer.

s> DIE

overall much better code.

s> I'm still looking for a better test than scanning the cf_email config
s> variable.

look into List::Utils::first.

but you are scanning that to see if the perl is activestate? there must
be some config variable that tells you that and is not an array. run
perl -V and look around and read perldoc Config.

uri

 

[Uri Guttman]

S> Uri Guttman wrote:

S> You'd think so, wouldn't you? Nothing that you can get from Config
S> tells you, except for what you can assume from an email
S> address. Running "perl -V" does tell you (in the form of an entry in
S> the list of applied patches), but forking a second copy of Perl seems
S> a bit inelegant.

perl -V just prints stuff it has from config.

and there are other ways to tell you are running under winblows. the
simplest is checking $^O which is the OS (see perlvar). i just check it
with $^O =~ /win32/i.

uri

 

[Samwyse]

S> Uri Guttman wrote:

S> You'd think so, wouldn't you? Nothing that you can get from Config
S> tells you, except for what you can assume from an email
S> address. Running "perl -V" does tell you (in the form of an entry in
S> the list of applied patches), but forking a second copy of Perl seems
S> a bit inelegant.

perl -V just prints stuff it has from config.

Really? I checked them both and found several things in the output of
'perl -V' that are (apparently) not be be found in Config. ActiveState
Perl betrays its heritage several places in 'perl -V', but (repeating
myself) in only one place in Config.

and there are other ways to tell you are running under winblows. the
simplest is checking $^O which is the OS (see perlvar). i just check it
with $^O =~ /win32/i.

I have several Perl interpreters laying around for which that check
succeeds, yet none of them were produced by ActiveState and so lack an
associated 'ppm' command.