Inheritence versus Composition

23 Aug 2014

Part of a healthy engineering culture is having a good code review process. There are many things that contribute to a good code review process, but one of those is thinking through why something should be the way it should. I recently did more writing than is usual which I've adapted to use as this post.

The code under review

We start with some code used to render columns of tickets in Sprint.ly's UI. Depending on where they are in the UI, these columns are sortable, you can drag & drop between them, or you can adjust which properties they are ordered by. The code looked something like this:

var ItemColumn = Backbone.View.extend(_.extend({
  render: function () {
    /* .. other stuff .. */
    this.initDraggable();
  }
}, draggable, droppable, sortable));

var droppable = {
  /* .. other stuff .. */
  initDroppable: function () { /* .. */}
}

The Python Equivalent might look something like this.

class ItemColumn(Backbone.View, Draggable, Droppable, Sortable):
    def render(self):
        this.initDraggable()

class Droppable(object):
    def initDroppable(self):
        # things would happen here        
        pass

This is a typical mixin approach, as other classes are "mixed in" to the inheritence heirarchy of the object. The result of this approach is the class gets additional methods and capabilities. There are some issues with this, as I explain below.

The alternative approach is what's called composition. This looks something like the code sample below. You'll notice the inheritence heirarchy is unchanged and it inherits from it's original base class. The classes that we would normally inherit from are instead passed in as arguments to the constructor of the object.

var ItemColumn = Backbone.View.extend({
  initialize: function (options) {
    /* .. other stuff .. */
    this.draggable = options.draggableHandler;
  },

  render: function () {
    /* .. other stuff .. */
    this.draggable.initDraggable();
  }
});

So why should we prefer composition over inheritence?

Dependencies of mixins are clearer.

One of the issues with the mixin approach is it makes the dependencies of this file ambiguous as to what things are necessary by this module versus what is necessary for the mixin. This means we could very easily have state on the object itself which serves no other purpose than to implicitly pass it to the mixin. As exemplified below, we're taking a dependency on a module higlightPlugin which isn't used by the made up TextRenderer class, but instead is used only for the mixin.

var mixin = {
  myMethod: function () {
    this.higlight.doWork();
  }
}

var TextRenderer = Backbone.View.extend(_.extend({
  initialize: function (highlightPlugin) {
    this.higlight = highlightPlugin; // not used in this class at all.
  }
}, mixin)

Doesn't break encapsulation.

By keeping these objects separated such that they don't share the same this object, we reduce possibilities when it comes to debugging. This means that you don't have to worry about calling into a library method like initDraggable and worry that it might be somehow altering your current object's state. This is an instance of the "inappropriate intimacy" code smell.

Easier to unit test.

As a side-effect of reducing intimacy with other modules and clearly documenting dependencies, the code becomes much easier to test. We can validate that the draggable is initted during the correct stage of the object's lifecycle by passing in a stub representation of the original object, and getting to ignore the underlying implementation.

No accidental conflicts in method names.

This is a minor one, but still worth mentioning. If two unrelated mixins happen to have the same name (such as render), as any other method in the inheritence chain, whichever is earlier in the list is the one that actually makes it onto the object. This isn't something that's possible with the composition-based approach.

Doesn't conflate the difference between identity and functionality.

This is a fuzzier one. For these purposes, let's think of objects as having a responsibility. What is the responsiblity of an Item Column? I would say that the responsibility is to render the contents of an item collection. This doesn't have provisions for managing the order of items in the collection (eg sortable). That would, in my mind, be the responsibility of an object which has control over each item's sort property. Similarly, adding and removing things to this collection (eg drag/drop) doesn't seem like the responsibility of this object.

What the mixin-based code's responsibilities are displaying the contents of the collection, removing / adding things from the backing collection based on user events, changing the sort value based on user action and loading additional information as necessary. That's a bunch of responsiblity for one object.

The way I think about it, we should delegate some of this responsibility to each of these objects. The sortable object should be responsibile for adjusting the position of things in response to user events. When the collection changes, the Column object would then maintain it's responsibility of rendering out the updated state. Any dragging and dropping would add to the collection itself, but would leave it to the column to render out this new content.

So when does inheritence make sense?

Inheritence has its place as well, however. I think they key distinction is it should be used when there's an "is-a" relationship, whereas "draggable", "droppable" and "sortable" describe behaviors of an object. An appropriate "is-a" would likely be Sprint.ly's quick sort bar which is an item column that displays results slightly differently, but is, by all other attributes, just a column of items. It has the same responsibility as a normal column (rendering a collection), but it's implementation is a bit different as well.

Conclusion

The result of this for our code was a bit of additional clarity on our own team's values, an opportunity to educate one another on pitfalls with a given approach, and setting direction for where the code should eventually end.