Anonymous Monk has asked for the wisdom of the Perl Monks concerning the following question:

im a newbie and i'm trying to do a little script that grabs phone numbers in phonebook.txt using a name list file (namelist.txt) my script is always printing the original contents of the phonebook.txt...what could be wrong :-( 
#usage: ./getfone.pl output 
use strict;

open(NL, "namelist.txt) || die $!;
open(PB, "phonebook.txt") || die $!;
open(OUT,">$ARGV[0].txt") || die $!;

while(1) {
  my @phonelisting = <PB>;
  chomp(@phonelisting);  
  my @name = <NL>;
  chomp(@name);
  
  my @phonesearch = grep @name, @phonelisting;
  
print OUT "@phonesearch\n";
last if eof;
}

close (NL);
close (PB);
close (OUT)

[download]

Replies are listed 'Best First'.
Re: keyword query
by samtregar (Abbot) on May 06, 2002 at 22:29 UTC
    Your problem is on this line: 
    my @phonesearch = grep @name, @phonelisting;

    [download]
    I'm not exactly sure what you expect that line to do, but I bet it's not what it does! The grep function takes two arguments - an expression and a list. Each member of the list is then evaluated using that expression. In this case you gave it the expression (!) @name and then evaluated each item of @phonelisting using it!

    Maybe you wanted: 

    my @phonesearch;
foreach my $name (@names) {
  push(@phonesearch, grep(/$name/, @phonelisting));
}

    [download]
    But I can't really be sure.

    -sam

    UPDATE: added some parens to make the push/grep line a little more obvious.

[reply]
[d/l]
[select]
Re: keyword query
by erikharrison (Deacon) on May 06, 2002 at 22:46 UTC

    Not bad really. You're very close, good for a beginner. Let's go through it.

    #usage: ./getfone.pl output 
use strict;

open(NL, "namelist.txt) || die $!;
open(PB, "phonebook.txt") || die $!;
open(OUT,">$ARGV[0].txt") || die $!;

    [download]

    So far so good. Especially good seeing the die. It might be useful to add some useful info of your own to the die's though, and as long as you are using strict you might as well use warnings.

    while(1) {

    [download]

    Alright, an infinite loop. These are rarely useful, outside of event handlers. Rethink this.

      my @phonelisting = <PB>;
  chomp(@phonelisting);  
  my @name = <NL>;
  chomp(@name);

    [download]

    Alright, you're reading the files into arrays and chomping them. Fair enough. However, you're doing this inside a loop. Consider - the while loop starts, every line of both files are read into the two arrays. Why loop again. Even if your loop is necessary, why read the files in at the begining of each iteration?

      my @phonesearch = grep @name, @phonelisting;

    [download]

    This is your real problem. grep doesn't quite work this way. grep take a block and a list. It iterates through the list, executing the block for each member of the list setting the $_ to the list member, then returns a list of every member which got the block to return true. This is where you're having problems.

    print OUT "@phonesearch\n";
last if eof;

    [download]
    Alright, the print is fine assuming that the grep went right (which it didn't). The eof however is less good. It does what you want it to - breaks the loop if we've reached the end of NL - which we always will, cause we read in the whole file to the array @name. Effectively, the while allways iterates once. Why not just drop the while?

    Also note that your otherwise well indented code, was improperly indented here. Line the print and the last up with the my in the previous line, for clarities sake.

    close (NL);
close (PB);
close (OUT)

    [download]

    And no problem here. You close the files (which perl would do for you, but it's nice that you did it anyway), however, you (validly) drop the semicolon after the last close. Now, while the semicolon isn't required it's considered good form, in case you add a line latter, and then wonder about the new slew of syntax errors.

    Cheers,
    Erik
[reply]
[d/l]
[select]
Re: keyword query
by VSarkiss (Monsignor) on May 06, 2002 at 22:34 UTC

    samtregar appears to have hit the nail on the head above. I just wanted to point out that there's no need for a loop. These lines: 

    my @phonelist = <PB>;
# ...
my @name = <NL>;

    [download]
    read the entire contents of the files into the respective variables. If the diagnosis above is correct, you can get rid of your outermost loop also.

[reply]
[d/l]
Re: keyword query
by Cyrnus (Monk) on May 06, 2002 at 22:41 UTC
    when using grep() the expresion goes first and the search string second. So this my @phonesearch = grep @name, @phonelisting; should be this my @phonesearch = grep /@name/, @phonelisting;

    edit:After reading the other replies I went back and looked up grep(). I was thinking it worked exactly like *NIX's grep but it seems I was mistaken. I've changed the above to relect this but I am unsure of the validity of putting an array in a REGEX. I would appreciate any input on this. I still stand by the below block as being my prefered way to preform this search.

    Instead of using grep however; I would do your while loop something like this (untested code) 
    my @phonesearch;
my @name =<NL>;
chomp(@name);
my $name;

while(my $phonelisting = <PB>) {
  chomp($phonelisting);  
  foreach $name (@name) {
    if ($phonelisting =~ /$name/) {
      push(@phonesearch,$phonelisting);
    }
  }
}
print OUT "@phonesearch\n";

    [download]
    notice I moved the declaration of @phonesearch outside the loop, this is so the value wouldn't expire when the loop ended. The reasons for my other changes are that using while(1) is a bad idea. It sets you up for an infinite loop and could bite you in a future program if it becomes a habit. Also for this type of application it makes more sense to me to go through the lists one element at a time.

    John
[reply]
[d/l]
[select]
Re: keyword query
by Anonymous Monk on May 06, 2002 at 22:16 UTC
    corrections: 
    open(NL, "namelist.txt") || die $!;
close (OUT);

    [download]
    still not working
[reply]
[d/l]

Back to Seekers of Perl Wisdom