Internal exposure of implementation details on inheritance

Question

I'm reading the "Effective Java" book which suggests to favor composition over inheritance. The example it gives shows something like this:

public class InstrumentedHashSet<E> extends HashSet<E> {
     // The number of attempted element insertions 
     private int addCount = 0;

     public InstrumentedHashSet() { }

     public InstrumentedHashSet(int initCap, float loadFactor) {
         super(initCap, loadFactor);
     }

     @Override public boolean add(E e) {
         addCount++;
         return super.add(e);
     }

     @Override public boolean addAll(Collection<? extends E> c) {
         addCount += c.size();
         return super.addAll(c);
     }

     public int getAddCount() {
         return addCount;
    }
}

This presents a problem because the implementation of addAll in HashSet uses the method add. So, when calling addAll each new elements is counted twice - once in addAll and once in add. In the next chapter it is explained that if we choose to allow extending our class we should explicitly explain the inner working of our methods that use overridable methods (meaning - in the addAll documentation we should specify its use of add and commit to this implementation forever).

I think that a better practice would be to decide that each method that is both an overridable, API method and also used by inner implementation should be extracted to an inner, private method. So Our ideal HashSet would have these methods:

public class HashSet<E> implements Set<E> {
     // Ignore irrelevant code


     @Override public boolean add(E e) {
         // The new add implementation only wraps the innerAdd method
         return innerAdd(e);
     }

     @Override public boolean addAll(Collection<? extends E> c) {
        // Do the same logic as before but use innerAdd instead of add
     }

     private boolean innerAdd(E e) {
         // The original add logic will be moved to here
     }
}

This way we encapsulate inner implementation and allow extending the class without fearing to override a method which is used by inner implementation. Of course that we can use the original add as before because by calling super.add(e) we de-facto use the innerAdd so that's not a problem.

I would like to know whether this might be a good practice (not for HashSet<E> which is already committed to the code that use it, but to new classes that are meant to be overriden)?

Answer 1

Yes, they could have done that, but I think you forget the crucial part: The fact that "addAll" does not rely on "add" has to be explicitly documented in the class' public interface . The problem is that doing this wouldn't too elegant. If you add implementation details to your public interface you break the abstraction principle in that class.

Look at this relevant excerpt from that particular chapter:

"This 'self-use' is an implementation detail, not guaranteed to hold in all implementations of the Java platform and subject to change from release to release".

In my opinion the takeaway here is that inheritance can easily break encapsulation and lead to insidious issues like this one, hence his opinion that Java classes should haven been made final by default. That way you would need to make a conscious decision when making a class inheritable.