Does anything else call get_next_file? Do any subclasses override get_next_file? If both of those are "no", get_next_file could be merged into AUTOLOAD, improving performance by avoiding a extra method call for each file.

Independent iterators should not be much of a problem, although the semantics of ->selected_file get "interesting": it would only return the file most recently produced from the most-recently-used iterator, which could lead to confusion. The get_TYPE_files methods avoid this for programs that need to do their own iteration, but I suggest adding a reset_file_iterator method to allow iteration to be aborted rather than requiring 1 while $collector->get_next_file to be used.

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]

$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

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?