Quoth pgodfrin <
[email protected]>:
Don't declare variables up-front like this. Perl isn't C or Pascal.
Declare them as you first need them.
Your code will be much easier to follow if you indent properly.
If you had used
if (my ($e, $v) = ...) {
as I originally had, $e and $v would be scoped to the 'if' statement. It
is always best to give variables as small a scope as you can.
To expand these two a little more (note the use of /x, which allows
whitespace and comments in the 'pattern' part):
s/ # replace...
(?<! \\ ) # not immediately after a backslash
# (this prevents '\$foo' from being replaced)
\$ # a dollar sign
(\w+) # an identifier, which will be captured into $1
/$ENV{$1}/gx; # ...with the appropriate env var
s/ # replace...
\\ # a backslash, followed by
(.) # any character (captured into $1)
/$1/gx; # ...with that character (so \\->\, \$->$, etc.)
Decent indentation makes comments like this redundant.
Since you don't check the return value of close (not usually useful on
filehandles opened for reading, anyway), there's no point in explicitly
calling it. Just let the variable go out of scope, and Perl will close
it for you.
There's no need for a return value like this that doesn't convey any
information to the caller. Falling off the end of a sub is a perfectly
respectable way to return (the return value will in fact be the value of
the last statement executed, but since you don't check it anyway that
doesn't matter). If you do want a 'null' return, a simple
return;
will return undef in scalar context and the empty list in list context,
which is usually appropriate.
I would recommend against using Env, unless you're a shell programmer
trying hard not to really learn Perl (which is OK, of course, but don't
expect a lot of sympathy from the people here
). Importing an
unspecified list of variables into your namespace just for the sake of
avoiding a hash lookup is not a good tradeoff. (Shell should be avoided
for much the same reason, neat though the idea is.) The fact that you
have to turn off 'once' warnings should alert you to the fact this is
not necessarily a good idea, though there are of course occasions where
turning off warnings is the right answer.
You will find it makes your life easier later if you use explicit import
lists where practical. When you come to this code later, it makes it
much simpler to work out where setrun is defined if you can see it in
the 'use' line.
use Mymod qw/setrun/;
Also, I presume this is just an example, but you should *really* choose
a better name than 'Mymod'. I tend to use BMORROW::* for my own private
modules, so I'd call this something like BMORROW::SetEnv, which means it
would need to live in a file called SetEnv.pm in a directory BMORROW
somewhere in @INC.
If you set $\ (or use the -l switch on the #! line) Perl will add those
newlines for you. If you have 5.10 you can use 'say' instead of 'print'
for the same effect. Also, several lines of print statements usually
work better as a heredoc:
print <<ENV;
TMPTEST: $ENV{TMPTEST}
TMPTOO: $ENV{TMPTOO}
Now with scalar vars...
TMPTEST: $TMPTEST
ENV
See how much easier it is to get things lined up when you can see how
they will appear?
You do realise Env->import (which is subtly but importantly different
from Env::import) has already been called, at compile time, by the 'use'
statement? If it hadn't, the next two lines would never have got past
'use strict'.
Again, there's no need to explicitly exit unless you want to do so
early. Falling off the end of the program is a perfectly good way out.
Ben