Re: [code-review] Re: Lingua::Phonology

Mark Dominus sikyal:

Thank you for your detailed review, and sorry for taking so long to get
back to it. Mostly, these were problems that I was aware of but was too
lazy to fix, though this nudge has got me moving on them.

>         sub new {
>                 my $proto = shift;
>                 my $class = ref($proto) || $proto;
>                 my $self = bless {}, $class;
>
>                 $self->{FEATURES} = new Lingua::Phonology::Features;
>                 $self->{SYMBOLS} = Lingua::Phonology::Symbols->new($self->{FEATURES});
>                 $self->{RULES} = new Lingua::Phonology::Rules;
>                 $self->{SYLL} = new Lingua::Phonology::Syllable;
>
>                 return $self;
>         }
>
> In general, I try to avoid the 'indirect object' syntax for methods.

Duly noted. I only use it for "new" calls, just because "new Class" is
such a common statement and is common across other languages.

>         # Get/set featureset
>         sub features {
>
> This is the first of four nearly identical subroutines.  I think the
> cardinal rule of computer programming is not to repeat code.  One
> reason for this is that if you repeat the code then  maintenance
> programmers must carefully compare all the versions to make sure there
> is not some subtlety they have missed.  In fact I almost missed that
> 'features' is a little different from the other three.

I had considered implementing a generalization of the accessor methods,
but figured with only four of them that it wasn't a major issue. Of
course, the number of accessors needed will grow in future versions of the
module, so I might as well get it done now.

Very useful discussion of accessor generalization snipped.

> This is very puzzling to me.  What will the method do if this error
> occurs?  This will be triggered if the user does something like
>
>         $object->features($erroneous_argument);

You're right, this should croak, not carp.

There's a wide variety of problems with the loadfile method. Most of these
problems stem from the fact that
* All of the member modules have their own loadfile() methods, which are
documented as part of the UI and now have to stay there
* All of the loadfile() methods should behave the same way
* The member loadfile() methods were written before the parent method
* Therefore, when it came time to write the parent method, it was easiest
to just pass the parsing off to those individual methods
* I had implemented a custom file format in previous versions, before I
came to my senses and switched to XML. Some of the infrastructure was
already written around the old format, though, and I was too lazy to
rewrite this.

Anyway, having not done it right the first time, I now have the problems
that you see here.

> Note that this test rules out the possibility of reading a file whose
> name is '0'.  Is there any reason not to use 'defined' here?

Not really. Actually, if I work on some of the other abstraction and code
cleanup you mention below, the need for this test disappears altogether.

> Advice 1: A function called 'loadfile' should load a file.
> Advice 2: A function should not drastically change its behavior based
>           on an unrelated external condition.

Appreciated.

> The member methods Lingua::Phonology::Symbols::loadfile, etc., did not
> load files; their arguments were not filesnames but literal strings.
> I've renamed them accordingly.

Actually, they do load files. That is,
$phono->features->loadfile('myfile') and $phono->loadfile('myfile') do the
same thing as far as the ::Features module is concerned. The difference is
that $phono->loadfile('myfile') *also* applies the settings found in
myfile to the Symbols, Rules, and Syllable modules, while
$phono->feaures->loadfile('myfile') ignores those settings not pertinent
to features. This behavior is desired (by me) and not subject to change.

However, you correctly point out that I wind up parsing things multiple
times. Instead of what I'm doing now, I think I'm going to add a
_load_from_structure() method to the member classes and do something like
the following:

# In Phonology.pm

sub loadfile {
	my ($self, $file) = @_;
	$parse = _parse($file);
	for ('features','symbols','rules','syllable')
		$self->$_->_load_from_structure($parse->{$_});

}

# In Features.pm

sub loadfile {
	my ($self, $file) = @_;
	$parse = Lingua::Phonology::_parse($file);
	$self->_load_from_structure($parse->{features});
}

Alternately, I might split _parse() and it's ilk off into
Lingua::Phonology::Common or some such.

> Since there are four sections, the XML file is parsed four times.
> This is wasteful.

This is true, but was necessitated by the fact that the sub-modules pass
different sets of options to XML::Simple. I should just make them all work
with the same options, I suppose.

> Also, the function as written seems to have a bug.  It says
>
>                 eval { $parse = XMLin($file, KeepRoot => 1, %parms) };
>
> but $file is actually a literal string containing the XML data, isn't it?

Not necessarily. XMLin accepts a filename, a handle, or a string literal
as its first argument, which is part of the reason why the loadfile()
method attempts to do the same.

Otherwise, your comments are generally accurate, and I'm going to go over
my code again and see what I need to do to it. Thanks muchly for all the
help. I'll repost when I'm satisfied with my revisions.


--
Jesse S. Bangs [email protected]
http://blog.glossopoesis.org

"We're counting on our virtues,
Cause it's too hard to count the dead."
 - Jason Webley