Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

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!

share|improve this question

migrated from stackoverflow.com 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
5  
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
1  
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
show 1 more comment

8 Answers 8

how about RecommendCageType() as oppsed to RequiresConcreteWall()

share|improve this answer
add comment

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.

share|improve this answer
add comment

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
add comment

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
add comment

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
add comment

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
add comment

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
add comment

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
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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