Re: Help tightening up a subroutine please

[gaal]

First, grep goes over the whole list. If you only need to stop at the first successful match, don't use it. You can use List::Util::first instead.

Other than that, just a blitz look.

>  while ($i < $num) {
>     $sortedKeys[$i] = $i;
>     $i++;
>  }

This can be written simply as

@sortedKeys = (0 .. $num - 1);

>  next unless defined %{$matches{$fasta_id}};
>  last unless defined @{$matches{$fasta_id}{$site}};

These may not always be true, but they autovivify an element so *will* be true the second time over you test them with the same key.

>  # as @fastarray is global, this will have global effect, and we do 
+not need to return it.
>
>  @fastarray=@newfast;

If the array is big, you'd do much better just updating a reference to it rather than the above, as you are copying all the elements. Avoiding globals wherever you can is generally a good idea...

Update: Most of my points contained mistakes: see below for corrections by kyle++ and BrowserUk++. That'd teach me to post when tired. The advice against globals is probably sound though. :-)

[kyle]

These may not always be true, but they autovivify an element so *will* be true the second time over you test them with the same key.

I don't think that's true.

my %x;

print "one\n" if defined %{$x{a}};
print "two\n" if defined %{$x{a}};

print Dumper( \%x );

It does autovivify, but it does so to a value that's still not defined (when tested that way, anyway). Full output:

$VAR1 = {
          'a' => {}
        };

UPDATE: I should add that I do not think this is a good way to do this check. It would be better to check that the key exists and then, if you need to know that there's something in there, dereference the array and check that.

[gaal]

Huh. You sent me to the manual since this greatly surprised me. FWIW you're right—thanks for the correction—but some of what I did remember does hold:
Use of "defined" on aggregates (hashes and arrays) is deprecated. It used to report whether memory for that aggregate has ever been allocated. This behavior may disappear in future versions of Perl. You should instead use a simple test for size:

       if (@an_array) { print "has array elements\n" }
       if (%a_hash)   { print "has hash members\n"   }

When used on a hash element, it tells you whether the value is defined, not whether the key exists in the hash. Use "exists" for the latter purpose.

[ikegami]

Update: Bah, I should head my own advice. The second statement never gets a chance to execute.

You're looking at the statements in isolation. Look at them as a pair.

Original post: (WRONG!)

# First time
next unless defined %{$matches{$fasta_id}};         # False
last unless defined @{$matches{$fasta_id}{$site}};  # False

# Second time
next unless defined %{$matches{$fasta_id}};         # Now true!!
last unless defined @{$matches{$fasta_id}{$site}};

What really happens:

# First time
next unless defined %{$matches{$fasta_id}};         # False -> next
last unless defined @{$matches{$fasta_id}{$site}};  # Skipped by next

# Second time
next unless defined %{$matches{$fasta_id}};         # Still false
last unless defined @{$matches{$fasta_id}{$site}};

[BrowserUk]

First, grep goes over the whole list. If you only need to stop at the first successful match, don't use it. You can use List::Util::first instead.

For this particular application, a bandpass filter rather than a low pass filter, List::Util::first is no substitute for grep.

( Actually, first() isn't even a low pass filter as it returns a single value not a filtered list but...)