get_next_file might get called directly, yeah. Do you think the extra method call is that big of a deal? I'm thinking that unless you're dealing with thousands and thousands of files, it probably won't be a huge performance drag.

I rewrote it to allow multiple iterators:

sub AUTOLOAD {
  our $AUTOLOAD;
  my $s = shift;
  $AUTOLOAD  =~ /.*::get(_next)*(_\w+)_files*$/ or
    croak "No such method: $AUTOLOAD";
  my ($next, $type) = ($1, $2);

  # Don't stomp on an already existing iterator
  my $last = $s->{_last_next_req};
  if ($next && $last && ($last ne $type) && @{$s->{_file_queue}{$type}
+}) {
    croak "File queue not empty, cannot create iterator. Aborting.";
  }
  $s->{_last_next_req} = $type if $next;

  # Don't re-fetch files if we already have files in queue
  if ($next && @{$s->{_file_queue}{$type}}) {
    return $s->get_next_file($type);
  }

  # Get the files and return appropriate file(s) based on method calle
+d
  my $attr = "${type}_files";
  my @files = @{$s->{$attr}};
  return @files if !@files || !$next;
  return $s->get_next_file($type, @files);
}

sub get_next_file {
  my $s = shift;
  my $type = shift || 'all';

  if (!$s->{_selected_file}) {
    my @files         = @_ ? @_ : $s->get_files;
    $s->{_file_queue}{$type} = \@files;
  }

  my $next_file        = shift @{$s->{_file_queue}{$type}};
  $s->{_selected_file} = $next_file;

  return $next_file;
}
[download]

It might work better to have a "stack" of iterators, but the more I think about this, the more I think that your original API with a single iterator inside the object and the option to retrieve an explicit list of files in a category will probably be better for long-term maintenance.

DOH! Better still, make iterators a separate class File::Collector::Iterator with AUTOLOAD providing both get_TYPE_files and scan_TYPE_files methods, with the latter returning an iterator object. This allows moving the _selected_file instance variable into that iterator instead of making it part of the collection, which does not really make logical sense.

Here is a rough draft: (in File::Collector)

sub AUTOLOAD {
  our $AUTOLOAD;
  my $s = shift;
  $AUTOLOAD  =~ /.*::(get|scan)(_\w+)_files*$/ or
    croak "No such method: $AUTOLOAD";
  my ($mode, $type) = ($1, $2);

  # Get the files and return appropriate file(s) based on method calle
+d
  return @{$s->{$type.'_files'}} if ($mode eq 'get');
  return new ref($s).'::Iterator' (@{$s->{$type.'_files'}}) if ($mode 
+eq 'scan');
}
[download]

(in File::Collector::Iterator)

sub new {
  my $class = shift;
  bless [@_], $class;
}

sub next_file {
  my $self = shift;
  shift @$self;
  return $self->[0];
}

sub selected_file { (shift)->[0] }
[download]

This is a low-overhead proof-of-concept that simply uses an array as the iterator and uses the first element of the array as the "selected file" variable.

This would require classes that build on File::Collector to also supply subclasses for File::Collector::Iterator, but that is probably exactly what you want: delete_file could be a method on the ::Iterator for the subclass that produces the "bad header files" category.

What do you think?

I noodled around with this. So, the best I can tell, this makes iterating over files a two step process. First I have to create the iterator and then I have to loop over it? Something like this:

  my $iterator = $fc->scan_blah_files;
  while ($iterator->next_file) {
    my $file = $iterator->selected_file;
    print $file . "\n";
  }
}
[download]

I rewrote it a bit to keep the get/next terminology:

sub AUTOLOAD {
  our $AUTOLOAD;
  my $s = shift;
  $AUTOLOAD  =~ /.*::get(_next)*(_\w+)*_files*$/ or
    croak "No such method: $AUTOLOAD";
  my ($next, $type) = ($1, $2);

  if ($next) {                                      # create an iterat
+or
    my $class = ref($s) . '::Iterator';
    if (!$type) {                                   
      return $class->new($s->get_files);            # iterator for all
+ files
    } else {
      return $class->new(@{$s->{$type.'_files'}});  # iterator for fil
+e subset
    }
  } else {                                        
    return @{$s->{$type.'_files'}};                 # return file subs
+et
  }
}
[download]

$PM = "Perl Monk's";
$MCF = "Most Clueless Friar Abbot Bishop Pontiff Deacon Curate Priest Vicar";
$nysus = $PM . ' ' . $MCF;
Click here if you love Perl Monks

Ok, I spent a lot more time on this. I got it back to a one step process while using your idea to create iterator objects AND can do multiple iterators now. Woot!

sub AUTOLOAD {
  our $AUTOLOAD;
  my $s = shift;
  $AUTOLOAD  =~ /.*::get(_next)*(_\w+)_files*$/ or
    croak "No such method: $AUTOLOAD";
  my ($next, $type) = ($1, $2);

  # Handle edge case when get_next_file() method is called
  if (!$next && $type eq '_next') {
    $next = 1;
    $type = '';
  }

  # return the requested list of files
  if (!$next) {
    my $attr = "${type}_files";
    my @files = @{$s->{$attr}};
    return @files;
  }

  # get a single file from an iterator
  if ($s->{"_iterator_$type"}) {
    my $file = $s->{"_iterator_$type"}->next_file;
    $s->{_last_iterator_file} = $file;
    undef $s->{"_iterator_$type"} if (!$file);
    return $file;
  } else {
    my @files = $type ? @{$s->{"${type}_files"}} : $s->get_files;
    return '' if !@files;
    my $class = ref($s) . '::Iterator';
    $s->{"_iterator_$type"} = $class->new(@files);
    $s->{_last_iterator_file} = $s->{"_iterator_$type"}->selected_file
+;
    return $s->{"_iterator_$type"}->selected_file;
  }
}

sub selected_file {
  my $s = shift;
  return $s->{_last_iterator_file};
}
[download]

Now I can do nested loops:

  while ($fp->get_next_file) {
    print $fp->selected_file . "\n";
    while ($fp->get_next_nonparseable_file) {
      print $fp->selected_file . "\n";
    }
  }
[download]

Thanks so much for the pointers and getting me to think hard about this. Much appreciated!

$PM = "Perl Monk's";
$MCF = "Most Clueless Friar Abbot Bishop Pontiff Deacon Curate Priest Vicar";
$nysus = $PM . ' ' . $MCF;
Click here if you love Perl Monks