Inheritance vs containment while extending a large legacy project

Question

I have got a legacy Java project with a lot of code. The code uses MVC pattern and is well structured and well written. It also has a lot of unit tests and it is still actively maintained (bug fixing, minor features adding). Therefore I want to preserve the original structure and code style as much as possible.

I aim to inject a new behavior into an existing class. Like a Duck that was trained to jump. Then I ask all my Ducks to jump. Those who can - jump. Those who cannot do nothing which is perfectly OK with me.

The new feature I am going to add is a conceptual one, so I have to make my changes all over the code. In order to minimize changes I decided not to extend existing classes but to use containment:

class ExistingClass
{
   // .... existing code


   //  my code adding new functionality
   private ExistingClassExtension extension = new ExistingClassExtension();
   public ExistingClassExtension getExtension() {return extension;}
} 

...
// somewhere in code
ExistingClass instance = new ExistingClass();

...
// when I need a new functionality
instance.getExtension().newMethod1();

All functionality that I am adding is inside a new ExistingClassExtension class. Actually I am adding only these 2 lines to each class that needs to be extended.

By doing so I also do not need to instantiate new, extended classes all over the code and I may use existing tests to make sure there is no regression.

However my colleagues argue that in this situation doing so isn't a proper OOP approach, and I need to inherit from ExistingClass in order to add a new functionality.

What do you think? I am aware of numerous inheritance/containment questions here, but I think my question is different.

EDIT: Most of people replying to my question presume that a containment relationship looks like that:

class NewClass
{
   OldClass instanceOfOldClass;
   void oldFunctionality() { instanceOfOldClass.oldFunctionality();}

   void newFunctionality() { // new code here }
}

But isn't this a kind of a containment relationship too?

class OldClass
{
   void OldFunctionality();
   NewClass instanceOfNewClass;

   void NewFunctionality {instanceOfNewClass.NewFunctionality();}
}

Answer 1

Your approach is what Michael Feather's calls a sprout class, see "Working Effectively with legacy code". It is a well-known technique for avoiding to change too many things in your old class (which makes the risk of accidentally breaking something small). Furthermore, it gives you the ability of testing the extended functionality in isolation (by writing unit test for your ExistingClassExtension), and it keeps the responsibilities separated between the old and the new class.

The (probably worse) alternative to this approach is to modify ExistingClass directly, and implement the new functionality directly in there (and if you do this more than 5 times, you will easily end up with a big ball of mud).

Inheritance is most probably the wrong approach here, at least when your ExistingClassExtension does not need access to the inner workings of ExistingClass, and you would have to modify the code which uses the existing class in many places by replacing the old class name by the inherited class name (probably even if that code does not make use of the extended functionality). Obviously, inheritance prohibits tests in isolation. Furthermore, think of what will happen if you need not just one extension, but five. Are you going to construct an inheritance tree five levels deep? In which order?

So talk to your colleagues, tell them that there are right and wrong uses of inheritance, and this is most probably a very typical case of wrong use.

Answer 2

You are misunderstanding the composition vs inheritance argument here. The idea is that rather than inherit the different behaviors of your class/module/component, you inject it and contain it, like so:

class Duck {
    private ICanFly flyAbility;
    private ICanSpeak soundAbility;
    private ICanSwim swimAbility;

    public Duck(ICanFly wings, ICanSpeak beak, ICanSwim fins) { ... }
}

So then the wings can be replaced with a JetPack, the beak with a VoiceCard and the fins with the UnderwaterPropeller, awesome Duck huh?

The idea is that your different objects can be broken down further by their behaviors, and that you can have Duck which is a bird that swims, Bat which is a mammal that flies, and Platypus which is a mammal that swims and lays eggs.

---

In your case, continuing with the existing structure and extending it (the structure, not necessarily the objects!) would be the best move. The question is how things are built nowadays. Don't break convention.

Answer 3

I think your approach is just equivalent to modifying the ExistingClass. If you inline the ExistingClassExtension inside the ExistingClass, you will see you have declared an inner class, which is accessible through a method of the outer class. If you further inline the inner class' methods and public members (which makes no difference to the callers of this code, except that they save one call to getExtension), you will notice that you have in fact plainly added some new methods underneath the ExistingClass.

Currently you have

class ExistingClass {
    private ExistingClassExtension extension = new ExistingClassExtension();
    public ExistingClassExtension getExtension() {return extension;}
}

class ExistingClassExtension {
    public boolean extendingMethod() { return false; }
}

Inline the class

class ExistingClass {
    class ExistingClassExtension {
        // ... extendingMethod ...
    }
    public ExistingClassExtension getExtension() {return new ExistingClassExtension();}
}

Inline the method

class ExistingClass {
    public boolean extendingMethod() {
        // ... extendingMethod ...
    }
}

I misinterpreted "containment" for "composition" and expected to find a design like this:

class ExistingClass {
    boolean existingMethod() { return true; }
}

class ExtendingClass {
    private ExistingClass instance = new ExistingClass();
    boolean existingMethod() { return instance.existingMethod();}
    boolean extendingMethod() { return false;}
}

while the inheritance option would just declare:

class ExtendingClass extends ExistingClass {
    boolean extendingMethod() { return false; }
}

The criterion to decide whether to compose or inherit was summarized by Scott Myers in "Effective C++" as "Make sure public inheritance models 'is a' relationships".

Without knowing what both classes do or represent, it is impossible to decide whether one 'is a' subtype of the other, but let me remark that in "Effective Java", Joshua Bloch recommends to prefer composition over inheritance in most situations, since inheritance provides your new class with an interface that may be too large, or out of its control. This may lead to subtle bugs, or broken builds in the future, when one of those minor fixes to the legacy code alters one of the private methods your newMethod1 relies upon.

I would suggest that you consider replacing this "containment" strategy with an actual composition of the legacy classes within new classes. You will get all the benefits you list in your question, plus the added benefit that you are not inheriting an interface that may not be what you thought, or may change underneath your feet.