Module

[Marty Pauley]

On Sun Aug 24 05:17:31 2003, Scott McWhirter wrote:
> 
> > You really shouldn't optimise before you have enough statistics to
> > demonstrate a problem.  Also, duplication of code is a much bigger
> > problem than execution speed.  Duplicated code is bad.
> Fair point... should try to compress it down... however if you have 4000
> regexes in your array and the string matches the first one, what's the
> point in iterating over the rest if you're using any().
> The point has been brought up with me before and I've been given enough
> reason to do it in this fashion.

I think your _loop_with_break has made it worse, especially since the
code inside it is compiling and matching the regex 3 times.

You have:

    return 1 if (($var =~ /(?$opt:$_)/) && ($action eq 'any'));
    if($action eq 'all'){
      return 0 unless ($var =~ /(?$opt:$_)/);
    }
    return 0 if (($var =~ /(?$opt:$_)/) && ($action eq 'none'));

Your repeated code here is an pessimisation!
If you really want to do the same thing, do this:

  if ($var =~ /(?$opt:$_)/) {
    return 1 if $action eq 'any';
    return 0 if $action eq 'none';
  } else {
    return 0 if $action eq 'all';
  }

so you only check the regex once.

It would be better if you didn't do that at all.

I understand why you think you should optimise the 'any' case, but you
need to check all the regexes for 'all' and 'none', so your code would
be much better like:

  sub all {
    my $self = shift;
    return $self->count == @{$self->{reg_ref}};
  }

  sub none {
    my $self = shift;
    return $self->count == 0;
  }

If you leave 'any' the way you had it before, you could satisfy your
evil craving for optimisation.

I still believe that simplicity is better than speed, so I would write:

  sub any {
    my $self = shift;
    return $self->count > 0;
  }

(Actually, I would probably write sub any { $_[0]->count > 0 } )

> > Alternatively, you might want to get rid of the lexical variables and
> > use a hash slice to throw the args into $self before checking.
> *blink* think i get what you mean...

You have:

  my ($var, $reg_ref, $opt) = @_;
  ...
  my $self = {
    var => $var,
    reg_ref => $reg_ref,
    opt => $opt,
    which => undef
  };
  bless $self, $class;
  return $self;

You could have:

  my $self = bless {}, $class;
  @self{qw/var reg_ref opt/) = @_;
  ...
  return $self;

This is not an optimisation: it may be faster or slower depening on the
omitted code.

> > A map would be better than a foreach + push combo; the main point of map
> > is to replace foreach + push combos.
> fair does... will look into it.

This made a big improvement, I think.  Now that you've done that, you
should use 'grep' instead.

Change:

  unless(defined($self->{'which'})){
    @matching = map { $var =~ /(?$opt:$_)/ ? $_ : (); } @$ref;
    $self->{'which'} = \@matching;
  }

into:

    $self->{which} ||= [ grep $var =~ /(?$opt:$_)/, @$ref ];

Have fun!

-- 
Marty
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
Url : http://mail.pm.org/archives/belfast-pm/attachments/20030828/4ec1b45c/attachment.bin