sub new { my $proto = shift; my $class = ref($proto) || $proto; my $self = {}; bless ($self, $class); return $self; }
Someone's going to complain about the constructor - but if you never call it in the disapproved manner, it doesn't really matter.
sub file { my $self = shift; if (@_) { $self->{file} = shift; } }
For consistancy, there should be an "else { $self->{file} }" there or something to allow you to use it as an accessor. That way, rather than ever saying "$self->{file}" (other than in this method), you say "$self->file()". This allows you to change the underlying data structure merely by changing the accessors if need be.
sub _common { my $self = shift; (my $key = (caller(1))[3]) =~ s/.*:://; if (@_) { $self->{$key} = shift; } else { $self->{$key}->($self) if defined $self->{$key}; } }
Interesting use of caller. Personally, I'd just pass the mode in:
sub _common { my $self = shift; my $key = shift; # ...
and then pre/loop/post would simply look like:
sub pre { my $self = shift; $self->_common(pre => @_); } # etc.
As to the meat of your algorithmic module:
$self->pre(); open my $fh, '<', $self->{file} or croak "could not open $self +->{file}: $!"; while (my $record = <$fh>) { chomp $record; $self->{record} = $record; $self->loop(); $self->{hash}{ $self->{key} } = $self->{value}; ++$self->{count}; } $self->post();
I'm not sure this is necessarily a good thing. You are forcing a few things. First, all communication is done via $self. This is only natural inside a single object, however your callbacks are all external to your object, so that doesn't make a bunch of sense to me. Second, you're limiting each loop to a single key/value. Some items in the loop may result in no data to be stored, while other items may be split into multiple keys/values. So something like this may be in order:
my %data = $self->loop(); @{$self->{hash}{keys %data}} = values %data; $count += scalar keys %data;
(that scalar may be redundant, but I like being explicit.) Besides that, because the data is being pass in implicitly, it's not really all that obvious what is going on. Passing in data via loop() isn't an option because then _common will think you're setting something. Global variables are worse. So maybe _common doesn't run the code automatically, or maybe it does, but doesn't do setting at all.

Also remember that the coderefs you're getting are generally going to be closures anyway. They're likely to be setting data in their own object, not yours. So maybe we don't need to return anything at all, just give the record to the callback, and ignore the return. And ignore the counting. Then we can put the record into $_ (localised properly, of course), and not pass anything in.

My two cents (CDN) :-)


In reply to Re: OOP first-timer seeks feedback; likes long walks on the beach. by Tanktalus
in thread OOP first-timer seeks feedback; likes long walks on the beach. by eff_i_g

Title:
Use:  <p> text here (a paragraph) </p>
and:  <code> code here </code>
to format your post, it's "PerlMonks-approved HTML":



  • Posts are HTML formatted. Put <p> </p> tags around your paragraphs. Put <code> </code> tags around your code and data!
  • Titles consisting of a single word are discouraged, and in most cases are disallowed outright.
  • Read Where should I post X? if you're not absolutely sure you're posting in the right place.
  • Please read these before you post! —
  • Posts may use any of the Perl Monks Approved HTML tags:
    a, abbr, b, big, blockquote, br, caption, center, col, colgroup, dd, del, details, div, dl, dt, em, font, h1, h2, h3, h4, h5, h6, hr, i, ins, li, ol, p, pre, readmore, small, span, spoiler, strike, strong, sub, summary, sup, table, tbody, td, tfoot, th, thead, tr, tt, u, ul, wbr
  • You may need to use entities for some characters, as follows. (Exception: Within code tags, you can put the characters literally.)
            For:     Use:
    & &amp;
    < &lt;
    > &gt;
    [ &#91;
    ] &#93;
  • Link using PerlMonks shortcuts! What shortcuts can I use for linking?
  • See Writeup Formatting Tips and other pages linked from there for more info.