Hi Chris,
in addition to the good advice that Uri gave you - some comments on your code.
On Sun, 4 Dec 2011 07:40:09 -0600
Chris Stinemetz <chrisstinem...@gmail.com> wrote:
> I have a program that I am working on improveing. The fist step I have
> taken is converting it in using the strict pragma.
>
> Now when this subroutine call is made I get the following compilation error:
>
> Global symbol "$cell" requires explicit package name at ./evdo.pl line 279.
> Global symbol "$cell" requires explicit package name at ./evdo.pl line 279.
>
> I understand lexical scope, but am having a hard time figuring out how to
> fix this.
>
> Any help is greatly apprciated.
>
> Chris
>
> sub getMarket
> {
> my $_cell = shift;
> my $_mkt = "";
> my $mkt = "";
> foreach $mkt (keys %marketInfo)
> {
This is better written using «foreach my $mkt» instead of «foreach $mkt»
> my $start = $marketInfo{$mkt}->{"start"};
You should assign $marketInfo{$mkt} to a variable, or alternatively do:
my ($start, $end) = @{$marketInfo{$mkt}}{qw(start end)};
> my $end = $marketInfo{$mkt}->{"end"};
> if( $cell >= $start && $cell <= $end)
> {
> $_mkt = $mkt;
> last;
> }
> }
This loop can be more succinctly expressed using first from List::Util :
return List::Util::first {
my ($start, $end) = @{$marketInfo{$_}}{qw(start end)};
$cell >= $start && $cell <= $end;
} keys(%marketInfo);
All that put aside, this lookup has O(N) complexity, and you can reduce it to
O(log(N)) by creating a sorted array of ranges that you will binary search.
> return $_mkt;
> }
>
>
> This is the foreach loop that calls the subroutine
>
> foreach my $c_s (sort num keys %evdo)
> {
> my $market = $c_s;
Why are you using both $c_s and $market to iterate over the loop? The «my $c_s»
is already lexical and safe.
> my $satt = $evdo{$market}{$pegs[0]};
> my $sest = $evdo{$market}{$pegs[1]};
> my $fit = $evdo{$market}{$pegs[2]};
> my $psEst = $evdo{$market}{$pegs[3]};
> my $catt = $evdo{$market}{$pegs[4]};
> my $cest = $evdo{$market}{$pegs[5]};
> my $pcEst = $evdo{$market}{$pegs[6]};
> my $rfLost = $evdo{$market}{$pegs[7]};
> my $cpDropCell = $evdo{$market}{$pegs[8]};
> my $cpDropRnc = $evdo{$market}{$pegs[9]};
> my $tuneAway = $evdo{$market}{$pegs[10]};
> my $tDrops = $evdo{$market}{$pegs[11]};
> my $pDcr = $evdo{$market}{$pegs[12]};
> my $ia = $evdo{$market}{$pegs[13]};
> my $pIa = $evdo{$market}{$pegs[14]};
> my $tccf = $evdo{$market}{$pegs[15]};
> my $failAp = $evdo{$market}{$pegs[16]};
> my $failTp = $evdo{$market}{$pegs[17]};
> my $failA10 = $evdo{$market}{$pegs[18]};
> my $failAAA = $evdo{$market}{$pegs[19]};
> my $failPDSN = $evdo{$market}{$pegs[20]};
Wow! Duplicate code central! And it's also susceptible to index mismatch.
What I suggest you to do would be to have the $evdo{$market} be an object and
then you can work on its fields/slots and methods. See:
http://perl-begin.org/topics/object-oriented/
> my $cell = substr($market,0,index($market,"_"));
This can be more idiomatically (and more briefly) done using:
(my $cell = $market) =~ s/_.*//s;
> my $mkt = &getMarket($cell);
Don't use leading ampersands in subroutine calls:
* http://perl-begin.org/tutorials/bad-elements/#ampersand-in-subroutine-calls
* https://www.socialtext.net/perl5/subroutines_called_with_the_ampersand (Page
requires JavaScript).
Regards,
Shlomi Fish
--
-----------------------------------------------------------------
Shlomi Fish http://www.shlomifish.org/
Chuck Norris/etc. Facts - http://www.shlomifish.org/humour/bits/facts/
Chuck Norris does not code; when he sits at a computer, it just does whatever
he wants. (By: Kattana.)
Please reply to list if it's a mailing list post - http://shlom.in/reply .
--
To unsubscribe, e-mail: beginners-unsubscr...@perl.org
For additional commands, e-mail: beginners-h...@perl.org
http://learn.perl.org/
