Sunday, April 01, 2007

Ruby: [Anti-pattern] Extract Module to shorten Class Definition

A Ruby module can be used to encapsulate a role that an object might play. For example, if an object wants to delegate, it can extend Forwardable.

Unfortunately, more than once I've seen an (anti)pattern of methods being moved into a module to shorten a class definition. This anti-pattern can be fairly easy to spot.
  • Do you have a module that only one class includes (or extends).
  • Do you have a module that requires other methods to be defined on the class that includes it.
I understand the motivation, when the class definition is getting a bit long and hard to follow it might make sense to take a group of similar methods and move them to a module. The problem with this refactoring is that it is possibly making the situation worse. If a class definition is already beginning to run long, it's possible that the class may be doing too much. Simply moving behavior into a module will not address this problem, and now if someone wants to see what an objects responsibilities are they have to look in more than one place.

A better solution could be to extract a class that encapsulates a subset of the responsibilities. If the behavior is required in various classes then it's probably time to look at converting the class to a module.

3 comments:

  1. Amen to this Jay. I think Modules should be treated mostly as mixins, as pieces of functionality that transparently modify the way a class works.

    When you are including a module just to use its methods your class becomes less ovious because in your code you're calling methods that don't exist in the class and you have to go hunting for them elsewhere. All you've done is split the class up and made it harder to follow the logic.

    Whereas if you are including a module like Enumerable you are actually including transparent functionality into your class which is the way modules should work.

    ReplyDelete
  2. Jay,

    I actually found myself running this one on Grabb.it. I have a TrackPresenter I use to render the XSPF for tracks in our APIs, as well as a PlaylistPresenter thar renders collections of tracks. I seperated the Presenter modules from my models because I didn't want to see Builder::XML code all over my ActiveRecord objects.

    It turned out to be a boon when I created another class that uses PlaylistPresenter, although TrackPresenter still closely follows the anti-pattern. I guess I'm just saying it's not as cut-and-dry as it could seem, and that putting walls between concerns can have design benefits.

    ReplyDelete
  3. I think the trick is to look at your motivation modulization. Splitting a class into modules can be thought of as a poor man's answer to Smalltalk's message categories - methods that have similar motivations get grouped into appropriate categories which you can use to filter the method list in the code browser.

    However, if that's you're reason for breaking the class into modules, you're almost certainly better keeping everything in a single file - you can then use code folding to hide the categories you're not currently interested in.

    There's also a case, I think, for pulling your class methods out into another category, and, quite possibly into another file at the same time.

    ActiveRecord's motivation for splitting the class into multiple modules is so that behaviour can be layered onto the Base functionality. On at least one occasion I've been grateful for this approach because, simply by implementing the required protocol I've been able to mix ActiveRecord::Errors and ActiveRecord::Validations into my own classes and gained great deal of handy functionality. (This would be a _lot_ easier if the 'layering' modules documented the interface they required from classes they are mixed into).

    ReplyDelete

Note: Only a member of this blog may post a comment.