Sign up ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free.

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


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?


share|improve this question

migrated from Jul 17 '11 at 11:41

This question came from our site for professional and enthusiast programmers.

@James: then you'll have to write parsers by hand. :S – Matteo Italia Jul 14 '11 at 22:14
Clearly this is a case of ridiculous-customer-requirement. There are no bison in Africa. You cannot design object models that have no connection to reality. Unless that reality is created by hands full of dollars. Which solves the problem. – Hans Passant Jul 14 '11 at 23:26
Eat all the bison? [I posted this previously but it was for some reason deleted, presumably by humorless jackasses.] – James McNellis Jul 16 '11 at 7:18
Does CageBuilder need its own class? What if there is a default MakeCage method, which can be overriden by each individual class. – Job Jul 17 '11 at 15:05
Is it only me or is it just to add a strength property for the cage building code to check against? – user1041 Jul 17 '11 at 16:59

9 Answers 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)

share|improve this answer
That may work as a mix-in or trait, but certainly not as a subclass. This is just begging for multiple inheritance next time another property is needed. – Ordous Jul 17 at 17:58

how about RecommendCageType() as oppsed to RequiresConcreteWall()

share|improve this answer

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

class Animal {
  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 {
  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.

share|improve this answer

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.

share|improve this answer

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.

share|improve this answer

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.

share|improve this answer

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().

share|improve this answer
isTooStrong() for what? You're adding cage specific code to the animal class. – Steve Evers Jul 17 '11 at 15:10

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.

share|improve this answer
-1: Only says what not to do. The OP already knows that this was a bad idea, hence the question. – Steve Evers Jul 17 '11 at 15:11

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

share|improve this answer
+1: Everyone else seems to be breaking the conceptual model of the class (which just encapsulates properties of different kinds of animals), to accomodate this specific use-case. A strength method could be queried by material.canHold(animal), allowing a clean way of supporting different kinds of material than ConcreteWall. – Aidan Cully Jul 17 '11 at 12:09
I like the strength() property approach better than others' suggestion of RequiresConcreteWall() because it is more flexible for enabling future requirements. For starters, make the CageBuilder class decide which materials are strong enough, and then you can easily extend the class with new materials. – jhocking Jul 17 '11 at 14:25

protected by gnat Jul 17 at 21:45

Thank you for your interest in this question. Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site.

Would you like to answer one of these unanswered questions instead?

Not the answer you're looking for? Browse other questions tagged or ask your own question.