[code-review] Re: Lingua::Phonology

        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.
I think the semantics is just too complicated and unreliable.  For 
example, this works:

        package A;
        sub new { print "new\n" }
        package main;
        my $z = new A;

but this does not:

        package A;
        sub new { print "new\n" }
        package main;
        sub A { print "A\n" }
        my $z = new A;


I always prefer to use 'Lingua::Phonology::Syllable->new' rather than
'new Lingua::Phonology::Syllable'.


        # 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.

There are a lot of tricks for eliminating duplications of accessor
methods like this.  Here is a straightforward technique: implement a
generic function that abstracts the common behavior of all the
accessors.

        # Implement an accessor method.
        # Return or modify member named $member in object $self.
        # ('modify' if $val is present.)
        # If 'modify', make sure $val is in class $class.
        # If $continuation is present, it is a code ref that
        # implements additional actions to be performed.

        sub accessor {
          my ($self, $member, $class, $continuation, $val) = @_;
          if (@_ == 5) {
            return carp "Bad argument to \L$member\E()"
              if not UNIVERSAL::isa($val, $class);
            $self->{$member} = $val;
          }
          $continuation->(@_) if $continuation;
          return $self->{$member};
        }

Now three of the methods become

        sub symbols  { my $self = shift;
                       $self->accessor('SYMBOLS', 'Lingua::Phonology::Symbols',
                                        undef, @_) }

        sub rules    { my $self = shift;
                       $self->accessor('RULES', 'Lingua::Phonology::Rules, 
                                        undef, @_) }

        sub syllable { my $self = shift;
                       $self->accessor('SYLL', 'Lingua::Phonology::Syllable,
                                        undef, @_) }

The fourth one, 'features', has a little extra behavior:

        sub features { my $self = shift;
                       $self->accessor('FEATURES', 'Lingua::Phonology::Features,
                                        sub {
                                          return unless @_ == 5;
                                          my ($self, $arg) = @_[0, 4];
                                          $self->{SYMBOLS}->features($arg);
                                        }, @_) }


I'm not really happy with the hardwiring of the 4's and 5's here, but
I don't think I have enough insight into how these functions are used
to do better right now.

Now I'd like to return to the definition of the accessor methods.
Each method has something like

            return carp "Bad argument to \L$member\E()"
              if not UNIVERSAL::isa($val, $class);

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);

Here $erroneous_argument was supposed to be a ::Features object, but
isn't.  The current behavior is to issue a warning and then continue.
But this is likely to cause some sort of serious failure much later,
because $object isn't constituted the way it was intended to be.
Essentially all future uses of $object are erroneous, possibly in
subtle ways.  Unless there's a good reason for 'carp' that I have
missed, I would want to change 'carp' to 'croak'.



        # Load a complete phonology definition from a file
        sub loadfile {
            my ($self, $file) = @_;

            if ($file) {

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?

                # Readable file?
                if (-e $file && -r $file) {
                    open IN, $file or return err("Couldn't open $file: $!");
                    $file = join '', <IN>;
                    close IN;
                }
                # A glob reference - assume is a handle
                elsif (ref $file eq 'GLOB') {
                    $file = join '', <$file>;
                }
                # If none of above, assume $file is literal string or null

Here the code to read the file is repeated.  By a small change in the
logic, we can eliminate the repetition and get a more straight-line
flow:

                # Readable file?
                if (-e $file && -r $file) {
                    open IN, $file or return err("Couldn't open $file: $!");
*                   $file = \*IN;
                }
                # A glob reference - assume is a handle
*               if (ref $file eq 'GLOB') {
                    $file = join '', <$file>;
                }
                # If none of above, assume $file is literal string or null

(Stars indicate the lines I've changed.)

Instead of
                if (ref $file eq 'GLOB') {

I would probably prefer

                if (UNIVERSAL::isa($file, 'GLOB')) {

because that works even on blessed glob references, such as those
produced by IO::Handle; now ->loadfile works when passed an IO::Handle
or IO::File object, or a socket, or some other filehandle-like object
generated by a module we've never heard of.

Also, I think the guard conditions in 
                # Readable file?
                if (-e $file && -r $file) {

are probably misguided.  Here your program is going to extra effort to
decide if the file is open before trying to open it.  Why use this
indirect method?  The easiest and most reliable way to find out if a
file is openable is to try to open it.  This block would then become:

                if (open IN, $file) {
                    $file = \*IN;
                } else {
                    return err("Couldn't open $file: $!");
                }

Now, I don't like the behavior of this function because I think it is
fragile.  If the 'open' succeeds, the $file is interpreted as a
filename; if not, it is interpreted as a literal string.  Interpreting
$file as a literal string is not a sensible thing for a function named
'loadfile' to do.  Suppose the programmer writes

        $object->loadfile('my_phonolgoy_file');

with a misspelled filename.  They should expect to get an error.  As
originally written, the function does not deliver an error message.
It blithely continues, using what is obviously the entirely wrong
data, the literal string 'my_phonolgoy_file'.  I can't imagine that
this is desirable behavior.

Even worse, suppose the user has not misspelled the filename.

        $object->loadfile('my_phonology_file');

The program works beautifully for six months, and then one day someone
accidentally removes the read permissions from the data file.  Does
the program stop and say "Sorry, permission denied while opening
'my_phonology_file'"?  No, it doesn't.  It continues blithely and
silently, generating completely bogus output.

I notice that none of this potentialy awful behavior is documented.

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.



Here's how I would write this:

        sub loadfile {
            my ($self, $file) = @_;
            my $data;

            $file ||= $self->default_file;

            unless (ref $file) {
              open IN, "<", $file or return err("Couldn't open $file: $!");
              $file = \*IN;
            }

            # A glob reference - assume is a handle
            if (UNIVERSAL::isa($file, 'GLOB')) {
              local $/;
              $data = <$file>;
            } else {
              croak "Invalid argument to 'loadfile': should be filename or handle; aborting";
            }

            $self->load_literal_data($data);
        }

        sub load_literal_data {
            my ($self, $data) = @_;
            my $succeeded = 1;
            for (qw(features symbols syllable rules)) {
                $succeeded &&= $self->$_->load_literal_data($file);
            }
            return $succeeded;
        }


Now Phonology objects have a 'load_literal_data' method so that if for
some reason the user does have a literal string that they want to
load, they can call '->load_literal_data($string)' to load it instead
of depending on undocumented and fragile behavior of 'loadfile'.

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.

I've added a ->default_file method.  Previously, the behavior in case
the filename was not specified was delegated to the member methods.
Maybe this is important; I haven't seen them.  But supposing that the
default behavior is to read from a default file, I think this is the
simplest way to do it.  $self->default_file can return the name of the
default file.  By making it into an ordinary accessor method, the user
can use

        $self->default_file('my_default');

to set the default file; or they can subclass the module and override
default_file to get more sophisticated behavior.

        # All calls to loadfile in all module eventually come here
        sub _read {
                my ($file, $key, %parms) = @_;

                my $parse;
                eval { $parse = XMLin($file, KeepRoot => 1, %parms) };

Without seeing the calls to the submodules' 'loadfile' methods, it's
hard to be sure what's happening here.    Here's my guess.  The XML
file contains a section for 'Syllable', a section for 'Symbols', a
section for 'Rules', and so on.  The function loads in the XML file,
parses it, and, in the folowing section, returns an XML object for the
selected section:

                return err("Errors reading $file: $@") if $@;

            # Find the desired element ($rv) at any depth in the parsed structure w/
            # _recurse_search()
                my $rv;
                return $rv if $rv = _recurse_search($parse, $key);
                return err("Couldn't find element <$key>");
        }

Since there are four sections, the XML file is parsed four times.
This is wasteful.   The parsed XML data is a property of the Phonology
object and should be stored in it.  Assuming that I understand
correctly what is happenning, I think I would want to do something
more like this:

        sub load_literal_data {
            my ($self, $data) = @_;
            my $succeeded = 1;
            my $xml = {{whatever it is you need to do 
                        to turn $data into an XML object}};
            $self->set_xml($xml);
            for (qw(features symbols syllable rules)) {
                my $section = _recurse_search($xml, $key);
                my $member = uc($_ . "_xml");
                $self->{$member} = $section;
                $succeeded = 0 unless $section;
            }
            return $succeeded;
        }

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?

        # Find a particular element in a complex data structure w/ hash and array
        # references
        sub _recurse_search { 

            my ($ref, $key) = @_;

                my @list;
                if (ref $ref eq 'HASH') {
                        return $ref->{$key} if exists $ref->{$key};
                        @list = values %$ref;
                }
                elsif (ref $ref eq 'ARRAY') {
                        @list = @$ref;
                }
                else {
                        return undef;
                }

                for (@list) {
                        my $rv;
                        return $rv if $rv = _recurse_search($_, $key);
                }

            # Getting here indicates failure
            return undef;
        }

Here I think you'll probably want to use UNIVERSAL::isa rather than
'ref' to test for hashness and arrayness.  But maybe not.

I think I would have written this:

                for (@list) {
                        my $rv;
                        return $rv if $rv = _recurse_search($_, $key);
                }

as this:

                for (@list) {
                        my $rv = _recurse_search($_, $key);
                        return $rv if $rv;
                }

because it's a little more normal looking.

Now, we're still doing four recursive searches of the parsed XML
document.  Only one search is necessary.  Whether this makes a
difference depends on how big the XML file is.  I will assume that the
XML file is not too big so that it doesn't matter.  If it did matter,
I would want to rewrite _recurse_search this way:

        sub _recurse_search {
          my ($self, $ref, $targets) = @_;

$targets will be a hash that maps the interesting 'key' values to
member names.  When an appropriate key is located in the $ref data,
its value will be stored into the appropriate member of $self:

          my @subitems;
          if (UNIVERSAL::isa($ref, 'HASH')) {
            for my $k (keys %$ref) {
              if (exists $targets->{$k}) {
                $self->{$targets->{$k}} = $ref->{$k};
              } else {
                $self->_recurse_search($ref->{$k}, $targets);
              }
            }
          } elsif (UNIVERSAL::isa($ref, 'ARRAY')) {
            for my $e (@$ref) {
                $self->_recurse_search($e, $targets);
            }
          }
        }


Then you call something like

        $object->_recurse_search($xml,
                                 { features => 'FEATURES',
                                   symbols  => 'SYMBOLS',
                                   syllable => 'SYLL',
                                   rules    => 'RULES',
                                 });



        # Global variable used in savefile() and write()
        our $add_root = 1;

I don't know what this is for, but I don't like it.  I think
'savefile' is a bad idea generally.  The subobjects should each have a
'to_str' method that returns a string representation of the target
object.  Then you write

        sub to_str {
          my $self = shift;
          my $subobject_data =
            join "\n\n", 
              map $self->$_->to_str, 
                  qw(features symbols syllable rules);
          "<phonology>\n$subobject_data\n</phonology>\n";
        }

        sub savefile {
          my ($self, $file) = @_;
          $self->_print($file, $self->to_str);
        }


Now, the primary purpose of '_print' is to turn $file from a filename
into a glob reference.  We've done this once before, in 'loadfile',
which suggests that it should be a utility function.  Perhaps
something like this:

        sub to_handle {
          my ($name, $mode) = @_;
          return $name if UNIVERSAL::isa($name, 'GLOB');
          my $handle = IO::Handle->new;
          open $handle, $mode, $name or return;
          return $handle;
        }

Then 'savefile' becomes:

        sub savefile {
          my ($self, $file) = @_;
          my $handle = to_handle($file, '>')
            or croak ...;
          print $handle $self->to_str;
        }

and we can get rid of '_print' completely.

'loadfile' similarly becomes a lot simpler:

        sub loadfile {
            my ($self, $file) = @_;
            my $data;

            {
              my $handle = to_handle($file || $self->default_file)
                or croak(...);
              local $/;
              $data = <$handle>;
            }

            $self->load_literal_data($data);
        }


The next function isn't used anywhere:

        # Takes a hash reference and a file and parses it to a string, then writes it
        # to a file if a filename is given. The actual writing is done in _print().
        sub _write {


so get rid of it.

        # A very short error writer, also imported by sub-modules
        sub err {
                carp shift if warnings::enabled();
                return undef;
        } # end err

In general, 'return undef' is a mistake.  The reason is that a
function that does 'return undef', if called in list context, will
return a *true* value:

        @result = err(...);
        if (@result) {
          print "err() returned true\n";
        }

This *will* print 'err returned true'.

People often write

        sub some_func {
          if (...) {
            return 1;  # True
          } else {
            return wantarray ? () : undef;  # False
          }
        }

but this isn't the easy way to do it.  The right way to write a
function in Perl that returns a false value is:

        sub some_func {
          if (...) {
            return 1;  # True
          } else {
            return;  # False
          }
        }

So here:
        
        # A very short error writer, also imported by sub-modules
        sub err {
                carp shift if warnings::enabled();
                return;
        }

I think I might prefer

        # A very short error writer, also imported by sub-modules
        sub err {
*               carp @_ if warnings::enabled();
                return;
        }

since it's extra flexibility at no cost.

I hope this was helpful.