Thursday, May 5, 2011

DRY vs. "prefer containment over inheritance"

There is a general rule of OO design that you should model is-a relationships using inheritance and has-a relationships using containment/aggregation and forwarding/delegation. This is further narrowed by the admonishment from the GoF that you should generally favor containment over inheritance, suggesting, perhaps, that if you could make a strong case for either one in a particular situation, that containment should generally get the nod due to the maintenance problems inheritance can sometimes cause.

I understand the reasoning behind this thinking, and I don't necessarily disagree with it. However, when I see a class with scores of methods, each just forwarding to some instance variable, I see a form of code duplication. Code duplication is, in my opinion, the ultimate code smell. Reimplementing an enormous protocol of methods just because the relationship between two classes isn't strictly is-a seems like overkill. It is additional, unnecessary code added to the system, code that now needs be tested and documented like any other part of the system--code that you likely wouldn't have had to write if you just inherited.

Do the costs of adhering to this containment-over-inheritance principal ever outweigh its benefits?

From stackoverflow
  • Yeah, what you're seeing there is a horrific collision of design paradigma from different corners of the universe: the GoF's aggregation/composition leveraging colliding with the "Law of Demeter".

    I am on record as believing that, in context of aggregation and composition use, the Law of Demeter is an anti-pattern.

    Contrary to it, I believe that constructs like person->brain->performThought() are absolutely right and appropriate.

  • The cost of virtually anything can outweigh its benefits. Having hard and fast, NO EXCEPTIONS rules will always get you into trouble with development. In general, it's a bad idea to use (if your language/runtime support it) reflection in order to gain access to variables that aren't intended to be visible to your code. Is this a bad practice? As with everything,

    it depends.
    

    Can composition sometimes be more flexible or easier to maintain than straight-up inheritance? Sure. That's why it exists. In the same way, inheritance can be more flexible or easier to maintain than a pure composition architecture. Then you have interfaces or (depending on the language) multiple inheritance.

    None of these concepts are bad, and people should be conscious of the fact that sometimes our own lack of understanding or resistance to change can cause us to create arbitrary rules that define something as "bad" without any real reason to do so.

    toby : Couldn't agree more. While it seems to be the natural tendency of engineers to want to find the "one true way", the reality is that everything depends on its context, there is just not a right answer for everything. And *that*, is the right answer for everything.
  • I agree with your analysis, and would favour inheritance in those cases. It seems to me that this is a bit the same thing as blindly implementing stupid accessors in an naive effort to provide encapsulation. I think the lesson here is that there simply isn't any universal rules that always apply.

  • This might not answer your question but there's something that has always bothered me about Java and its Stack. Stack, as far as my knowledge scopes, should be a very simple (or probably the most simple) container data structure with three basic public operations: pop, push and peek. Why would you want in a Stack insertAt, removeAt, et al. Kind of functionality? (in Java, Stack inherits from Vector).

    One may say, well, at least you don't have to document those methods, but why having methods that aren't supposed to be there on the first place?

    Emil H : Yes, this is really of-topic. But I do agree that the bloated interface of java.util.Stack is annoying.
    Min : This is a pretty interesting observation on some of the pitfalls of inheritance though. If your answer was rephrased, this would be pretty on topic. The Java stack could have contained a Vector and then only has pop, push, and peek. On another note, I think having more flexible data structures in the BCL is a good thing.
  • GoF is old text by now.. Depends on what OO environment you look at (and even then it is obvious you can come across it in OO-unfriendly pieces such as C-with-classes).

    For runtime environments you pretty much have no choice, single inheritance. And any workaround attempted to circumvent its limitations is just that, no matter how sophisticated or 'cool' it might seem.

    Again you will see this manifested everywhere, inclusive of C++ (most capable one) where it interfaces with C callbacks (which is widespread enough to make anyone pay attention). C++ however offers you mix-ins and policy-based designs with template features so it can occasionally help for hard engineering.

    While containment can give you benefits of indirection, inheritance can give you easily accessible composition. Pick you poison.. aggregation ports better but always looks like violation of DRY while inheritance can lead to easier reuse with different set of potential maintainance headaches.

    The real problem is that the computer language or modelling tool should give you an option what it ought to do independent of choice and as such make it less human error-prone; but not many people model before letting computers write programs for them + no good tools are around (Osla certainly isn't one) or their environment is pushing something as silly as reflection, IoCs and what not.. which is very popular and that in itself tells a lot.

    There was once a piece done for an ancient COM tech that was called Universal Delegator by one of the best Doom players around, but it isn't the kind of development anyone would adopt these days.. It required interfaces sure (and not a real-hard-requirement for general case). The idea is simple, it predates all the way to interrupt handling.. Only similar aproaches give you the best of both worlds and they are somewhat evident in scripted pieces suches as JavaScript and functional programming (although far less readable or performing).

  • You've got to pick the right solution to the problem at hand. Sometimes inheritence IS better than containment, sometimes not. You have to use your judgment, and when you can't figure out which way to go, write a little code and see how bad it gets. Sometimes writing some code can help you make a decision that's not obvious otherwise.

    As always: the right answer depends on so many factors that you can't make hard-and-fast rules.

    chaos : Some people do. I can kinda see it. It's very relaxing compared to actual work.
    Michael Kohne : I've seen the worst of both ways. One app I deal with has a main class that inherits from 20 (no, I'm not kidding) mostly (but not completely) abstract classes. 80-90% of the functions in that class just call another function. Lots of inheritance AND lots of boilerplate! I try VERY hard not work with that app...
  • To some degree, this is a question of language support. For example, in Ruby we could implement a simple stack that uses an array internally like so:

    class Stack
      extend Forwardable
      def_delegators :@internal_array, :<<, :push, :pop
      def initialize() @internal_array = [] end
    end
    

    All we're doing here is declaring what subset of the other class's functionality we want to use. There's really not a lot of repetition. Heck, if we actually wanted to reuse all of the other class's functionality, we could even specify that without actually repeating anything:

    class ArrayClone
      extend Forwardable
      def_delegators(:@internal_array, 
                      *(Array.instance_methods - Object.instance_methods))
      def initialize() @internal_array = [] end
    end
    

    Obviously (I hope), that's not code I would generally write, but I think it shows that it can be done. In languages without easy metaprogramming, it can be somewhat harder in general to keep DRY.

  • While I agree with everyone who has said "it depends" -- and that it's also language-dependent to a degree -- I'm surprised no one has mentioned that (in the famous words of Allen Holub) "extends is evil". When I first read that article I have to admit I got a little put off, but he's right: regardless of the language, the is-a relationship is about the tightest form of coupling there is. Tall inheritance chains are a distinct anti-pattern. So while it's not right to say you should always avoid inheritance, it should be used sparingly (for classes -- interface inheritance is recommended). My object-orientation-noob tendency was to model everything as an inheritance chain, and yes, it does reduce code duplication, but at a very real cost of tight coupling, which means inevitable maintenance headaches somewhere down the road.

    His article is much better at explaining why inheritance is tight coupling, but the basic idea is that is-a requires every child class (and grandchild and so on) to depend on the implementation of the ancestor classes. "Programing to the interface" is a well-known strategy for reducing complexity and assisting with agile development. You can't really program to the interface of a parent class because the instance is that class.

    On the other hand, using aggregation/composition forces good encapsulation, making a system much less rigid. Group that reusable code into a utility class, link to it with a has-a relationship, and your client class is now consuming a service provided according to a contract. You can now refactor the utility class to your heart's content; as long as you conform to the interface, your client class can remain blissfully unaware of the change, and (importantly) it shouldn't have to be recompiled.

    I'm not proposing this as a religion, just a best practice. They're meant to be broken when needed, of course, but there's generally a good reason that "best" is in that term.

  • You could consider implementing the "decorator" code in an abstract base class that (by default) forwards all method calls to the contained object. Then, subclass the abstract decorator and override/add methods as necessary.

    abstract class AbstractFooDecorator implements Foo {
        protected Foo foo;
    
        public void bar() {
            foo.bar();
        }
    }
    
    class MyFoo extends AbstractFooDecorator {
        public void bar() {
            super.bar();
            baz();
        }
    }
    

    This at least eliminates repetition of the "forwarding" code, if you've got many classes wrapping a specific type.

    As for whether or not the guideline is a useful one, I suppose emphasis should be placed on the word "prefer". Obviously there will be cases where it makes perfect sense to use inheritance. Here's an example of when inheritance should not have been used:

    The Hashtable class was enhanced in JDK 1.2 to include a new method, entrySet, which supports the removal of entries from the Hashtable. The Provider class was not updated to override this new method. This oversight allowed an attacker to bypass the SecurityManager check enforced in Provider.remove, and to delete Provider mappings by simply invoking the Hashtable.entrySet method.

    The example highlights that testing is still required for classes in an inheritance relationship, contrary to the implication that one only needs to maintain/test "encapsulating"-style code -- the cost of maintaining a class that inherits from another might not be as cheap as it first appears.

0 comments:

Post a Comment