Java – Inheritance vs Containment in Extending Large Legacy Projects

designjavalegacyobject-orientedunit testing

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();}
}

Best Answer

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.

Related Topic