On Tue, Jun 03, 2003 at 08:36:05PM +0200 Kevin Pfeiffer wrote:
> Since it seemed like a nice exercise to work on I played with this some
> myself. Goals being to try to avoid global variables, use subroutines and
> keep MAIN 'uncluttered' and pass arguments to subs as needed.
>
> I think I did okay (holding breath), but I'm wondering about things like:
>
> my @sorted_keys = sort_results(\%score, \%opt);
>
> ## [...]
>
> sub sort_results {
>
> my %scores = %{ (shift) };
> my %opt = %{ (shift) };
>
> ## [...]
>
> }
>
> In the subroutine I'm dereferencing the hash and array and placing the
> values into new variables, correct? Should I instead be working directly on
> the original values (via a reference)? Would that be something like:
>
> sub sort_results {
>
> my ($score_ref, $opt_ref) = @_;
>
> ## etc...
> }
If you from then on referred to elements of the hash with something like
$score_ref->{ key };
then this would be it. It depends on what you want. By dereferencing the
whole data-structure you're essentially creating a copy. The downside of
that is that it needs more memory and is slightly less efficient.
However, that way your subroutine wont have side-effects. If you
directly work on the references, you'd change the hashes in your main
program (only on read-access, of course). That itself can be a valid
design decision. So it's up to you what you want.
> (Thanks for your always helpful advice and comments!)
>
> -K
>
>
> #!/usr/bin/perl
> use warnings;
> use strict;
>
> # from perl.beg (Stuart White)
>
> # desired result:
> # (attempted shots)
> #
> # Marbury 1
> # Jackson 3
> # Hardaway 1
> # Stoudemire 2
> # Bowen 3
> # Duncan 2
> # Williams 1
> # Marion 2
> # Ginobili 1
>
> {
>
> use Getopt::Std;
> my (%opt, $total_attempts);
>
> getopts('hl', \%opt); # commandline options
> my %score = get_data();
> my @sorted_keys = sort_results(\%score, \%opt);
>
> print (format_output([EMAIL PROTECTED], \%score));
>
> }
>
> ## END MAIN ##
>
> ## BEGIN SUBS ##
>
>
> sub get_data {
>
> my %scores;
>
> while (<DATA>) {
>
> if (/.*\] (\w+).*/) { # get player's name
> $scores{$1}++;
> }
>
> }
>
> return %scores;
>
> }
>
>
> sub sort_results {
>
> my %scores = %{ (shift) };
> my %opt = %{ (shift) };
>
> unless (%opt) { # default is sort by name
>
> my @sorted_keys = sort keys %scores ; # sorted by key
Was this branch ever executed so far? You create a lexical variable
inside...of course, @sorted_key wont be visible outside this
unless-block.
> } else {
>
> my $sort_hi if ($opt{h});
What is this statement for?
> my @keys_sorted_by_value = sort {
>
> compare ($scores{$a}, $scores{$b}, $opt{h})
>
> } keys %scores; # sorted by value
> }
>
> }
Ah, I should have read on. Either the unless or the else branch is the
last thing in your subroutine and so the last statement they evaluate is
returned. Hmmh, no. I wouldn't do it like that. It requires too much of
brain-power. ;-) What I often do in such a case is return early from a
sub and thus doing away with an else block. Like this:
sub sort_results {
my %scores = %{ (shift) };
my %opt = %{ (shift) };
if (%opt) {
return sort {
compare($scores{ $a }, $scores{ $b }, $opt{ h })
} keys %scores;
}
return sort keys %scores;
}
First of all, I turned the logic around. What previously was the else
part is now the if part. I use an explicit return there. So the second
return statement is only reached when %opt was false.
> sub compare { # sorts high & low scores
>
> my ($aa, $bb, $sort_hi) = @_;
> ($aa, $bb) = ($bb, $aa) if $sort_hi; # reverse if sort high
> (option h)
>
> return $aa <=> $bb;
>
> }
>
> sub format_output {
>
> my @sorted_keys = @{ (shift) };
> my %score = %{ (shift) };
> my $total_attempts;
> my $output = "\n";
>
> foreach (@sorted_keys) {
>
> $output .= sprintf "%12s %2s\n", $_, $score{$_};
> $total_attempts += $score{$_};
>
> }
>
> $output .= "================\n";
> $output .= "TOTALS: $total_attempts\n\n";
>
> }
The rest looks fine to me. However - and so turning back to your
original question - for this program you are probably best off by
directly working with the references. sort_results() would then look
like:
sub sort_results {
my ($scores, $opt) = @_;
if (%$opt) {
return sort {
compare($scores->{$a}, $scores->{$b}, $opt->{h})
} keys %$scores;
return sort keys %$scores;
}
By not creating a copy of your hashes, you save a little on memory and
CPU-cycles.
Tassilo
--
$_=q#",}])!JAPH!qq(tsuJ[{@"tnirp}3..0}_$;//::niam/s~=)]3[))_$-3(rellac(=_$({
pam{rekcahbus})(rekcah{lrePbus})(lreP{rehtonabus})!JAPH!qq(rehtona{tsuJbus#;
$_=reverse,s+(?<=sub).+q#q!'"qq.\t$&."'!#+sexisexiixesixeseg;y~\n~~dddd;eval
--
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
