Pondering Role Organization

Ovid on 2009-04-06T09:21:42

Important Update: Please do not code like I did below. I know that using separate with statements is stupid and I don't understand why I did that in the code below. If you have separate with statements, you won't get method collision detection, so the code below should be written like this:

package PIPs::ResultSetBase::Group;

use Moose;
extends 'PIPs::ResultSetBase::Pips';
with qw(
    PIPs::ResultSet::Role::DoesCreateWithEntity
    PIPs::ResultSet::Role::DoesPromotionSearch
    PIPs::ResultSet::Role::DoesTagSearch
);

__PACKAGE__->meta->make_immutable( inline_constructor => 0 );
no Moose;

1;

Now back to your original post :)

We have some DBIx::Class::ResultSource classes -- Groups, Seasons, Franchises and Collections -- which represent editorial rules around how other things can be grouped. Their corresponding DBIx::Class::ResultSet classes really don't have any behavior that's unique to editorial groups. They're just ways of containing groups of classes. One of them looks like this:

package PIPs::ResultSet::Franchise;

use Moose;
extends 'PIPs::ResultSetBase::Group';

__PACKAGE__->meta->make_immutable( inline_constructor => 0 );
no Moose;

1;

Because we don't inherit directly from our result set class, we've an extra layer that you have to read to fully understand everything. What does that layer look like?

package PIPs::ResultSetBase::Group;

use Moose;
extends 'PIPs::ResultSetBase::Pips';
with 'PIPs::ResultSet::Role::DoesCreateWithEntity';
with 'PIPs::ResultSet::Role::DoesPromotionSearch';
with 'PIPs::ResultSet::Role::DoesTagSearch';

__PACKAGE__->meta->make_immutable( inline_constructor => 0 );
no Moose;

1;

So we're basically doing this:

DBIx::Class::ResultSet
         ^
         |
PIPs::ResultSetBase::Pips
         ^
         |
PIPs::ResultSetBase::Group
         ^
         |
PIPs::ResultSet::Franchise

Ignoring the inheritance in DBIx::Class, we've stacked an extra three levels on top. Why? This has gained us nothing. My goal is eventually to have something like this:

package PIPs::ResultSet::Franchise;

use Moose;
with 'PIPs::Role::DoesResultSet';
with 'PIPs::ResultSet::Role::DoesCreateWithEntity';
with 'PIPs::ResultSet::Role::DoesPromotionSearch';
with 'PIPs::ResultSet::Role::DoesTagSearch';

__PACKAGE__->meta->make_immutable( inline_constructor => 0 );
no Moose;

We can thus see instantly that this class can be treated as a result set, creates an "entity" (long story) and can be searched by promotions and tags. My only concern is that this result set no longer identifies itself as a "group" the way it did when it inherited from the "group" base class. I could create a "DoesGroup" role:

package PIPs::ResultSet::Role::DoesGroup;

use Moose::Role;
with 'PIPs::ResultSet::Role::DoesCreateWithEntity';
with 'PIPs::ResultSet::Role::DoesPromotionSearch';
with 'PIPs::ResultSet::Role::DoesTagSearch';

__PACKAGE__->meta->make_immutable( inline_constructor => 0 );
no Moose;

1;

And the Franchise class would merely do this role and you can call $franchise->meta->does_role('PIPs::ResultSet::Role::DoesGroup'), but that gains me little over inheritance because I no longer have a list of convenient behaviors at the top of the Franchise class.

On the other hand, do I really want that? I often find that code which handles ->can and ->does is a code smell because the object system should handle dispatching and we shouldn't be manually checking our types, but it is useful at times.

I've decided to hold on this refactoring because frankly, I'm unsure what's the right thing to do here. Decisions, decisions ...

---

-does a code smell?

sartak on 2009-04-06T18:12:00

I disagree with ->does being a code smell. I'll just let a slide from my recent Moose talk explain what I mean:

$creature = get_creature;

if ($creature->does('Role::Doglike')) {
    $creature->bark(at => $mailman);
}