C++ - Overuse of virtual methods?

Question

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

Answer 1

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.

Answer 2

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.

Answer 3

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.