When you're initializing your variables, using separate if() blocks is verbose. This is a good place to use a defined-or-expression, a // b, if you have Perl5.10 or later, or a || b for Perl5.8, where 'a' is the default you want to assign if 'a' wasn't specified.
a || b means: if 'a' is true in the boolean sense, use the value 'a', otherwise use 'b'. Boolean false is 'undef', 'zero' and the empty string, everything else is true.. If you legitimately want to pass zero or an empty string as an argument, the Perl5.8 '||' fails, which is why the newer '//' was invented. a // b reads: If 'a' is defined, use 'a', otherwise use 'b'. The Perl5.8 way to achieve that is to use a ternary expression: defined a ? a : b .. if the test in the first chunk is tru, use the value after the question mark '?', otherwise use the chunk after the colon ':'.
The // is often pronounced 'err' cause it's useful for detecting error situations.
#modern way # my $sortby = $cgi->param( 'sortby' ) // 'name'; # Perl5.8 way # my $sortby = $cgi->param( 'sortby' ) || 'name'; # If first value can be zero or empty string .. # my $sortby = ( defined $cgi->param( 'sortby' ) ? $cgi->param( 'sortby' + ) : 'name' ); # alternate arrangement # my $sortby = ( defined $cgi->param( 'sortby' ) ? $cgi->param( 'sortby' ) : 'name' );
With the 'or' or the 'err', declaring and initializing three variables takes three lines, and the repetition reinforces the concept of what's happening. The ternary condition is lengthier, but still 9 lines instead of 15. You don't actually need the parentheses, I use them because emacs will then line up the next line under the opening parenthesis. In this case I think the entire statement could fit on one line. When it's too long, I prefer the alternate arrangement, the components are clearer: if the test is is true, use line 2, else use line 3. Damian Conway's Perl Best Practices recommends the first version, because the test is out front, and the values are stacked. It's definitely better if you are chaining tests:
my $variable = test1() ? value1 : test2() ? value2 : test3() ? value3 : default_value ;
On a slightly different angle, I would suggest grouping variables into a structure. Those three variables are all components of the SQL generation process.
use Readonly; Readonly my %sql_defaults => ( sortby => 'name', ascdesc => 'desc', letter => q{ }, ); my %sql_args; $sql_args{sortby} = $cgi->param( 'sortby' ) || $sql_default{sortby}; $sql_args{ascdesc} = $cgi->param( 'ascdesc' ) || $sql_default{ascdesc} +; $sql_args{letter} = $cgi->param( 'letter' ) || $sql_default{letter}; # alternately, use a suffix 'for' to merge the three lines: # 'qw()' does 'quote words' # $sql_args{$_} = $cgi->param( $_ ) || $sql_default{$_} for qw( sortby ascdesc letter );
The one problem with using hashes or arrays, is you have to pass a reference in the subroutine call, \%sql_args, rather than the plain hash. In the subroutine, that gets assigned to an ordinary scalar variable $sql_args, and you have to use an arrow to access the components, $sql_args->{sortby}.
While suggesting style improvements, I've pointed the way to fixing your bug: assigning from the $cgi->param() routine, rather than from $_. if() tests do not assign to $_.
As for the process of invoking a subroutine, when you call a subroutine with arguments a, b, c, the value of the variable are assigned into the array @_. In the subroutine, you need to assign teh array into individual variables. But you're assigning them to the global variables they came from! You need a my> before the parentheses to declare new local variables which happen to have the same name as th global variables.
As Occam said: Entia non sunt multiplicanda praeter necessitatem.
In reply to Re: passing variables to a sub
by TomDLux
in thread passing variables to a sub
by ag4ve
| For: | Use: | ||
| & | & | ||
| < | < | ||
| > | > | ||
| [ | [ | ||
| ] | ] |