It's OK To Make Mistakes (Re: Cedric on Super)

January 29, 2004 11:33 AM

Cedric: Don't Call Super

I'll restate my point: whenever you feel the need to call super inside a method that is not a constructor, it's a code smell. If on top of that, this method can be overridden by subclasses, you absolutely need to get rid of that constraint because I guarantee you that someone (a user or even yourself) will break that contract.

How do you solve this problem? With a technique called "hooks".

(Where "hooks" is a restatement of the GoF Template Method pattern)

I'm going to stand up for overriding methods here. It's simple, it works, and generally you can catch and fix any resultant errors very quickly (and beat the programmer responsible over the head with a clue-bat until they promise never to do it again1). Template methods are only really useful when the superclass has some very specific behaviour it must demand the subclass provide. When the behaviour is optional, unpredictable, or may spread across multiple subclasses, template methods don't sit comfortably any more.

That said, I always thought there should be some kind of chain-of-responsibility for JUnit setup and tear-down. It'd be neat if you could optionally choose to have each test class add a setup command object to the chain, and then the test-runner can ensure the entire chain gets run. The problem, though, comes from generalising this specific problem (setUp() in JUnit can be problematic if you have big test-class hierarchies) to the general. (Don't override methods with the intent of calling super())

Just because a particular coding technique can be dangerous doesn't mean that it's a priori a Bad Thing. Read Javablogs for a day or two, and you'll always find a handful of articles saying "It's possible to make a mistake if you do X. Therefore, don't do X."

I wonder if this is a cultural peculiarity to Java? After all, Java was designed specifically to not be able to do a lot of the things that cause problems in C++ programs. Maybe this initial success makes us feel that we can solve all programming problems by just cutting out anything that could be dangerous -- limiting ourselves to further and further subsets of programming until it's impossible to make a mistake?

I don't think I'd like to use the language that resulted from such a process. There's only so far you can tighten the straitjacket before you can't stand up any more.

Inheritance is a tool. It's a hammer. Use a hammer for long enough, and you're going to end up with bruises on your thumb. You could wear really thick gloves, but that's going to make you so clumsy that you'll spend three times as long bashing the nail in, and it'll be crooked when you've finished.

(And now I'm getting too close to the static/dynamic typing debate, so I'd better take a few steps back :) )

As programmers, we should be aware of the dangers implicit in what we do. We should program defensively. But we shouldn't program so defensively that we create paths of hoops we need to jump through just to get something working. Directly overriding a method and then calling super() is a really useful and simple technique if you know what the risks are2. It is certainly not, in and of itself, a code smell.

1 Which is doubly amusing when said programmer is yourself.
2 These risks are significantly higher if you don't control the source of the class you're overriding. Inheritance tends to break encapsulation in a big way.

5 Comments

The point is really, designing for extensibility rather than simply using it leads to more easily maintainable code, and prevents the sorts of problems Cedric found (ie. do it, but do it advisedly). The normal case for method extension is to do some simple decoration of a class (eg. Collection member type checking). Doing it via extension is much simpler than decoration by delegation as the latter can be too long and messy if the interface is large (eg. List* ).

I agree with Cedric in his example though. DBUnit had changed setUp()'s contract from the normal TestCase contract (which is a template), into a critical implementation that is required to be called. It should have been finalised and appropriate new templates created. [However also in Cedric's case, he didn't need to override setUp() in the first place! What he really wanted was a TestSuite decorator (actually a junit.extensions.TestSetup**, as all he was doing was initialising the driver class, and you only need to do that once for the VM before the tests start].

* http://java.sun.com/j2se/1.4.1/docs/api/java/util/List.html
** http://junit.sourceforge.net/javadoc/junit/extensions/TestDecorator.html)

Inheritance doesn't break encapsulation. Programmers break encapsulation. If you understand that your protected interface is just like your public interface but even more 'volatile' and if you clearly javadoc the contract of your protected methods then you don't have much to do. And keep in mind that unless you're a super-paranoid programmer who declares all his public methods final, then all those pretty public methods you expose to the world are overridable by subclasses.

Inheritance breaks encapsulation. Encapsulation is the idea that you can vary the implementation of a class, but so long as its methods fulfil the same contracts, no other classes will be affected by the change.

Once you inherit from a class, you start relying on how that class is implemented internally. For example, there's the old problem having a class where method A calls method B. Overriding B in the subclass will change the behaviour of A, even though you haven't specifically overridden A as well.

I'm with Bo... inheritance doesn't break encapsulation if the contracts are stated and followed.

If method A is depending on the specific implementation of method B, then method A is just plain *wrong* unless B is a private or final method or in a final class. Method A should only be relying upon the stated contractual guarantees of method B. And any override of method B that fails to meet those contractual guarantees is *wrong*.

If method A calls a non-private non-final method B of a non-final class, then it should be *intended* that the behavior of method A will change depending on the implementation of method B. After all, that is the whole point of Template Method: to allow the behavior of a method to vary depending on the overrides of methods that it calls.

To put it more simply, in a non-final class any method that is neither private nor final has an implied clause in its contract that says that its implementation can vary, and any method that relies upon the specific implementation is in violation of the contract.

Alas, nobody really wants to go to the trouble to derive and perfect the contracts for every method and class that they write, and then to ensure that those contracts are the only assumptions made between provider and clients. Nor are they interested in properly coding a private/final method for code where the specific implementation does matter, wrapping it with an overridable public accessor method if needed. So instead we get dicta like "inheritance breaks encapsulation", because they're easier to work with in practice.

I think we're just quibbling over the definition of the term 'encapsulation'.

That it's bad programming is irrelevant to the fact that it breaks encapsulation. The contracts relevant to extension are far broader than those relevant to regular class interactions, and create a significant reliance on the class having good internal behaviour.

The reason I brought it up was that I encountered an example of it today: the classic "Effective Java" example of someone extending the List class naively.

That was the whole point of saying "if you don't control the source". Controlling the source gives you protection against bad programming or badly-documented extension contracts.

Comments are no longer being accepted for this blog entry. If you really want to make your voice heard, you can always email me.

Previously: Snippet

Next: Atom: Can't we wait until it's ready?