"The key motivation for independent iterator objects is that they allow the program to have multiple iterators even on the same category by removing the need for the File::Collector object to track the iterators."

Yes, but there is a big convenience to having File::Collector track the iterators, isn't there? I can call $s->selected_file from anywhere and refer to the same file from any of the various class methods. I don't have to worry about passing the iterator objects around. Or am I missing something?

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

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

I'll give a concrete example to make clear what I mean:

package FileParser;

use parent qw ( Dondley::WestfieldVote::FileCollector );

sub get_data {
  my $s = shift;
  return $s->get_obj_prop('data', 'raw_data', $_[0]);
}

package FileCollector;

sub get_obj_prop {
  my $s    = shift;
  my $obj  = shift;
  my $prop = shift;
  my $file = shift || $s->selected_file;  # grabs the current file in 
+iterator

  if (!$prop || !$obj || !$file) {
    $s->_croak ("Missing arguments to get_obj_prop method"
      . ' at ' .  (caller(0))[1] . ', line ' . (caller(0))[2] );
  }

  my $o = $obj . '_obj';
  my $object = $s->{_files}{$file}{$o};
  my $attr = "_$prop";
  if (! exists $object->{$attr} ) {
    $s->croak ("Non-existent $obj object attribute requested: '$prop'"
      . ' at ' .  (caller(0))[1] . ', line ' . (caller(0))[2] );
  }
  my $value = $object->{$attr};
  if (ref $value eq 'ARRAY') {
    return @$value;
  } else {
    return $value;
  }
}

package Data.pm

sub new {
  my $class = shift;
  my $data  = shift;

  my $obj   = bless {
    _raw_data            => $data, # raw Spreadsheet::Read object
    _stripped_data       => undef,
    _num_rows            => undef,
    _cols                => undef,
    _num_cols            => undef,
    _non_blank_cols      => undef,
    _first_row           => undef,
  }, $class;

  return $obj;
}

# And finally, in a file:

my $fp = FileParser->new('some/dir');
while ($fp->next_parseable_file) {
  my $data = $fp->get_data;
}
[download]

This seems super convenient. If I'm not using an iterator, I can just pass a file to the get_data() method.

With the "bundle" model and a slight modification, that last loop becomes:

foreach my $data ($fp->bundle_parseable_files->all->get_data) {
  # do something with each $data
}
[download]

The AUTOLOAD in File::Collector::Iterator::All is slightly different:

  sub AUTOLOAD {
    our $AUTOLOAD;
    my $self = shift;
    my @result = ();
    if (defined wantarray)
      { push @result, $$self->$AUTOLOAD(@_) while ($$self->next_file) 
+}
    else
      { $$self->$AUTOLOAD(@_) while ($$self->next_file) }
    return wantarray ? @result : \@result;
  }
[download]

That code for using the bundle did not work but this did:

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

I see the problem, too: $AUTOLOAD gets the fully-qualified method name, which can be used, but a call with that name will not find inherited methods.

If performance is an issue, you might want to benchmark that solution against:

{
  package File::Collector::Iterator::All;
  sub AUTOLOAD {
    our $AUTOLOAD;
    my $self = shift;
    my $method = ($AUTOLOAD =~ m/::([^:]+)$/);
    $$self->$method(@_) while ($$self->next);
  }
}
[download]

I do not know enough to predict which should be faster.