“Has a” vs “Is a” - code smells for deciding

Question

I wrote this yesterday, in a class Foo inheriting from Bar:

public override void AddItem(double a, int b)
{
    //Code smell?
    throw new NotImplementedException("This method not usable for Foo items");
}

Wondered subsequently if this is a possible indication that I should be using a Bar, rather than inheriting from it.

What other 'code smells' could help to chose between inheritance and composition?

EDIT I should add that this is a snippet, there are other methods which are in common, I just didn't want to go into too much detail. I have to analyse the implications of switching to composition, and wondered if there might be other 'code smells' which could help to tip the balance.

Answer 1

The example you gave above is clearly a code smell. The AddItem method is a behaviour of the base class Bar. If Foo doesn't support the AddItem behaviour, it shouldn't inherit from Bar.

Let's think of a more realistic (C++) example. Let's say you had the following class:

class Animal
{
    void Breathe() const=0;
}

class Dog : public Animal
{
    // Code smell
    void Breathe() { throw new NotSupportedException(); }
}

The base abstract class Animal provides a pure virtual Breathe() method, because an animal must breathe to survive. If it doesn't breathe, then by definition it is not an animal.

By creating a new class Dog which inherits from Animal but doesn't support the Breathe() behaviour, you are breaking the contract stipulated by the Animal class. The poor dog won't survive!

The simple rule for public inheritance is that you should only do it if the derived class object truly "is a" base class object.

In your particular example:

• Foo doesn't support the AddItem() behaviour stipulated by the Bar contract.
• Therefore, by definition, Foo is "not a" Bar, and shouldn't inherit from it.

Answer 2

So why would you inherit from Bar, if Foo without extensions doesn't behave like Bar? Come to think of it, I wouldn't even declare a method like 'AddItem' as virtual in the base class.

Answer 3

No! A square might be a rectangle, but a Square object is definitely not a Rectangle object. Why? Because the behavior of a Square object is not consistent with the behavior of a Rectangle object. Behaviorally, a Square is not a Rectangle! And it is behavior that software is really all about.

From The Liskov Substitution Principle in Object Mentor.

Answer 4

Yes, that you have to "un-implement" methods is an indication that perhaps you shouldn't use an "is-a" relationship. Your Foos don't seem to really be Bars.

But start by thinking about your Foos and Bars. Are Foos Bars? Can you draw sets and subsets on a paper, and will every Foo (that is, every memeber of the Foo class) also be a Bar (that is, a member of the Bar class)? If not, you probably don't want to use inheritance.

Another code smell that indicates that Foos aren't really Bars, and that Foo shouldn't inherit Bar, is that you can't use polymorphism. Lets say you have a method that takes a Bar as an argument, but it won't be able to handle a Foo. (Perhaps because it calls the AddItem method in its argument!) You'll have to add a check, or handle the NotImplementedException, making the code complicated and hard to understand. (And smelling!)

Answer 5

Of course there's the direct converse, if your Foo implements a lot of interface that just forwards messages to the Bar that it owns, this could indicate that it should be a Bar. Only could, though, smells aren't always correct.

Answer 6

While the example above probably indicates that something goes wrong, NotImplementedException itself is not wrong always. It's all about the contract of superclass and about subclass implementing this contract. If your superclass has such method

// This method is optional and may be not supported
// If not supported it should throw NotImplementedException 
// To find out if it is supported or not, use isAddItemSupported()
public void AddItem(double a, int b){...}

Then, not supporting this method is still ok with the contract. If it is not supported, you probably should disable corresponding action in UI. So if you are ok with such contract, then subclass implements it correctly.

Another option when client explicitly declares that it does not use all class methods and is never going to. Like this

// This method never modifies orders, it just iterates over 
// them and gets elements by index.
// We decided to be not very bureaucratic and not define ReadonlyList interface,
// and this is closed-source 3rd party library so you can not modify it yourself.
// ha-ha-ha
public void saveOrders(List<Order> orders){...}

Then it is ok to pass there implementaion of List that does not support add,remove and other mutators. Just document it.

// Use with care - this implementation does not implement entire List contract
// It does not support methods that modify the content of the list.
public ReadonlyListImpl implements List{...}

While having your code to define all your contract is good, as it let's your compiler to check if you violate the contract, sometimes it is not reasonable and you have to resort to weakly defined contracts, like comments.

In short words it comes to the question, if you really can safely use your subclass as superclass, taking into account that superclass is defined by its contract which is not only code.

Answer 7

The .Net framework has examples like this, particularly in the System.IO namespace - some readers don't implements all of their base class properties / methods and will throw exceptions if you try to use them.

E.g. Stream has a position property, but some streams don't support this.

Answer 8

Composition is preferable to inheritance as it reduces complexity. Better approach would be to use constructor injection and keep a reference to Bar in your Foo class.