views:

478

answers:

8

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.

+3  A: 

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.

Stijn Sanders
Think about Foo behaving 90% like Bar. And client code that uses only those 90% of behavior. Like for example code that uses java.util.List but does not modify it, only iterates and get's evements by index. Then you can give it a subclass where set,add,remove and clear methods are not implemented.
Pavel Feldman
Pavel: The problem comes when someone wants to actually use the other 10% and the compiler doesn't stop him because you've subverted the static typechecking safety net.
Stefan Monov
+1  A: 

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.

Graham Lee
+17  A: 

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.
LeopardSkinPillBoxHat
I don't know whether the NotImplementedException is a code smell -- that Exception suggests to me that it's not implemented _yet_. If it was a NotSupportedException, I agree.
Lennaert
Agreed. Consider that laziness on my part - I just copied and pasted it from the original question.
LeopardSkinPillBoxHat
I modified my answer to NotSupportedException.
LeopardSkinPillBoxHat
Upvoted for example. Cheeky and useful
toast
+3  A: 

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!)

Thomas Padron-McCarthy
I don't know who downvoted this, but the explanation you give is pretty clear and useful. Downvoters should take the time to explain why they find something wrong.
Daniel Daranas
Yeah, I got two downvotes on the question, with no comment.
Benjol
+3  A: 

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.

Daniel Daranas
I think deciding a behavior of Rectangle and Square is up to developer. If the only things developer needs from Rectangle is .getHeight() and .getWidth() then Square is a Rectangle behaviorally. If developer needs something like .setSize(windth,height) then Square is probably not a Rectangle
Pavel Feldman
@Pavel: Of course. This decision belongs to the class designer. If you go to the document and read the quote in its context, you will find that shapes are resizeable - which by the way is the only interesting example, when debating inheritance and taxonomy.
Daniel Daranas
+1 Very interesting link.
KaptajnKold
@KaptajnKold Thank you. I'm glad that it's useful. That was a long time ago, last year.
Daniel Daranas
A: 

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.

ck
This is an example of bad design. Another thing is if you had a property like "IsPositionSupported" which by design you must query as a precondition before using "Position"; then at least you would know from the beginning that "Some streams have Position, others don't".
Daniel Daranas
Having a supported flag is definitely a good idea, but I can see the reasoning behind this - roughly 7 of the 8 streams that inherit from stream have the position enabled, but the last, whilst it is essentially a stream, can't return a posiiton or seek.
ck
A: 

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.

drozzy
+1  A: 

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.

Pavel Feldman