I was asked to use as less modules as possible, since the code is compiled and turned out to be a big file. Also dependencies issue always around.....this is why I invented the sub from scratch...but thanks anyway Chanan
-----Original Message----- From: [email protected] [mailto:[email protected]] On Behalf Of sawyer x Sent: Sunday, January 18, 2009 12:41 PM To: Perl in Israel Subject: Re: [Israel.pm] FW: defined function It's worth noting that there are a lot of modules that help you do debugging and log file output very efficiently and modularly. Check CPAN, it really is amazing. On Sun, Jan 18, 2009 at 12:33 PM, bc.other <[email protected]> wrote: > Hi, > > Thanks for your remarks, this sub is used internally by me, and not by > others. > This is why I checked the values against 1 and 0 and not to any "other > value" > > The idea was: > RaiseError will cause the program to exit using the exit code, > PrintError will cause the print of the message into STDOUT > > This way I can use it to just send a warnings, which I should have changed > the "Error:" inot "Warn:" (in the print out) > Or just use it to send Error message, and allow the script to continue / or > not - depends on the user decisions > The default values, says print the error and exit using exit code. > > As for sending the exist code any other time then numeric value - is not an > issue - since I know for sure the user will > Want a numeric answer coming back. > > Thanks > Chanan > > > -----Original Message----- > From: [email protected] [mailto:[email protected]] On Behalf > Of sawyer x > Sent: Sunday, January 18, 2009 12:08 PM > To: Perl in Israel > Subject: Re: [Israel.pm] FW: defined function > > - 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. > _______________________________________________ > Perl mailing list > [email protected] > http://perl.org.il/mailman/listinfo/perl > > _______________________________________________ > Perl mailing list > [email protected] > http://perl.org.il/mailman/listinfo/perl > _______________________________________________ Perl mailing list [email protected] http://perl.org.il/mailman/listinfo/perl _______________________________________________ Perl mailing list [email protected] http://perl.org.il/mailman/listinfo/perl
