Well, I'll have to think about a more elegant and efficient way, but so far I don't see any serious problem with what you are doing. Remember the saying "a sufficiently encapsulated hack is no longer a hack." The questions to ask are: "is this really causing efficiency problems?" and "is there a more maintainable solution?" If it suits your needs, don't fix it without good reason. This is a Bad Idea.

Elaborating: let's say you make this method 50% more efficient in terms of "time" (loosely speaking). If you find out that it's only taking up 1% of your programs runtime, you possibly have wasted time making a non-issue more performant. Further, I find for myself that when I start worrying about performance issues, even on systems I've built from the ground up, I almost always guess wrong about where the performance bottlenecks are (even the "obvious" database performance issues may be memory bound, IO bound or just bloody awful queries).

That being said, I would personally create a private $can_contain method.

my $can_contain = sub {
    my ( $self, $object ) = @_;
    $self->container_type eq $object->container;
};

sub insert {
    my ( $self, $object ) = @_;
    unless ( $self->$can_contain( $object ) ) {
        die "I can't hold that object!";
    }
    push @{$self}, $object;
}
[download]

With private methods like that, you can be guaranteed that no on else will inherit them, no one outside of your class will see the data they don't need to see and if anything else in the method needs to know if it "can contain" something, you're safe.

Though I still wouldn't fix it until it merits the fix :)

can anyone recommend a more elegant and efficient way to achieve the same behaviour?

The nice thing is that this module emulates a similar strategy for Perl6, so you'll be able to reuse your experience more directly as time passes.

In the end, you want a good cohesion, but loose coupling. Imagine if you took half of your classes and threw them in another project. Will those classes and their extra methods make sense elsewhere?

This doesn't easily apply to business-level classes, since everyone's business works different. :)

Can you give a good example of how that works here? The original poster didn't say these objects were related via inheritance. A Tree probably isn't a subclass of Trees. The latter sounds like a collection of the former. The only inheritance being talked about in that node is the "listable" abstract base class. Trying to shoehorn inheritance into this model could be a bad thing, but maybe you're seeing something I'm not.

Cheers,
Ovid

New address of my CGI Course.

In this case, inheritance would just be used to mark classes as implementing a certain interface. There is no "implements" equivalent in Perl, so you can fake it by making an empty class with a descriptive name and POD that describes an interface, and then adding it to the @ISA for classes which implement that interface. Then you have a simple way to check, via isa().

Sure. And I'm suggesting inheritance as a different road than using methods to determine type. He doesn't wish for nodes to be inserted into certain containers, no?

Check out Re: "polymorphism" hack: nuts or wise?,(Update Re^3: "polymorphism" hack: nuts or wise?). Grape limes link a bike?

----
Give me strength for today.. I will not talk it away.. Just for a moment..
It will burn through the clouds.. and shine down on me.

Yes, you're right: a node isn't a special case of a tree - and neither is a tree a special case of a trees object. Also, I'm a bit worried about multiple inheritance. I may want to implement use fields; and I can imagine going crazy if I have multiple inheritance.

Let's break this down just a bit. First you say you want to ensure that a container can only contain certain types of things. In programming-speak, that says "comparison required". So you have two choices: string or integer comparison. Unless performance is critical, string seems fine to me.

Now, as to how you're testing, by using the inheritance tree to call the right function, that doesn't seem too bad to me, either. Normally one would check the other way around - check if $obj_to_insert->isa($self->contained_type()), I think, but either way could work, depending on the design goals.

sub Listable::insert {
    my ($self, $child) = @_;
    
    die "Cannot insert ", ref($child), " into a ", ref($self)
        unless $self->is_legal_to_insert($child);

    push @{$self}, $child;
}

sub Listable::insert {
    return 1;
}

sub Matrices::is_legal_to_insert {
    my ($self, $child) = @_;
    return 1 if $child->isa( 'Matrix' );
    return $self->SUPER::is_legal_to_insert( $child );
}
[download]

Each class gets a bite at the apple to say if such'n'such is legal or not to insert. Now, I chose to make a definition based on the fact that I'm assuming "Matrices" are only allowed to contain items of type "Matrix". You could say "Foo" is allowed to have everything except "Bar", in which case that would look like:

sub Foo::is_legal_to_insert {
    my ($self, $child) = @_;
    return 0 if $child->isa( 'Bar' );
    return $self->SUPER::is_legal_to_insert( $child );
}
[download]

Now, in this solution, you will always want to redispatch to your parent if you don't have an opinion on the matter. This is because you never know if today's parent will be your parent or your grandparent tomorrow. This way, you're refactor-proof.

Now, we're asking the container if it's allowed to hold children of that class. Another option would be to ask the children if they're allowed to be held by that type of container. That may not make sense for your current project, but it's an option that you should be conceptually familiar with.

Intuitively, I would say it is "better" design for the children to be asked who can hold them rather than the other way around. My sense is that this is related to the notion that child objects should know about the parent object they inherit from rather than the other way around. Not sure if this is that important, or applicable, in this particular case, though.

One way to solve the problem is to use the isa method from UNIVERSAL. You can say if ($obj->isa('Node')) to see if an object is a Node, or inherits from Node.

You can say if ($obj->isa('Node')) to see if an object is a Node, or inherits from Node.

Is that even necessary? If an object inherits from Node, it'll have the Object->container method in the same way as the parent Node object has (provided, that is, if the object doesn't override the method). Also, it would send me back to a chain of if / elsif / else statements?