C++ - Overuse of virtual methods?

Recently I was given a task where I had to implement something similar to the following:

There are some animals with certain attributes, such as:

Dog1: name: tery, color:white, fav drink: grape juice

Dog2: name: chiva, color:black, fav drink: lemonade

Bird1: name: tweety, canfly: yes, cansing: no

Bird2: name: parry, canfly: no, cansing: yes

How would you do this in C++ efficiently using OOP prractices?

I did something like this:

class Animal {  
    Animal(...);  
    ...  
    public String getName(开发者_Go百科) const;  
    public void setName(string s);  
    ...  
    private:  
    String name;  
}  

class Bird : public Animal {  
    Bird(...);  

    public bool canFly() const;  
    public void setCanFly(bool b);  
    ...

    private:  
    bool canFly;  
    bool canSing;  
}  

class Dog : public Animal {  
    ...  
}  

The problem with this implementation is that i cannot benefit from polymorhism :

Animal* p = new Anima(...);  
...  
p->canFly();  

and I have to use casting:

((Bird*)p)->canFly();  

At the end I was criticized of not using virtual functions in base class, and using casts instead of OOP.

But in my opinion it doesn't make sense to use virtual functions here because getName() should be in the base class to avoid multiple implementations of same method. And canFly is not a valid property for dogs for example.

Then I would have to define something absurd like this for each other (future) animal that also inherits from the base class, which would create unnecessary overhead:

bool Dog::canFly () const {
return false;
}

Who is right here, did I not get the basic principles of polymorphism?

---

Of course 'canFly' is a valid property for a dog, it's just going to return false.

There's no point in having canFly at all if you only implement it when it needs to be true. In your example, by the time you've done your c-style case to a flying animal, then you've already committed to the type of animal, at which point you don't really need to call canFly, because you already know the answer.

If you really don't want to implement canFly in a large number of non-flying animals, then implement virtual bool canFly() const { return false; } in your base class, and just override it in the flying animals.

I'd assume that this is just a contrived 'homework' question, so the answer is bound to look contrived too, but a style which involves lots of casting object types is really going to be bad news in real work.

---

Well, you don't need a single base class. Consider this:

Animal
  |
  |--Flying Animal
  |        |---Bird
  |
  |--Non Flying Animal
           |---Dog

where:

class Animal
{
public:
  virtual bool CanFly () = 0;
  String Name ();
};

class Flying : public Animal
{
public:
  virtual bool CanFly () { return true; }
};

class NonFlying : public Animal
{
public:
  virtual bool CanFly () { return false; }
};

class Bird : public Flying
{
};

class Dog : public NonFlying
{
};

There are many other ways to do this as well, even using composition rather than inheritance.

EDIT: Composition. Having a hierarchy where each level in the hierarchy represents a smaller group of members (there are fewer dogs than animals) presents the problem of how to divide the set of all possible types into a hierarchy. As Lagerbaer pointed out in the comments, some birds can't fly.

So instead of creating a complex tree, have a simple tree (or no tree) and have each animal contain a list of characteristics of that animal:

class Animal
{
public:
   String Name ();
   List <Characteristic> Characteristics ();
};

class Characteristic
{
public:
   String Name ();
};

class CanFly : public Characteristic
{
public:
  bool CanFly ();
};

class Legs : public Characteristic
{
public:
  int NumberOfLegs ();
};

And then, to create a dog:

Animal *CreateDog ()
{
  Animal *dog = new Animal;
  dog->Characteristics ()->Add (new CanFly (false));
  dog->Characteristics ()->Add (new NumberOfLegs (4));
  return dog;
}

and to create a bird:

Animal *CreateBird ()
{
  Animal *bird = new Animal;
  bird->Characteristics ()->Add (new CanFly (true));
  bird->Characteristics ()->Add (new NumberOfLegs (2));
  return bird;
}

There are two advantages to this:

1. You can extend it to add whatever characteristics you want.
2. You can drive the creation of animals from data rather than hard coding it all.

If your language of choice supports reflection, then searching the characteristics list is very straightforward. In non-reflection languages, you'll need to implement some way to identify what each characteristic is.

---

To address the technical issue, this is wrong:

((Bird*)p)->canFly(); 

This C-style cast performs a static_cast; if p points to a Dog, the cast will succeed but the result will be incorrect. Bad Things Happen.

When you don't know the most derived type of the pointed-to object and you don't have some way of determining its type via the base class pointer, you need to use dynamic_cast:

if (Bird* bp = dynamic_cast<Bird*>(p)) {
    // p points to a Bird
}
else {
    // p points to something else
}

dynamic_cast returns a null pointer if the cast fails, allowing you to check the type of the object.

---

To address the design issue, it depends. In real-world software you can't always have virtual functions in the base class that define the behavior of every possible derived class. It's just not possible. Sometimes it is necessary to dynamic_cast to a derived class to be able to call functions not declared in the base class.

Casts probably were not necessary in this very simple case, but if you were to consider a more complete class hierarchy defining the animal kingdom, you'd find that you would either need an enormous number of functions in the Animal base class or you would have to use casts at some point.

---

Virtual methods only make sense where there is a need for subclasses to provide their own implementation, or to force them to (pure virtual).

In the case of your canFly and canSing usage, where data members in the base class support invariant implementation in all subclasses, making those get/set methods virtual makes no sense at all to me.

A better candidate for virtual would be the corresponding fly and sing methods, where base class implementation might throw and only when the properties are set true would an animal-specific implementation be provided in a subclass.

---

struct Animal {
  std::string name;
  std::string favoriteDrink;
  bool canFly;
  bool canSing;
};

Feel free to wrap get/setters around the members if it makes you happy.

But one thing people tend to forget is that polymorphism is about behavior. It is about making different classes that look the same, but behave differently.

In this example, there is no different behavior between any of the animals, and so making more than one class is overkill.

There is no actual behavior required for any of the animals. The only operations we need are the ability to ask "what is its name", "can it fly", "can it sing" (and of course, "will it blend?")

All of these operations make as much sense for a penguin as they do on a terrier, a blue whale or a shrew. The behavior is the same, only the data changes. And so it should be one class, with different instances for different animals.

And so trying to split them into separate classes goes against all the goals of OOP: you end up intentionally duplicating code, doing less code reuse, and you're making your code less polymorphic, not more. In my solution, any animal is a drop-in replacement for any other animal. Once you start messing about with different classes and virtual methods, you have to actually write new code for each new animal in order for it to be a suitable implementation of the Animal base class.

If you ever need to add the actual Fly() method, you might need different classes. The mechanics of flying are different for a sparrow, an eagle and a bat (although even this depends on the objective. Depending on what abstraction level the application is working on, the "fly" routine might consist of nothing more than setting another bool flag somewhere, or perhaps giving the animal a positive non-zero altitude, in which case the same implementation is reusable for any flying animal).

But at the moment, all we need is the ability to ask whether or not an animal can fly. And the implementation of that is trivially reusable.

But of course, it's clear from the task you were given that the correct answer (where "correct" is defined as "the I expected when I asked the question" is "use lots of virtual methods for everything, and give everything its own class".

Which just goes to show that the more OOP zealotry you get from someone, the lower the odds that they actually understand OOP.

See also my blog post on the topic

---

It might be too much in that simple case, but later on you could keep all your animals in a linked list (or standard list or array or whatever) and then iterate over all entries and just call the base methods to do all kinds of stuff without having to worry about casts.

Just think of a simple game with GameObject being the base class and the Methods update() and draw() being virtual. You then inherit other classes, e.g. PlayerObject, EnemyObject, PowerUpObject, etc.

In your main loop you could then do something like this:

GameObject *p = firstObject;
while(p)
{
    p->update();
    p = p->nextObject;
}

This will iterate over all game objects and call the proper update() methods (e.g. moving the player, letting a power up spin or whatever) and you don't have to do some special casting, e.g. checking to see if it's a player or whatever.

---

I think you are right. Adding every conceivable property that some family of animals can have to a base class Animal is plain silly and produces too much overhead.

Although it is clear what was intended in the task, i.e., that you really have a virtual function canFly in the base class, I think this is poor design.

---

Declaring something virtual doesn't stop you implementing it in the base class.

It's a mechanism for saying that you should use the most specific implementation available. It is distinct from over-riding the implementation in the derived class.

Why should returning false from canFly() for a dog be a problem? Some birds can't fly and there are non-birds that can fly.

---

In my humble opinion, having getter and setter methods is indicative of poor object-oriented design. And this problem space is not particularly conducive to showing off what good object-oriented design is either.