[Israel.pm] FW: defined function

sawyer x xsawyerx at gmail.com
Sun Jan 18 02:08:11 PST 2009


- Obviously, you should use "defined" instead of not at all. That's an
easy question.
- Also, with hashes you should use "exists" to check if there is such
a key. Otherwise, you would get a warning, because it's tried to
access a value of a key that doesn't exist. that's obviously a
problem, don't you think? :)
- The reason why use strict && use warnings did not prevent the script
from running it is because it's a runtime error, and not compile time.
strict (or warnings) would suffice with warning you while running it
(during runtime). You should be able to use judgment, "exists" and
"defined" to avoid such problems.
- when sending a hash to a function, it's better - for health reasons
- to send a hash reference instead. To create an anonymous hash
reference, use {}. Such as { printError => 0 }.

I had only a few comments on the subroutine you pasted:
- I would write it a bit cleaner: no if()s at the end, using "exists"
for hash values, etc.
- Wouldn't you want RaiseError to print the error as well? Or just exit?
- Simple design problem:
You take into account that perhaps someone wouldn't send an exit
error, but then if they send a hash, it will be create havoc with the
parameters in the subroutine. That's a good reason why you would use
references. That way, if a person does: printWarningError("msg", {
RaiseError => 1 }), it will pass the hash reference to $lv_ec.
However, you'll have to check if $lv_ec is actually a hash reference
instead of an exit status. So, instead, I would suggest to put the
exist status in a hash. You could also put the error msg in the hash,
but you probably will definitely have a msg, so you don't have to put
it in the hash. That way, it would look like this:
printWarningError( "my msg", { RaiseError => 1, ExitStatus => 0 } );

The way I would revise your subroutine, with your permission:
sub printWarningError {
    my ( $lv_msg, $lv_args_href ) = @_;  # message to print
    $lv_msg || return 0;

    # defualt values
    if ( ! exists $lv_args_href{'ExitStatus'} ) {
        $lv_args_href{'ExitStatus'} = 0;
    }

    foreach my $arg ( qw( PrintError RaiseError ) ) {
        if ( ! exists $lv_args_href{$arg} ) {
            $lv_args_href{$arg} = 1;
        }
    }

    # this is actually better than writing
    # if ( !exists $lv_args_href{'PrintError'} ) {
$lv_args_href{'PrintError'} = 1 }
    # if ( !exists $lv_args_href{'RaiseError'} ) {
$lv_args_href{'RaiseError'} = 1 }
    #
    # because it provides less duplication, and makes it easier to make changes

    chomp $lv_msg;
    if ( $lv_args_href{'PrintError'} ) {
        print "Error: $lv_msg\n";
    }
    if ( $lv_args_href{'RaiseError'} ) {
        exit $lv_args_href{'ExitStatus'};
    }
    return $lv_args_href{'ExitStatus'};
}

printWarningError( "my msg", { PrintError => 0, ExitStatus => 0 } );
----
It seems longer (of course, notice I put a whole block of comments to
explain why I used 5 lines - include end statements - instead of 2),
but it's much more expressive, and allows to expand your code much
more. It's also more dynamic and checks better whether something is
defined or not.



More information about the Perl mailing list