36

From : http://www.artima.com/lejava/articles/designprinciples4.html

Erich Gamma: I still think it's true even after ten years. Inheritance is a cool way to change behavior. But we know that it's brittle, because the subclass can easily make assumptions about the context in which a method it overrides is getting called. There's a tight coupling between the base class and the subclass, because of the implicit context in which the subclass code I plug in will be called. Composition has a nicer property. The coupling is reduced by just having some smaller things you plug into something bigger, and the bigger object just calls the smaller object back. From an API point of view defining that a method can be overridden is a stronger commitment than defining that a method can be called.

I don't understand what he means. Could anyone please explain it?

jpaugh
  • 201
  • 1
  • 7
q126y
  • 1,713
  • 3
  • 14
  • 24

5 Answers5

63

A commitment is something that reduces your future options. Publishing a method implies that users will call it, therefore you can't remove this method without breaking compatibility. If you'd kept it private, they couldn't (directly) call it, and you could some day refactor it away without problems. Therefore, publishing a method is a stronger commitment than not publishing it. Publishing an overridable method is an even stronger commitment. Your users can call it, and they can create new classes where the method doesn't do what you think it does!

For instance, if you publish a clean-up method, you can ensure that resources are properly deallocated as long as users remember to call this method as the last thing they do. But if the method is overridable, someone might override it in a subclass and not call super. As a result, a third user might use that class and cause a resource leak even though they dutifully called cleanup() at the end! This means that you can no longer guarantee the semantics of your code, which is a very bad thing.

Essentially, you can no longer rely on any code running in user-overridable methods, because some middleman might override it away. This means that you have to implement your clean-up routine entirely in private methods, with no help from the user. Therefore it's usually a good idea to publish only final elements unless they are explicitly intended for overriding by API users.

Null
  • 125
  • 1
  • 9
Kilian Foth
  • 107,706
  • 45
  • 295
  • 310
  • 11
    This is possibly the best argument against inheritance that I've ever read. Of all the reasons against it that I've encountered, I've never come across these two arguments before (coupling and breaking functionality through overriding), yet both are very powerful arguments against inheritance. – David Arno Dec 04 '15 at 16:34
  • 1
    This problem is even more interesting (or terrifying?) if you consider not just a single inheritance, but a chain of them. Or a complicated mess of inheritances - at least with a linear chain it's "clear" what the potential minefield is. – enderland Dec 04 '15 at 17:38
  • 5
    @DavidArno I don't think it's an argument against inheritance. I think it's an arguments against "make everything overridable by default". Inheritance is not dangerous by itself, using it without thought is. – svick Dec 04 '15 at 19:35
  • A solution to the cleanup issue would be to have a private cleanup that at the end calls a user-specifiable cleanup. – JAB Dec 04 '15 at 20:23
  • 15
    While this sounds like a good point I can't really see how "a user may add his own buggy code" is an argument. Enabling inheritance allows users to add lacking functionality without loosing updateability, a measure which can prevent and fix bugs. If an users code atop of your API is broken it its not an API flaw. – Sebb Dec 04 '15 at 20:40
  • 4
    You could easily turn that argument around into: the first coder makes a cleanup argument, but makes mistakes and doesn't clean everything up. The second coder overrides the cleanup method and does a good job, and coder #3 uses the class and doesn't have any resource leaks even though coder #1 made a mess of it. – Pieter B Dec 05 '15 at 09:07
  • 2
    @svick, you are of course entitled to that opinion. In my view, this is just one of a great many arguments against ever opting for inheritance to anything. To that extent, I never use it unless I'm working with an API that forces me to. I even go as far as marking all my public classes as final/sealed so they cannot be inherited from. A step too far in the minds of some, but the nature conclusion to reach regarding inheritance in my view. – David Arno Dec 05 '15 at 11:41
  • 3
    @Sebb You can't just blindly allow every method to be overridable. For every virtual method the circumstances under which it'll be called (which methods call it, which methods don't, and what the state of the object will be at the time) become part of the public API. If every method is virtual you *must* maintain these conditions for every single pair of methods (e.g. foo can't suddenly start calling bar, or stop calling baz) See [this paper](https://www.cs.cmu.edu/~aldrich/papers/selective-open-recursion.pdf) for details. You must pick which methods should be virtual and *nail down the rest*. – Doval Dec 05 '15 at 17:41
  • 3
    @Sebb There's also the issue that single (class) inheritance doesn't compose. I've got some functionality in class A and some other functionality in class B, but I can't inherit from both A and B. All in all inheritance makes a very poor *default*. It's something you should only reach for only after careful consideration. – Doval Dec 05 '15 at 17:46
  • 6
    @Doval Indeed. Which is why it's a travesty that inheritance is lesson number one in just about every introductory OOP book and class. – Kevin Krumwiede Dec 06 '15 at 06:41
  • Considering protected access level of fields the overridden method can access, opens more possibilities for derived classes to invalidate assumptions the base class made. – AaronLS Dec 06 '15 at 20:35
  • @Doval I'm not saying everything should be virtual by default. My point is that a few key functions/classes marked as virtual will make some users lives far easier and should not be disallowed just because "a user may screw it up". – Sebb Dec 07 '15 at 09:14
  • If you don't permit your class to be inherited from, people using your code will use composition instead. This comes with all the same possibilities of someone forgetting to cleanup. You don't gain anything in this regard by avoiding inheritance. – eigensheep Dec 31 '15 at 23:04
30

If you publish a normal function, you give a one-sided contract:
What does the function do if called?

If you publish a callback, you also give a one-sided contract:
When and how will it be called?

And if you publish an overridable function, it's both at once, so you give a two-sided contract:
When will it be called, and what must it do if called?

Even if your users aren't abusing your API (by breaking their part of the contract, which might be prohibitively expensive to detect), you can easily see that the latter needs far more documentation, and everything you document is a commitment, which limits your further choices.

An example of reneging on such a two-sided contract is the move from show and hide to setVisible(boolean) in java.awt.Component.

Deduplicator
  • 8,591
  • 5
  • 31
  • 50
  • +1. I'm not sure why the other answer was accepted; it makes some interesting points, but it's definitely not the right answer to *this* question, in that it's definitely not what is meant by the quoted passage. – ruakh Dec 05 '15 at 18:47
  • This is the correct answer, but I don't understand the example. Replacing show and hide with setVisible(boolean) seems to break code that doesn't use inheritance too. Am I missing something? – eigensheep Dec 31 '15 at 23:09
  • 3
    @eigensheep: `show` and `hide` still exist, they're just `@Deprecated`. So the change doesn't break any code that merely invokes them. But if you've overridden them, your overrides will not be called by clients who migrate to the new 'setVisible'. (I've never used Swing, so I don't know how common it is to override those; but since it happened a long time ago, I imagine the reason that Deduplicator remembers it is that it bit him/her painfully.) – ruakh Jan 01 '16 at 01:53
13

Kilian Foth's answer is excellent. I'd just like to add the canonical example* of why this is a problem. Imagine an integer Point class:

class Point2D {
    public int x;
    public int y;

    // constructor
    public Point2D(int theX, int theY) { x = theX; y = theY; }

    public int hashCode() { return x + y; }

    public boolean equals(Object o) {
        if (this == o) { return true; }
        if ( !(o instanceof Point2D) ) { return false; }

        Point2D that = (Point2D) o;

        return (x == that.x) &&
               (y == that.y);
    }
}

Now let's sub-class it to be a 3D point.

class Point3D extends Point2D {
    public int z;

    // constructor
    public Point3D(int theX, int theY, int theZ) {
        super(x, y); z = theZ;
    }

    public int hashCode() { return super.hashCode() + z; }

    public boolean equals(Object o) {
        if (this == o) { return true; }
        if ( !(o instanceof Point3D) ) { return false; }

        Point3D that = (Point3D) o;

        return super.equals(that) &&
               (z == that.z);
    }
}

Super simple! Let's use our points:

Point2D p2a = new Point2D(3, 5);
Point2D p2b = new Point2D(3, 5);
Point2D p2c = new Point2D(3, 7);

p2a.equals(p2b); // true
p2b.equals(p2a); // true
p2a.equals(p2c); // false

Point3D p3a = new Point3D(3, 5, 7);
Point3D p3b = new Point3D(3, 5, 7);
Point3D p3c = new Point3D(3, 7, 11);

p3a.equals(p3b); // true
p3b.equals(p3a); // true
p3a.equals(p3c); // false

You are probably wondering why I'm posting such an easy example. Here's the catch:

p2a.equals(p3a); // true
p3a.equals(p2a); // FALSE!

When we compare the 2D point to the equivalent 3D point, we get true, but when we reverse the comparason, we get false (because p2a fails instanceof Point3D).

Conclusion

  1. It is usually possible to implement a method in a subclass in such a way that it's no-longer compatible with how the super-class expects it to work.

  2. It is generally impossible to implement equals() on a significantly different subclass in a way that is compatible with it's parent class.

When you write a class that you intend to allow people to subclass, it's a really good idea to write out a contract for how each method should behave. Even better would be a set of unit tests people could run against their implementations of overridden methods to prove that they do not violate the contract. Almost nobody does that because it's too much work. But if you care, that's the thing to do.

A great example of a well-spelled out contract is Comparator. Just ignore what it says about .equals() for the reasons described above. Here's an example of how Comparator can do things .equals() can't.

Notes

  1. Josh Bloch's "Effective Java" Item 8 was the source of this example, but Bloch uses a ColorPoint which adds a color instead of a third axis and uses doubles instead of ints. Bloch's Java example is basically duplicated by Odersky/Spoon/Venners who made their example available online.

  2. Several people have objected to this example because if you let the parent class know about the sub-class, you can fix this issue. That's true if there are a small enough number of sub-classes and if the parent knows about them all. But the original question was about making an API which someone else will write sub-classes for. In that case, you generally can't update the parent implementation to be compatible with the sub-classes.

Bonus

Comparator is also interesting because it works around the issue of implementing equals() correctly. Better yet, it follows a pattern for fixing this type of inheritance issue: the Strategy design pattern. The Typeclasses that Haskell and Scala people get excited about are also the Strategy pattern. Inheritance isn't bad or wrong, it's just tricky. For further reading, check out Philip Wadler's paper How to make ad-hoc polymorphism less ad hoc

GlenPeterson
  • 14,890
  • 6
  • 47
  • 75
  • 1
    SortedMap and SortedSet don't actually change the definitions of `equals` from how Map and Set define it, though. Equality completely ignores the ordering, with the effect that, for example, two SortedSets with the same elements but different sort orders still compare equal. – user2357112 Dec 04 '15 at 18:08
  • 1
    @user2357112 You are right and I have removed that example. SortedMap.equals() being compatible with Map is a separate issue which I will proceed to complain about. SortedMap is generally O(log2 n) and HashMap (the canonical implmementation of Map) is O(1). Therefore, you would only use a SortedMap if you *really* care about ordering. For that reason I believe the order is important enough to be a critical component of the equals() test in SortedMap implementations. They should not share an equals() implementation with Map (they do through AbstractMap in Java). – GlenPeterson Dec 04 '15 at 18:18
  • 1
    Josh Bloch goes into detail (he might even use these same examples) in his book when he laments the confusing contract of `Object.equals()` in Java. –  Dec 04 '15 at 20:22
  • 3
    "Inheritance isn't bad or wrong, it's just tricky." I understand what you're saying, but tricky things typically lead to errors, bugs, and problems. When you can accomplish the same things (or nearly all the same things) in a more reliable way, then the more tricky way *is* bad. – jpmc26 Dec 05 '15 at 00:31
  • 7
    This is an **awful** example, Glen. You just used inheritance in a way it should not be used, no wonder the classes do not work the way you intended. You broke the Liskov's substitution principle by providing a wrong abstraction (the 2D point), but just because inheritance is bad in your wrong example does not mean it is bad in general. Although this answer might look reasonable, it will only confuse people who do not realize it breaks the most basic inheritance rule. – Andy Dec 05 '15 at 08:28
  • @jpmc26 great point: "The more tricky way is bad!" Amen! But what is the more reliable way you mention? – GlenPeterson Dec 05 '15 at 15:40
  • It depends on the specific situation. Sometimes an interface or composition/callbacks are good options. In your example, I would either just segregate Point2D and Point3D completely (as they don't really have the relationship inheritance suggests) or I would have a single, arbitrary dimension class that doesn't consider points with different numbers of dimensions equal. You might could have a method that tests for "equality" with a subset of the dimensions, but this would be a very explicit method so the caller knows what they're getting. (It would be transitive and symmetric.) – jpmc26 Dec 05 '15 at 19:24
  • I agree with David Packer. `Point3D` would work *exactly* the same if you dropped the `extends Point2D`. It's inheritance in name only. – chepner Dec 06 '15 at 02:20
  • 1
    @chepner and @DavidPacker I've changed the examples to use super() and rely on the super-class as much as possible. It's functionally the same as it was before, just leverages inheritance more. Any implementing class can break Liskov Substitution and you can design an interface that is impossible to inherit from correctly in all circumstances. `.equals()` is such a method on java.lang.Object. Comparator (and the Strategy pattern) work around the problems with `equals()` but it's more work to use. http://glenpeterson.blogspot.com/2013/09/object-equality-is-context-relative.html – GlenPeterson Dec 06 '15 at 12:28
  • 1
    @DavidPacker I'm missing something here. If you project the 3D point onto a plane (e.g. Z = 0), it can theoretically be meaningfully compared with other 2D points on that plane. But in 3D space, the comparison is meaningless because the 2D point lacks information in that context (it lacks a Z coordinate). Each meaningful field in a class can be thought of as a comparison context. If you add an important field in a sub-class, you add a context which breaks the equals contract between sub-class and super-class. It breaks Substitution. Usually you subclass to add a meaningful field... – GlenPeterson Dec 06 '15 at 12:49
  • Equality is symmetric. If `p2a.equals(p3a)` is true, then `p3a.equals(p2a)` should be as well. Your implementation is broken. – chepner Dec 06 '15 at 15:58
  • 3
    ELI5 of Liskov's Substituton Principle says: *Should a class `B` be a child of a class `A` and should you instantiate an object of a class `B`, you should be able to cast the class `B` object to it's parent and use the casted variable's API without losing any implementation details of the child.* You broke the rule by providing the third property. How do you plan to access the `z` coordinate after you cast the `Point3D` variable to `Point2D`, when the base class has no idea such property exists? If by casting a child class to its base you break the public API, your abstraction is wrong. – Andy Dec 06 '15 at 16:00
  • Note, there is absolutely nothing wrong with comparing the `Point3D` to `Point2D` via the `equals()` method. You can implement it and it should and will work in some conditions, if you want. The problem is the abstraction. Many (note: unsupported by facts) developers say: *Favor composition over inheritance.* You can use the `Point2D` to build the one fo 3D layer. [Here's a quick C# example](http://pastebin.com/0NPF9wTQ) of the correct approach, when the `Point3D` class uses the `Point2D`. Notice, how I avoided code duplication in the Equals method, even without the use of inheritance. – Andy Dec 06 '15 at 16:26
  • @DavidPacker In your example, you work around the issue by letting Point2D know about the Point3D subclass. The OP question was about why it's a stronger commitment to define an API method to be overridden, which implies you don't have knowledge of all sub-classes. Designing a class and sub-class together is a much easier problem than making a public API for other people to subclass, for exactly this reason. – GlenPeterson Dec 06 '15 at 17:06
  • I didn't really mean to tie my comments to OP's question, I was only providing a feedback on your given example, which is a bad representation of the solution, mostly because it shows the wrong way to use inheritance in an OO language. I don't really think your answer provides answer to OP's question either, it shows a wrong use of inheritance, but does not show the limitance of the contract because of it, which is what OP wanted to know. – Andy Dec 06 '15 at 19:42
4

Inheritance Weakens Encapsulation

When you publish an interface with inheritance permitted, you substantially increase the size of your interface. Each overrideable method could be replaced and so should be thought of as callback provided to the constructor. The implementation provided by your class is merely the default value of the callback. Thus, some kind of contract should be provided indicating what the expectations on the method are. This seldom happens and is a major reason why object oriented code is called brittle.

Below is a real (simplified) example from the java collections framework, courtesy of Peter Norvig (http://norvig.com/java-iaq.html).

Public Class HashTable{
    ...
    Public Object put(K key, V value){
        try{
            //add object to table;
        }catch(TableFullException e){
            increaseTableSize();
            put(key,value);
        }
    }
}

So what happens if we subclass this?

/** A version of Hashtable that lets you do
 * table.put("dog", "canine");, and then have
 * table.get("dogs") return "canine". **/

public class HashtableWithPlurals extends Hashtable {

    /** Make the table map both key and key + "s" to value. **/
    public Object put(Object key, Object value) {
        super.put(key + "s", value);
        return super.put(key, value);
    }
}

We have a bug: Occasionally we add "dog" and the hashtable gets an entry for "dogss". The cause was someone providing an implementation of put that the person designing the Hashtable class did not expect.

Inheritance Breaks Extensibility

If you allow your class to be subclassed, you are committing to not add any methods to your class. This could otherwise be done without breaking anything.

When you add new methods to an interface, anyone who has inherited from your class will need to implement those methods.

eigensheep
  • 448
  • 4
  • 6
3

If a method is meant to be called, you only have to ensure it works correctly. That's it. Done.

If a method is designed to be overridden, you also have to think carefully about the scope of the method: if the scope is too large, child class will often need to include copy-pasted code from parent method; if it is too small, many methods will need to be overridden to have desired new functionality - this adds complexity and needless line count.

Therefore creator of parent method needs to make assumptions on how the class and its methods might be overridden in the future.

However, the author is talking about a different issue in the quoted text:

But we know that it's brittle, because the subclass can easily make assumptions about the context in which a method it overrides is getting called.

Consider method a which is being normally called from method b, but in some rare and non-obvious cases from method c. If author of overriding method overlooks the c method and its expectations on a, it's obvious how things can go wrong.

Therefore it's more important that a is defined clearly and unambiguously, well documented, "does one thing and does it well" -- more so than if it were a method only designed to be called.

Rainy
  • 314
  • 1
  • 4