You seem to be missing that you can put all of the "action" methods that your subclasses add onto File::Collector in the subclasses of File::Collector::Iterator instead.

This gives a better-separated design, where subclasses of File::Collector add categories and subclasses of File::Collector::Iterator add actions. This way, File::Collector is more of a passive data store, while the iterator subclasses perform actions, using a Model/Controller pattern.

Well, I played around with this some more. I got it working but it makes the code a lot uglier. For example, here's what I have to generate nested iterators now:

    my $it1 = $fp->next_parseable_file;
    while (my $file1 = $it1->next) {
      print $fp->short_name($file1) . "\n";
      my $it2 = $fp->next_nonparseable_file;
      while (my $file2 = $it2->next) {
        print $fp->short_name($file2) . "\n";
      }
    }
[download]

Whereas before, I just had:

    while ($fp->next_parseable_file) {
      print $fp->short_name . "\n";
      while ($fp->next_nonparseable_file) {
        print $fp->short_name . "\n";
      }
    }
[download]

The iterators now have to be created separately from the while loop. And I'm stuck passing around the $file to the short_name method.

I get that having the separation in logic the way you describe is probably more ideal from a logical perspective. But maybe this is a case where some rules should be broken? I don't have enough experience to say. Or maybe there is some trickery I could invoke to get the best of both worlds?

$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 think I might be on to something good...or it could be colossally stupid, I'm not sure which. If I could indulge your attention for a little bit more, I'd appreciate it.

So check this out:

# prints out short name of all "txt-files"
$da->bundle_txt_files->do->print_short_names;
[download]

And here is the iterator built using your suggestions (I changed the "all" method name to "do," which seemed to make more sense):

package File::Collector::Iterator ;
use strict;
use warnings;

use Carp;
use Log::Log4perl::Shortcuts       qw(:all);

my $collector;           # private package variable for storing the Fi
+le::Collector 

sub print_short_names {
  my $s = shift;
  print $collector->{files}{$s->selected_file}{short_path} . "\n";   #
+ uses the File::Collector object to look up short path
}

sub new {
  my $class = shift;
  $collector = shift;      # new gets passed the $collector from File:
+:Collector AUTOLOAD
  bless [@_], $class;
}

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

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


# do method of executing methods on child classes
sub do {
  my $self = shift;
  bless \$self, 'File::Collector::Iterator::All';
}

{
  package File::Collector::Iterator::All;
  use Log::Log4perl::Shortcuts       qw(:all);
  sub AUTOLOAD {
    our $AUTOLOAD;
    my $self = shift;
    my @method = split /::/, $AUTOLOAD;
    my $method = pop @method;
    $$self->$method(@_) while ($$self->next);
  }
}
[download]

What do you think? Is this a good solution?

$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

Ideally, the iterator should need no connection to its parent other than shared data, and I see no reason that File::Collector should be a singleton object, which means that it needs to be an instance variable if it is used at all.

We can avoid that by transferring more data into the newly-constructed iterator. I now see that your application attaches some additional attributes to each file, where I had previously supposed that you care only about the file name. The solution is simple: add a name key to each item in $collector->{files} and change selected_file to:

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

(also change the return value of next)

When constructing an iterator, pass a list of the relevant entries in ->{files} (which are hashrefs, so the per-category lists should be aliases into the master file set) to new. This even allows another bit of AUTOLOAD magic to provide selected_file_KEY getters that read $self->[0]->{KEY} on an iterator. The explicit selected_file method is therefore an alias for selected_file_name with this approach.

OK, I think I see what you are saying. I will play with it some more tomorrow when I'm fresh. Thanks.

$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