Inheritance gone wrong

Question

I have some code where a good inheritance model has gone downhill and I am trying to understand why and how to fix it. Basically, imagine you have a Zoo hierarchy with:

class Animal  
class Parrot : Animal 
class Elephant : Animal 
class Cow : Animal

etc.

You have your eat(), run(), etc methods and all is good. Then one day someone comes along and says - our CageBuilder class works great and uses the animal.weight() and animal.height(), except for the new African Bison which is too strong and can shatter the wall, so I am gonna add one more property to the Animal class -- isAfricanBizon() and use that when choosing the material and only override it for the AfricanBizon class. Next person comes and does something similar and next thing you know you have all these properties specific for some subset of the hierarchy into the base class.

What's a good way to improve/refactor such code? One alternative here would be to just use dynamic_casts to check for the types but that clutters the callers and adds a bunch of if-then-else all over the place. You can have more specific interfaces here but if all you have is the base class reference that doesnt help much either. Any other suggestions? Examples?

Thanks!

Answer 1

It seems like the problem is instead of implementing RequiresConcreteWall(), they implemented a flag call IsAfricanBison(), and then moved the logic on whether or not the wall should change outside the scope of the class. Your classes should expose behavior and requirements, not identity; your consumers of these classes should work from what they are told, not based of what they are.

Answer 2

isAfricanBizon() isn't generic. Suppose you extend your animal farm with a hyppopotamus which is also too strong but returning true from isAfricanBizon() to have the proper effect would be just silly.

you always want to add methods to the interface which answer the specific question, in this case it would be something like strength()

Answer 3

I think your problem is this: you have various clients of the library that are interested in only a subset of the hierarchy but that are passed a pointer/reference to the base class. That is in fact the issue that dynamic_cast<> is there to solve.

It is a matter of design of the clients to minimize use of dynamic_cast<>; they should use it to determine if the object requires special treatment and if so do all the operations on the down-casted reference.

If you have "mix-in" type collections of functionality that apply to several separate sub-hierarchies, you might want to use the interface pattern that Java and C# use; have a virtual base class that is a pure-virtual class, and use dynamic_cast<> to determine if an instance provides an implementation for it.

Answer 4

One thing you can do is replace the explicit checking of type like isAfricanBison() with checking of the properties you're actually interested in, i.e. isTooStrong().

Answer 5

Animals shouldn't care about concrete walls. Maybe you can express it with simple values.

class Animal {
public:
  virtual ~Animal() {}
  virtual size_t height() const = 0;
  virtual size_t weight() const = 0;
  virtual bool isStrong() const = 0;
};

Cage *CreateCageFromSQL(Animal &a);
Cage *CreateCageFromOrangePeelsAndSticks(Animal &a);

I suspect that isn't viable though. That is the problem with toy examples, though.

I would never want to see RequiresConcreteWalls() or lines and lines of dynamic pointer casts at any rate.

This is usually a cheap solution. It's easy to maintain and conceptualize. And really, the problem states that its tied to the animal type anyway.

class Animal {
public:
  virtual ~Animal() {}
  virtual CageBuilder *getCageBuilder() = 0;
};

This doesn't preclude you from using shared code either, just pollutes Animal a bit.

But how the cage is built may be a policy of some other system, and maybe you have more than one type of cage builder per animal. There many weird and convoluted combinations you can come up with.

I've used Component Based Design to good ends, the main problem with it is that it can be troublesome when Animal's ownership is shared. How to avoid throwing in destructors being the pain point.

Double Dispatch is another option, though I've always been reticent to jump into it.

Beyond that it's hard to guess at the problem.

Answer 6

Well surely all Animals have the inherent property of attemptEscape(). While some the method may pose a false result in all scenarios while others may have a chance based off of heuristics of their other intrinsic characteristics such as size and weight. Then certainly at some point attemptEscape() becomes trivial as it will most certainly return true.

I am afraid I don't completely understand your question though... all animals have related actions and characteristics. Ones specific to the animal should be introduced where it is fitting. Trying to directly relate Bison to Parrots is not a good inheretence setup and should really not be an issue in a proper design.

Answer 7

Another option would be to use a factory which creates cages appropriate for each animal. I think this can be better in case conditions are very different for each of them. But if it's just this one condition the above mentioned RequiresConcreteWall() method will do it.

Answer 8

how about RecommendCageType() as oppsed to RequiresConcreteWall()

Answer 9

Why not do something like this

class Animals { /***/ } class HeavyAnimals{} : Animals //The basic class for animals like the African Bison

With the class HeavyAnimals you can create the African Bison class by extending the HeavyAnimals class.

So now you the parent class (the Animals) which can be use to create other base class like the HeavyAnimal class with can be use to create the African Bison class and other Heavy Animals. So with the African Bison you now have access to the Animal class methods and property (this is the base for all animals) and access to the HeavyAnimals class (this is the base for Heavy Animals)