Me know nuthin' 'bout objects.
There are days when I feel really, really stupid. Today's stupidity has actually been going on for several weeks, simmering in the back of my mind. I just received a bug report for TAPx::Parser from Torsten Schönfeld. Seems that my parser doesn't like blank lines in the output. After some debugging, it became clear that the following is a big bucket of suck:
sub next {
my $self = shift;
if (@{$self->_m_tokens}) {
return shift @{$self->_m_tokens};
}
if ($self->_current_chunk) {
if ( $self->_stream_started ) {
$self->_start_tap(0);
}
else {
$self->_start_tap(1);
$self->_stream_started(1);
}
}
my $next_chunk = $self->_stream->next;
if (! $self->_current_chunk() && $next_chunk ) {
$self->_current_chunk($next_chunk);
return $self->next;
}
unless ( defined $next_chunk ) {
$self->_end_tap(1);
}
if ( defined $self->_current_chunk ) {
my @current_tokens = map {
my $result = TAPx::Parser::Results->new($_);
$self->_validate($result);
$result;
} $self->_lex($self->_current_chunk());
my $token = shift @current_tokens;
push @{$self->_m_tokens()} => @current_tokens;
$self->_current_chunk($next_chunk);
return $token;
}
$self->_finish;
return;
}
What the hell is that? I can barely follow it and I wrote it! I hated writing it at the time and I hate it even worse, now. But that's because I was really, really stupid and let a known design flaw remain in my parser. I didn't have my responsibilities appropriately distributed.
When I said at the beginning "me know nuthin'", though this is a bit of a slam on me, it's also sound advice about objects. I shouldn't know a damn thing about 'em. I should beg them for information and tell them what they want to know. That's it. I shouldn't know anything about their internals or, for that matter, their state. However, in the above code, I was actually trying to track the state of an iterator or stream. I wasn't reaching in, I was trying to track it externally. That was stupid.
If you really, really need to know the state of something and you can't ask it, you've probably got a design flaw. In the iterator code, I copied it almost verbatim from Test::Harness::Iterator. I'm sure that the Test::Harness code is fine for its needs, but it's completely innapropriate for mine. In fact, my subconscious was even trying to tell me I was being a moron because prior to that method you find the following comment:
# all of this annoying current and next chunk stuff is to ensure that we # really do know if we're at the beginning or end of a stream.
And that horror is all because I didn't implement first and last methods for my iterator. Well, I've now finished rewriting the iterator, but it's on my computer at home and I'm at work, so I won't be able to upload right away. While I haven't actually implemented it, a first pass at rewriting the next method would look something like this:
sub next {
my $self = shift;
my $stream = $self->_stream;
my $next = $stream->next;
$self->_start_tap( $stream->first ? 1 : 0 );
if ( $stream->last ) {
$self->_finish;
return;
}
# this is a tiny oversimplification
my $token = TAPx::Parser::Results->new( $self->_lex($next) );
$self->_validate($token);
return $token;
}
That's much cleaner. If I had gone ahead and fixed this problem when I first knew about it, I wouldn't have had two serious bug reports about this method in the space of five days.
Those methods should probably be called is_first and is_last instead, though.
Re:Naming
Ovid on 2006-09-15T06:42:28
Agreed. I'll go make that change.