Stories
Slash Boxes
Comments
NOTE: use Perl; is on undef hiatus. You can read content, but you can't post it. More info will be forthcoming forthcomingly.

All the Perl that's Practical to Extract and Report

use Perl Log In

Log In

[ Create a new account ]

Ovid (2709)

Ovid
  (email not shown publicly)
http://publius-ovidius.livejournal.com/
AOL IM: ovidperl (Add Buddy, Send Message)

Stuff with the Perl Foundation. A couple of patches in the Perl core. A few CPAN modules. That about sums it up.

Journal of Ovid (2709)

Friday December 04, 2009
08:32 AM

Pitfalls in Converting Base Classes to Roles

[ #39977 ]

I should have responded to this a while ago, but didn't. Basically, nothingmuch has a blog post responding to some of my comments about roles and it's worth reading. However, there's one problematic bit:

If you take a working multiple inheritance based design and change every base class into a role, it will still work. Roles will produce errors for ambiguities, but if the design makes sense there shouldn't be many of those to begin with. The fundamental structure of the code hasn't actually changed with the migration to roles.

For the majority of code bases I see, this is doubtless true. However, I tend to get jobs where I work on large code bases. For example, just looking at our lib/ directory:

$ find lib/ -name '*.pm' | wc -l
652

That's not huge, but it's not particularly small, either. And it doesn't count our various scripts, tests, schemas, etc. So let's say I want to add a boolean attribute to an object and said attribute must be persisted in the database and must be readable/writeable via our API. That's a single attribute. Just one. I need to:

  • Write the database migration level
  • Update our RelaxNG schemas
  • Update our data extractors (extracts data from XML docs)
  • Add the column to our domain objects
  • Add the column to our resultsource classes
  • Possibly add the column to resultset classes
  • Update our XML builders
  • Write tests
  • And possibly more, depending on why we're adding the attribute

That's a lot of work, but adding a single boolean attribute can take me an hour or two if I'm not interrupted. And, believe it or not, this is easier than it used to be. And, believe it or not, much of that isn't the silly grunt work that it appears to be because there's a lot of stuff that is hard to automate away safely (my unreleased Bermuda project was an attempt to do this). Some can and we've managed some of it, but not all.

In short, we have a complicated code base that has decades of programmer hours in it. We used to rely a lot on multiple inheritance, despite the well-known issues with MI. Now let's revisit the key sentence in nothingmuch's post: "If you take a working multiple inheritance based design and change every base class into a role, it will still work."

To nothingmuch's credit, he does point out that ambiguities will cause errors, but there's a lot more to it than that. As projects grow organically, things often get put into incorrect spots. We had all sorts of code shoved into base classes that our derived classes shouldn't inherit from, but did. Since tests tend to be written against expectations, it's easy to not write tests for things you don't expect.

The reason classes sometimes have inappropriate behavior is because abusing inheritance tends to mean that you're having classes manage both responsibilities and cross-cutting concerns. I've found that base classes often had to be broken up into two or more roles (nothingmuch has a great example of Test::Builder doing the same thing and that's a small code base). That can get really annoying when you find that each role requires the same private method and you're trying to figure out where to put it. As you juggle things like this, behaviors will change and you had better have a good test suite to catch them.

There's another, more subtle problem. Imagine that you have the following (it gets much worse when MI is involved):

AbstractParent
     ^
     |
AbstractChild
     ^
     |
ConcreteClass

In ConcreteClass we have this:

sub clean_the_kitchen {
    my $self = shift;
    $self->SUPER::clean_the_kitchen;
    $self->mop_the_floor;
    return $self;
}

What happens with that SUPER:: call? If it's only in AbstractParent, you're fine, but if it's also in AbstractChild, converting AbstractChild into a role will break your code. Further, it might break your code in very subtle ways that are hard to detect. This might seem uncommon, but we actually had this happen quite a bit when we were converting over to roles. This means that our concrete class methods needed to be written as:

after 'clean_the_kitchen' => sub {
    my $self = shift;
    $self->mop_the_floor;
    return $self;
};

That's easy to get wrong and, frankly, method modifiers such as before, after and around generally make me feel uncomfortable. Like role method aliasing and excluding, they're code smells suggesting your design needs work. However, as we were trying for "the simplest thing that could possibly work", we went with this with the intent of revisiting it later. We still haven't revisited it. We have work to do.

In short, converting bases classes to roles and resolving composition errors is often enough for small code bases, but as code bases get large and complex, it's much harder. There are plenty of pitfalls and they're not always obvious. Plus, despite having excellent code coverage for tests, you probably don't have great path coverage (few do). This means that switching from base classes to roles can easily introduce behavioral changes that you haven't been expecting.

See also Tips For Converting Base Classes Into Roles. (gosh, tags would be useful)

The Fine Print: The following comments are owned by whoever posted them. We are not responsible for them in any way.
 Full
 Abbreviated
 Hidden
More | Login | Reply
Loading... please wait.
  • Though that wasn't the main point of the post, which was that if you just go about converting base classes to roles where you have recurrence of god objects, you will get god objects implemented with roles ;-)

    As you showed, in real life lots of stuff that happens to be working probably shouldn't be. Many a time i find myself reading through old code and saying to myself something along the lines of "OH F*ET!$%*Y HOW THE H*U$!(&Y DID THIS PIECE OF &$!(&!Y% EVER WORK TO BEGIN WITH?!!". Something l

    • I certainly hope, though, that it didn't sound like I was being too critical of your reasoning. You had a great post.

      I'm concerned about the God Object comments, though. I think it's easy to create God Objects with roles and to fundamentally misunderstand OO, but God Objects are less of a concern (I think) with roles. For example, with Test::Builder generating TAP and providing the ability to share the TAP generator, you suggest that these are behaviors which can be separated by delegating to a TAP gener

      • And to illustrate the point, consider the opposite of the god object: ravioli code [c2.com]. It can be equally unmaintainable.

        • Attempting a definition of a "God Object"

          Any object that implements a model for more than one concept simultaneously.

          To take your "car" analogy for a second, you don't drive just a car. The interface presented to you delegates the actions you make to other components you don't have to think about yes, but at no point are the Engine Block, Steering Assembly, Chassis or Wheels wholly consumed by the "car". There are even those of us who prefer cars that make you *more* aware of this (manual transmission for e

      • Because you can always implement $customer->save in terms of a well factored interface, but going the other way is much harder.

        The interface that T::B would present is just one facet (albeit a very important one). It used to be that the tradeoff was too much java style boilerplate or a god object, but Moose's handles feature lets you have a nice interface on the outside and flexibility without needing combinatorial explosion on the inside.

        The point is that for 99% of the usage you can still use T::B beca

    • Personally I think delegates are a better solution for many of the things people use roles on a single object for.

      Don't forget that one of the primary purposes of roles is to encourage type-safe delegation.