Re: Composition vs. Inheritance

On Mon, Feb 26, 2007 at 10:28:48PM -0500, Vadim Nasardinov wrote:
> On Monday 26 February 2007 08:35, Robbert Haarman wrote:
> > I am intrigued by the suggestion that composition is somehow better
> > than inheritance
> 
> It's not better.  Gamma's point is that people tend to view
> inheritance as the be-all and end-all of OOP to the significant
> exclusion of composition even when the latter is more appropriate.

Ah! Well, that is something I can definitely agree with.

> I apologize for sounding like a broken LP record, but Gamma's
> observation about the "tight coupling between the base class and the
> subclass" is best illustrated by Joshua Bloch's example:
> 
>   http://courses.dce.harvard.edu/~cscie160/EffectiveJava.htm#lec012
>   http://people.csail.mit.edu/gregs/ll1-discuss-archive-html/msg03912.html
> 
> Say, we want to implement a Set that counts the number of attempted
> element insertions:
> 
>  | import java.util.HashSet;
>  | import java.util.Collection;
>  |
>  | public class InstrumentedSet<E> extends HashSet<E> {
>  |     private int addCount = 0; // number of attempted element insertions
>  |
>  |     public InstrumentedSet() {}
>  |
>  |     public InstrumentedSet(Collection<? extends E> c) {
>  |         super(c);
>  |     }
>  |
>  |     public boolean add(E o) {
>  |         addCount++;
>  |         return super.add(o);
>  |     }
>  |
>  |     public boolean addAll(Collection<? extends E> c) {
>  |         addCount += c.size();
>  |         return super.addAll(c);
>  |     }
>  |
>  |     public int getAddCount() {
>  |         return addCount;
>  |     }
>  |
>  | }
> 
> Pretty straightforward.
> 
> The problem with this class is that it produces the wrong count under
> Sun's JDK.  Consider:
> 
>  | import java.util.Arrays;
>  |
>  | public class Main {
>  |     public static void main(String[] args) {
>  |         InstrumentedSet<String> set = new InstrumentedSet<String>();
>  |         set.addAll(Arrays.asList(new String[] {"foo", "bar", "baz"}));
>  |         System.out.printf("Number of attempted insertions: %d%n",
>  |                           set.getAddCount());
>  |     }
>  | }
> 
> This prints:
> 
>  | $ java -showversion Main
>  | java version "1.6.0"
>  | Java(TM) SE Runtime Environment (build 1.6.0-b105)
>  | Java HotSpot(TM) 64-Bit Server VM (build 1.6.0-b105, mixed mode)
>  |
>  | Number of attempted insertions: 6
> 
> The reason for this is because the #addAll method is implemented like
> so in the superclass:
> 
>     | public boolean addAll(Collection<? extends E> c) {
>     |     boolean changed = false;
>     |     for (E elem : c) {
>     |         if (this.add(elem)) { changed = true; }
>     |     }
>     |     return changed;
>     | }
> 
> In other words, #addAll is implemented in terms of the overridable
> method #add.  Since we've overridden #add in InstrumentedSet, the
> overriden implementation of #addAll ends up counting each element
> twice.

So, a better solution would have been to implement just the basic 
operations (e.g. add) as part of the class definition, and the higher 
level operations in terms of the basic ones. That way, it would have 
been clear that you need have overridden only add.

The problem with this is, of course, that the above solution would be 
somewhat kludgy in Java. Instead of getting

aSet.add(anItem);
aSet.addAll(items);

you would get

aSet.add(anItem);
SetUtils.addAll(aSet, items);

> More abstractly, the problem is that callbacks are evil.

*chokes*

On the contrary, callbacks are very useful and elegant.

> Suppose you have components X and Y such that X's function foo()
> relies on Y's function bar().  Unbeknownst to X, Y's implementation of
> bar() relies on X's function baz().  In other words, suppose we have:
> 
>  | X:
>  |
>  |   function foo() {
>  |       temporarilyViolateInvariantsOfComponentX();
>  |       result = Y.bar();
>  |       restoreInvariantsOfComponentX(result);
>  |   }
>  |
>  |   // Precondition: X's invariants hold at entry into baz()
>  |   function baz() {
>  |       doStuffNaivelyExpectingInvariantsToHold();
>  |   }
>  |
>  | Y:
>  |   function (bar) {
>  |       partialResult = X.baz();
>  |       return finishComputation(partialResult);
>  |   }

Is that what you meant by "callbacks"?

> Y calls back into X at a time when X has its invariants temporarily
> messed up.  Hosage ensues.

Of course, this is entirely the fault of X giving control to code that 
it doesn't know while X is in an invalid state. It has little to do with 
anything being evil, and more with X being buggy.

> Inheritance is just a particular example of the evilness of callbacks.

Nah, just because there are some pathological cases doesn't mean the 
whole feature is useless. I think inheritance is very useful, if 
frequently overused by Java programmers.

> Possible solutions:
> 
>  1. Use composition if possible.  In the case of InstrumentedSet,
>     composition solves the problem nicely, if somewhat verbosely in
>     Java.

You mean something that contains a set, rather than extending one, and 
then just implements

add(item) { count++; set.add(item); }
addAll(items) { count += items.length; set.addAll(items); }

etc.?

That would clearly be a better solution than the original code. It would 
also allow anything implementing the Set interface to be used, rather 
than hard wiring the choice to HashSet.

>     (In OOP languages that provide the moral equivalent of
>     Smalltalk's doesNotUnderstand message, delegation is much terser.)

Care to give an example of that?

>  2. Tightly control the extensibility points.  Basic pattern:

Yuck, tying the hands of other programmers...

>     | public abstract class WithResource {
>     |     private final ResourceManager manager;
>     |
>     |     protected WithResource(ResourceManager manager) {
>     |         if (manager == null) { throw new NullPointerException("manager"); }
>     |         this.manager = manager;
>     |     }
>     |
>     |     public final void run() {
>     |         Resource resource = manager.acquireResource();
>     |         try {
>     |             doWork();
>     |         } finally {
>     |             manager.release(resource);
>     |         }
>     |     }
>     |
>     |     protected abstract doWork(Resource resource);
>     | }
> 
>     Intended usage:
> 
>        | new WithResource(resourceManager) {
>        |     public void doWork(Resource resource) {
>        |         // do work with 'resource'
>        |     }
>        | }.run();

By the way, this is an actual example of callbacks. Specifically, 
manager.acquireResource and manager.release are callbacks, as is doWork. 
Surely you didn't intend to say these are evil?

>  3. Avoid self-use.
> 
>     Don't implement public overridable methods in terms of other
>     public overridable methods in the same class.  In the above Set
>     example, the superclass could (should?) have been implemented like
>     so:
> 
>     | public boolean addAll(Collection<? extends E> c) {
>     |     boolean changed = false;
>     |     for (E elem : c) {
>     |         if (this.__add(elem)) { changed = true; }
>     |     }
>     |     return changed;
>     | }
>     |
>     | public boolean add(E elem) {
>     |     return this.__add(elem);
>     | }
>     |
>     | // not overridable
>     | private boolean __add(E elem) {
>     |     // add the element, if not already present
>     |     // if already present, return false.  otherwise, true
>     | }

I don't know about anything else, but I find the above ugly. For 
elegance, addAll _should_ be implemented in terms of add.

> Of these three options, Option #3 smacks of violated encapsulation.
> For your class to be safely subclassable, you have to inform your
> potential subclassers whether or not there is any self-use going on in
> your current superclass implementation.  This reveals an
> implementation detail that should, in theory, be hidden.

Which is exactly the problem the original code ran into. It guessed 
addAll wasn't implemented in terms of add, but that guess turned out to 
be wrong. Had it guessed the other way, it might have been wrong as 
well. There was no way to know which way things really were. Worse, 
(assuming such info is not in the specification), there is no way to 
know what is true in the developer's environment now will be true in all 
environments the code ever runs in. Clearly an undesirable situation.

> None of this is specific to Java (other than verboseness).

True. On the other hand, the way things are expressed does matter. For 
example, I could easily imagine Java programmers have more of a tendency 
to lump many actions together in a single method (reducing 
composibility) than have OCaml programmers, because of all the 
boilerplate Java imposes on method definition.

Regards,

Bob

-- 
I'm a dyslexic agnostic with insomnia... I lie awake at night wondering
if there really is a dog!

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)

iD8DBQFF4+/gfb9wcmD+WN4RAqeQAJ9hKygZ4mH52LHSEsn0ZFMwuJcFMQCfcgQn
19ewnTGzmYdiFAGkJrCtaPA=
=Fq6j
-----END PGP SIGNATURE-----