Overloading inherited class methods

[Ovid]

I seem to have coded myself into a design corner and I'm trying to figure the best way out. I have a base class that I'll call Foo::PersistentObject. This class is not to be instantiated, but instead, objects will inherit some common methods from it:

• new
• open
• save
• delete
• get_list

The intent is to make all objects that use this class persistent in the database without the application programmer needing to worry about it. The applications in the poop group didn't seem applicable for various reasons (need gradual refactoring, database limitations, etc.).

The problem is that the get_list method returns a list of objects, but the constructor has been overloaded and it's causing some problems as a result.

Here's how a subclass might use it:

package Foo::People;
use Foo::PersistentObject;
@ISA = 'Foo::PersistentObject';
use strict;

my %ARGS = (
    _id      => 'people_id',
    _table   => 'people',
    _mapping => {
        id         => 'people_id',
        first_name => 'first',
        last_name  => 'last'
    }
);

sub new {
    my ($class,$id) = @_;
    $ARGS{_open_this_id} = $id if $id;
    my $self = $class->SUPER::new( %ARGS );    
}

sub get_list {
    my $class = shift;
    $class->SUPER::get_list( %ARGS );
}

1;

Note: The intent of the %mapping hash is to provide external method names that map to internal field names so that I can change field names in the database without the programmer needing to worry about the interface.

The programmer could use this class as follows:

my $person = Foo::People->new;
$person->open( $id );

my $person = Foo::People->new( $id ); # same thing as above

my $people = Foo::People->get_list; # get a list of all people (as obj
+ects)

The problem lies in the fact that I chose to overload the constructor. See that line with the _open_this_id line? Well, if I call get_list, the super class method looks like this:

sub get_list {
    my ($class, %args) = @_;
    my ($id_name,$table) = @args{qw(_id _table)};
    my $sql = "SELECT $id_name FROM $table";

    # the database object is deliberately unavailable to the
    # programmers
    my @objects;
    my $ids = $DBO->_arrayref( $sql );
    foreach my $id ( @$ids ) {
        push @objects => $class->new(%args,open_id => $id);
    }
    return \@objects;
}

You can see that I am creating a list of objects and as I am now passing an argument list to new, it thinks that there is an id to open, but this id winds up being one of the keys of the %args hash. This causes my id to be incorrect and the constructors fail. I got around this by doing the following:

sub new {
    my ($class,$id) = @_;
    $ARGS{open_id} = $id if $id;
    $class->_new( %ARGS );    
}

sub _new { 
    my $class = shift;
    $class->SUPER::new( @_ );
}

Then, in the Foo::PersistentObject::get_list method, I call the _new class method instead of new. It seems to solve the problem, yet it also seems a bit hackish at the same time. Having to simulate overloading based on method signatures is making this code considerably more ugly.

Can anyone offer advice on a better strategy here? Also, I'm curious to know if the whole idea might be unsound. I think that it's reasonable, but I know there are those who assert that inheritance should be avoided in Perl.

Cheers,
Ovid

New address of my CGI Course.
Silence is Evil (feel free to copy and distribute widely - note copyright text)

[dws]

(You use _open_this_id in the top snippet, and open_id in the latter ones. I assume that's a typo.)

Instead of polluting the %ARGS hash (which can lead to threading problems should you ever go that way, and can royally confuse someone if they create a new instance of Foo::People without passing an _open_this_id pair, and then find that they've gotten an _open_this_id field with an id they didn't expect), why not do

  sub new {
      my $class = shift;
      $class->SUPER::new(%ARGS, @_);
  }

This nice thing about this approach is that it propogates up the chain of overriden methods nicely. That is, if Foo::People provides an %ARGS template, then it can both extend and selectively override an %ARGS template provided by Foo::PersistentObject. All without mucking up global state.

[Ovid]

Duh. That makes sense. Of course, if also pass %ARGS by reference, I don't have to worry about extra argments getting slurped up in the arg list. That's a nice benefit of pass by reference. That's a quick change that I think I'll go make.

Update: Ah, the beauty of test suites. Converting everything took me about two minutes and I can prove that is still works as well as it did :) Thanks for the help!

ok 1 - use Foo::People;
ok 2 - Foo::People->can('new')
ok 3 - The object isa Foo::People
ok 4 - Foo::People->can('open')
ok 5 - ... and opening with an existing id should succeed
ok 6 - Foo::People->can('get_company_id')
ok 7 - ... and it should return the correct company id
ok 8 - Calling a constructor with an ID should succeed
ok 9 - The object isa Foo::People
ok 10 - Foo::People->can('get_company_id')
ok 11 - ... and it should return the correct company id
ok 12 - Foo::People->can('get_list')
ok 13 - ... and it should return an arrayref of people
ok 14 - ... as objects:  isa Foo::People
ok 15 - Foo::People->can('get_company_id')

Cheers,
Ovid

New address of my CGI Course.
Silence is Evil (feel free to copy and distribute widely - note copyright text)

[dws]

Of course, if also pass %ARGS by reference, I don't have to worry about extra argments getting slurped up in the arg list.

But is that really a problem? The benefit of leaving the argument list flattened is that duplicates are taken care of automagically when you eventually do the   { @_ } in   bless { @_ }, $class; If instead you're dealing with a list of references to hashes, you have a lot more work to do in the constructor. Unless the hash arguments are going to get really big, I'd pass them flattened.

[merlyn]

Besides the wonderful other advice you've gotten in this thread, let's look at why it broke, which I don't think has been said directly.

Your overriden "new" doesn't have the same protocol as the base class "new". (The overridden "new" takes an id, then a hash of parameters, but the base class "new" just takes a hash of parameters.) That's asking for trouble. If you have a different protocol, you can't override. You have to pick a new method name, or use Class::MultiMethods to let thedamian duke it out on your behalf.

-- Randal L. Schwartz, Perl hacker
Be sure to read my standard disclaimer if this is a reply.

[feanor_269]

Whoo!!! I was going to post on something like how to do what Class::MultiMethods does when I got stopped up on it. :)
This sure makes things a lot easier.

Feanor_269

[demerphq]

It seems you and dws have worked out your design issues but I felt like throwing a comment or two. :-)

First off, i'm not the hugest fan of named parameters. I agree they are useful when you have a lot of items to pass around, or when you are generating code from a DB or the like, but I find I reuse mostly code that has simple and easy to understand semantics. Usually that means only a few arguments. But sometimes it means being a bit more creative like this

sub new {
   my $class=shift;
   my $id;
   if (@_ % 2) {
      $id=shift;
   }
   my %args=@_;
   #....
}

You see what I mean? We can use both positional and named parameters in the same call, so long as you put the positional item first and shift them off before you assign to the %hash. Now we can say

  my $foo=Foo->new(100);
  my $foo=Foo->new(100,%ARGS);
  my $bar=Bar->new(%ARGS);
  my $bar=Bar->new();

You can extend this idea to more arguments, but you have to be careful how you do it. For a single argument however it works fine. You can also play other games, for instance checking type. Many subs we commonly use do basic type checking on their arguments to provide a natural feel, but also a fine level of control. File::Find is a good example. Personally I think that a bit of extra work setting up an intuitive interface saves a lot of hassle later.

cheers, :-)

--- demerphq
my friends call me, usually because I'm late....