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

Hi -- The following code works, but it feels wrong to me, like when I see code bastardizing grep or map into doing the work of foreach. Here's I'm using s///e to accumulate ids from some html... is there a preferred way to use regexps to snip out and save pieces of document? Thanks for any style tips -- rkg 
use strict;
use FileSlurp;
my @disp;
foreach my $f (glob "*.htm") {
    print STDERR "working on $f...\n";
    open (F, $f) or die "cannot open $f";
    my $content = read_file($f);
    $content =~    s{id=(\d+)}{push(@disp, $1);$1}sieg;
    close(F);
}
print join(",", @disp), "\n";

[download]
rkg

Replies are listed 'Best First'.
Re: Using s///e and it just doesn't feel right
by diotalevi (Canon) on Jun 20, 2003 at 14:01 UTC

    It shouldn't feel right to you. That's a complete misuse of the substitution operator. You didn't need to alter $content, you went out of your way to make sure it didn't really change and then you've got this implicit loop and are calling push() many times instead of just once.

    push @disp, $content =~ /id=(\d+)/ig;
[reply]
[d/l]
Re: Using s///e and it just doesn't feel right
by Zaxo (Archbishop) on Jun 20, 2003 at 14:04 UTC

    Use one of the HTML::Parser family of modules. Your regex will fail if there spaces around the '=' or the id's are quoted.

    After Compline,
    Zaxo

[reply]
Re: Using s///e and it just doesn't feel right
by zby (Vicar) on Jun 20, 2003 at 14:02 UTC
    You can change it into a loop: 
    while($content =~ /id=(\d+)/sig){
    push(@disp, $1);
}

    [download]
    I think in standard code I would prefere the loop - while the s///e is great for golfing.

    Update: Yeah diotalevi has a better solution using the fact that a match in list context returns a list of matches.

[reply]
[d/l]
[select]
Re: Using s///e and it just doesn't feel right
by halley (Prior) on Jun 20, 2003 at 14:05 UTC
    Hm, I wonder what you think grep and map were designed to do?

  • foreach iterates over a list
  • grep iterates over an array to extract meaningful elements
  • map iterates over an array to generate a related array

    • --
    [ e d @ h a l l e y . c c ]

[reply]
[d/l]
[select]
Re: Using s///e and it just doesn't feel right
by kral (Monk) on Jun 20, 2003 at 15:56 UTC
    like when I see code bastardizing grep or map into doing the work of foreach
    I don't understand what you mean when you say "bastardizing".
    Functions like grep and map are explicit for working with arrays'elements.
    ----------
    kral
    (sorry for my english)
[reply]
[d/l]
[select]

      map should not be used in a void context, that's why foreach is there. 

      map { print "$_\n" } (@array);

      [download]
      foreach (@array) {
   print "$_\n"
}

      [download]

      These are functionally the same, but map generates a return value that can be assigned to another array, and foreach does not, therefore map is not appropriate. Instead it should be used for its return value: 

      @squares = map { $_ * $_ } (@array);

      [download]

      The above code with foreach would have to be: 

      foreach (@array) {
   push @squares, $_ * $_
}

      [download]
[reply]
[d/l]
[select]

        When the map function is used as you suggest above, it is being called in void context, which I agree is not as good as for. Having said that, I would suggest that a more perlish idiom of saying the above would not require calling map in void context at all: 

        print map "$_\n", @array;

        [download]

        And better still: 

        print join "\n", @array;

        [download]

        Just some thoughts.

[reply]
[d/l]
[select]
        map should not be used in a void context, that's why foreach is there.

        And why then is map allowed to be called in void context? =)

        -Waswas
[reply]
        These are functionally the same, but map generates a return value that can be assigned to another array, and foreach does not, therefore map is not appropriate.
        There are also situations where forech aren't appropriate. Like: 

        foreach(@array){
 push(@tmp, $_) if(/$regex/);
}

        [download]
        But I respect this way of coding 'cause TMOWTDI.
        ----------
        kral
        (sorry for my english)
[reply]
[d/l]
[select]

Back to Seekers of Perl Wisdom