Converting code to perl sub, but not sure I'm doing it right

Question

I'm working from a question I posted earlier (here), and trying to convert the answer to a sub so I can use it multiple times. Not sure that it's done right though. Can anyone provide a better or cleaner sub?

I have a good deal of experience programming, but my primary language is PHP. It's frustrating to know how to execute in one language, but not be able to do it in another.

sub search_for_key
{
    my ($args) = @_;

    foreach $row(@{$args->{search_ary}}){
        print "@$row[0] : @$row[1]\n";
    }

    my $thiskey = NULL;

    my @result = map { $args->{search_ary}[$_][0] }     # Get the 0th column...
        grep { @$args->{search_in} =~ /$args->{search_ary}[$_][1]/ } # ... of rows where the
            0 .. $#array;                               #     first row matches
        $thiskey = @result;

    print "\nReturning: " . $thiskey . "\n";
    return $thiskey;    
}

search_for_key({
    'search_ary' => $ref_cam_make, 
    'search_in' => 'Canon EOS Rebel XSi'
});

---Edit---

From the answers so far, I've cobbled together the function below. I'm new to Perl, so I don't really understand much of the syntax. All I know is that it throws an error (Not an ARRAY reference at line 26.) about that grep line.

Since I seem to not have given enough info, I will also mention that:

I am calling this function like this (which may or may not be correct):

search_for_key({
    'search_ary' => $ref_cam_make, 
    'search_in' => 'Canon EOS Rebel XSi'
});

And $ref_cam_make is an array I collect from a database table like this:

$ref_cam_make = $sth->fetchall_arrayref;

And it is in the structure like this (if I understood how to make the associative fetch work properly, I would like to use it like that instead of by numeric keys):

Reference Array
Associative
row[1][cam_make_id]: 13, row[1][name]: Sony

Numeric
row[1][0]: 13, row[1][1]: Sony
row[0][0]: 19, row[0][1]: Canon
row[2][0]: 25, row[2][1]: HP

sub search_for_key
{
    my ($args) = @_;

    foreach my $row(@{$args->{search_ary}}){
        print "@$row[0] : @$row[1]\n";
    }

    print grep { $args->{search_in} =~ @$args->{search_ary}[$_][1] } @$args->{search_ary};
}

Answer 1

If you're intending to return whether a match is found, this code should work (inefficiently). If you're intending to return the key, though, it won't -- the scalar value of @result (which is what you're getting when you say $thiskey = @result;) is the number of items in the list, not the first entry.

$thiskey = @result; should probably be changed to $thiskey = $result[0];, if you want mostly-equivalent functionality to the code you based this off of. Note that it won't account for multiple matches anymore, though, unless you return @result in its entirety, which kinda makes more sense anyway.

Answer 2

This whole sub is really confusing, and I'm a fairly regular perl user. Here are some blanket suggestions.

• Do not create your own undef ever -- use undef then return at the bottom return $var // 'NULL'.
• Do not ever do this: foreach $row, because foreach my $row is less prone to create problems. Localizing variables is good.
• Do not needlessly concatenate, for it offends the style god: not this, print "\nReturning: " . $thiskey . "\n";, but print "\nReturning: $thiskey\n";, or if you don't need the first \n: say "Returning: $thiskey;" (5.10 only)
• greping over 0 .. $#array; is categorically lame, just grep over the array: grep {} @{$foo[0]}, and with that code being so complex you almost certainly don't want grep (though I don't understand what you're doing to be honest.). Check out perldoc -q first -- in short grep doesn't stop until the end.

Lastly, do not assign an array to a scalar: $thiskey = @result; is an implicit $thiskey = scalar @result; (see perldoc -q scalar) for more info. What you probably want is to return the array reference. Something like this (which eliminates $thiskey)

printf "\nReturning: %s\n", join ', ', @result;
@result ? \@result : 'NULL';

Answer 3

You are moving in the direction of a 2D array, where the [0] element is some sort of ID number and the [1] element is the camera make. Although reasonable in a quick-and-dirty way, such approaches quickly lead to unreadable code. Your project will be easier to maintain and evolve if you work with richer, more declarative data structures.

The example below uses hash references to represent the camera brands. An even nicer approach is to use objects. When you're ready to take that step, look into Moose.

use strict;
use warnings;

demo_search_feature();

sub demo_search_feature {
    my @camera_brands = (
        { make => 'Canon', id => 19 },
        { make => 'Sony',  id => 13 },
        { make => 'HP',    id => 25 },
    );

    my @test_searches = (
        "Sony's Cyber-shot DSC-S600",
        "Canon cameras",
        "Sony HPX-32",
    );

    for my $ts (@test_searches){
        print $ts, "\n";
        my @hits = find_hits($ts, \@camera_brands);
        print '  => ', cb_stringify($_), "\n" for @hits;
    }
}

sub cb_stringify {
    my $cb = shift;
    return sprintf 'id=%d make=%s', $cb->{id}, $cb->{make};
}

sub find_hits {
    my ($search, $camera_brands) = @_;
    return grep { $search =~ $_->{make} } @$camera_brands;
}