Implementation of a Kennel

Question

I need a code review for the following question:

Using C++ object oriented design, provide the implementation of Kennel so that:

• The method AddCat() adds a Cat to the Kennel, providing its name.
• The method AddDog() adds a Dog to the Kennel, providing its name.
• The method RollCall() prints the Animal's name and sound to stdout:
  • Cats identify themselves by printing "Meow" to stdout.
  • Dogs identify themselves by printing "Woof" to stdout.

Kennel .h

    #pragma once
    #ifndef KENNEL_H
    #define KENNEL_H

    #include <string>
    #include <iostream>
    #include <vector>

    class Kennel
    {

    public:
        Kennel() { };
        virtual ~Kennel();
        void AddCat(const std::string & name);
        void AddDog(const std::string & name);
        void RollCall();
        virtual void makeSound(std::string name) { }

    private:
        std::vector <Kennel*> KennelList;
    protected:
        std::string name;

    };

    //Dog inherits Kennel
    class Dog :public Kennel
    {
    public:
        Dog(std::string dogName)
        {
            name = dogName;
        }
        ~Dog() { };
        void makeSound(std::string name)
        {
            std::cout << name << " says Woof" << std::endl;
        };
    };

    //Cat inherits Kennel
    class Cat :public Kennel
    {
    public:
        Cat(std::string catName)
        {
            name = catName;
        }
        ~Cat() { };
        void makeSound(std::string name)
        {
            std::cout << name << " says Meow" << std::endl;
        };
    };


    #endif

Kennel.cpp

    #include "Kennel.h"


    Kennel::~Kennel()
    {
        for (auto i : KennelList)
        {
            delete i;
        }
    }

    void Kennel::AddCat(const std::string & name)
    {
        KennelList.push_back(new Cat(name));
    }

    void Kennel::AddDog(const std::string & name)
    {
        KennelList.push_back(new Dog(name));
    }

    void Kennel::RollCall()
    {
        for (unsigned int i = 0; i < KennelList.size(); ++i)
        {
            KennelList[i]->makeSound(KennelList[i]->name);
        }
    }

main

#include "Kennel.h"
    int main()
    {
    Kennel kennel;

    kennel.AddCat("Garfield");
    kennel.AddDog("Odie");
    kennel.AddDog("Pluto");
    kennel.AddCat("Felix");
    kennel.AddCat("Sylvester");
    kennel.AddCat("Scratchy");
    kennel.AddDog("Scooby Doo");
    kennel.AddCat("Puss in Boots");
    kennel.AddDog("Goofy");
    kennel.AddDog("Old Yeller");

    kennel.RollCall();
}

Answer 1

I like how you made the Kennel class the virtual base for your animals. The other reviewers might have a point in that this could become confusing, but for such a simple project I think this is quite effective. It avoids the need for a pure virtual Animal class, which is kind of neat. Mind you, I'm highly skeptical of the "object oriented" approach as it is typically practiced. I can count on one hand the number of times I've been able to use inheritance to actually simplify things.

You also do not use using namespace std. Fantastic!

Naming

Your variable names are OK for the most part. But do note that AddCat and AddDog use name as input argument, and the object has a name member variable also. Thus within these functions, the member variable is not visible. You should try to avoid duplicating names like this. One common solution is to have member variables consistently named as m_name or name_. This makes it obvious what they are, and it also prevents name clashes.

The methods you implemented based on the exercise's requirements have a different naming style than the makeSound method you created. You also have two member variables, one starts with an upper case letter and the other with a lower case letter. Try to be consistent in naming!

Memory management

The destructor for Kennel iterates over KennelList and destroys all allocated memory. From the looks of it you're not leaking memory. But ideally you don't want to have to deal with deallocation. Why not let the compiler figure out what to deallocate?

std::vector<std::unique_ptr<Kennel>> KennelList;

If you use a vector of std::unique_ptr elements, you don't need to worry about delete. When you stick a pointer into the KennelList, it will from that moment on take care of not only the pointer, but also the pointed-to data. You would do something like this to put your pointers into the vector:

KennelList.emplace_back(new Cat(name));

Input arguments

For the AddCat method you take a const reference to a string as input argument. This is excellent (though I prefer to write std::string const& name rather than const std::string & name, it prevents some misunderstandings between me and the compiler...). However, for all the other methods you take a std::string by value (Dog and Cat constructors, makeSound). There is no need to make these copies.

Though one alternative for the constructor could be

Dog(std::string dogName)
{
  name = std::move(dogName);
}

Here, instead of taking dogName by reference, you take it by value, making a copy. This copy you then move into your member variable. The advantage of making the copy at the input of the function is that when you call the function,

Dog("Kipper");

a temporary std::string is made. The compiler will not create a copy of this temporary to pass to the Dog constructor, but it will directly use that temporary. Thus, the std::string is constructed only once, and directly stored in your new object.

Note also that makeSound doesn't need an input argument at all. The object should know its own name!

Loops

In your destructor you use the modern-style loops:

for (auto i : KennelList)

But in RollCall you write

for (unsigned int i = 0; i < KennelList.size(); ++i)

Again, consistency is good. Also, the first loop form is better because it's less verbose, and thus much easier to read.

std::endl

std::endl not only prints a newline, it also flushes the stream. It's much more efficient to let the system decide when to flush.

std::cout << name << " says Woof" << std::endl;

produces the same output as

std::cout << name << " says Woof\n";

Inheritance

I know you're supposed to use "object oriented" design in this exercise, but I think if you have one object it's "object oriented", no? I like to avoid inheritance where I can, because I usually find the resulting code simpler to read and to maintain, and it tends to be much more efficient. For example, note how this code is much shorter:

class Kennel {
   public:
      void AddCat(std::string name) {
         kennelList_.emplace_back(Species::Cat, std::move(name));
      }
      void AddDog(std::string const& name) {
         kennelList_.emplace_back(Species::Dog, std::move(name));
      }
      void RollCall() {
         for (auto animal : kennelList_) {
            std::cout << animal.name;
            switch (animal.species) {
               case Species::Dog:
                  std::cout <<  " says Woof\n";
                  break;
               case Species::Cat:
                  std::cout <<  " says Meow\n";
                  break;
            }
         }
      }
   private:
      enum class Species { Dog, Cat };
      struct Animal {
         Species species;
         std::string name;
         Animal(Species s, std::string n) 
            : species(s), name(std::move(n)) {}
      };
      std::vector <Animal> kennelList_;
};

Besides simpler code, it is also more efficient. Not that it matters at all in this application, but I like to think about efficiency. Feel free to skip the rest of this section, you're just starting out, but maybe you find this interesting.

One of the most important concepts in modern hardware is data locality. Data that is close together is faster to process because it plays better with the cache. The other important concept is branch prediction. Modern hardware tries to predict what the outcome of a conditional statement is, and will start executing that code even if it hasn't done the computations yet that the condition depends on. If it fails to predict correctly, stuff gets thrown out and it starts to process the correct branch. If it can predict well, if statements don't delay things all that much. Otherwise, it's good to prevent ifs.

Note what happens when processing the elements of a std::vector<Kennel*>: the pointers themselves are contiguous in memory, it's easy to get them. But they point at things allocated independently, and therefore possibly not contiguous. The RollCall function must fetch these objects (cache is ineffective here), get their virtual function table pointer, look up the address for the makeSound method, then call the method (branch prediction is not applicable, because the CPU is literally waiting for a pointer that points at the code to be executed).

Compare this to what happens in the simpler code above: the Animal objects are all contiguous in memory (because we hold the items directly). The RollCall method loops over these, and one of two code branches is executed depending on a value in it. The cache and branch prediction can do their thing here.

Though, since what is being done inside the loop is writing to stdout, all of the above is way fast in comparison, and the efficiency matters little here.

Answer 2

Think of the phrase : public to mean "is a".

A dog is not a kennel, nor is a cat, so Dog and Cat should not inherit from Kennel. You need a class called Animal. Cats and dogs are animals, so Cat and Dog should derive from Animal. Animals are contained in a kennel.

Answer 3

Further to what Jive Dadson mentioned.

You can reduce your methods in Kennel to

void AddAnimal(Animal *animal);

This could be called by

kennel.AddAnimal(new Cat("Garfield")); // or new Dog(), as appropriate

Your roll call remains the same because your Animal class has the abstract method virtual void makeSound(std::string name) { }. And this Animal class, not the Kennel class contains the name (Animal has a name)

protected:
    std::string name;

Using the correct abstraction makes adding other animals (e.g. Horse, Snake) easy and logical to do. Using your current code, adding a Horse means writing the Horse class and then an AddHorse method. With the abstraction I have suggested, all you do is write the Horse class (class Horse :public Animal) and your Kennel class just works as before.

Answer 4

Supplementing my earlier answer (rather than editing it and negating the comments so far). After the comments exchange between myself and Cris Luengo, I thought there was some additional information that would be useful in this Code Review.

A key point made by Cris is that a pure approach to OOP is not appropriate for this simple example. In the real world I would largely agree, although saying a Dog can be inherited from Kennel is stretching the maintainability a tad. I will offer some revised code below on the basis that:

• This is part of a graduated learning exercise
• The code base will be used later to expand upon OOP fundamentals
• A pure OOP approach is warranted.

Cris's excellent points about code efficiency must be considered if trying to do this in the real world.

class Kennel
{
public:
    Kennel() { };
    ~Kennel(){
        for (auto i : KennelList) {
            delete i;
        }
    };
    void ReceiveAnimal(Mammal * newAnimal){
                KennelList.push_back(newAnimal);
    };
    void RollCall(){
        for (unsigned int i = 0; i < KennelList.size(); ++i){
            KennelList[i]->makeSound();
        }
    };
private:
    std::vector <Mammal*> KennelList;
protected:
};

class Mammal
{
public:
    Mammal(std::string newName){
        name = newName;
    }
    ~Mammal() { };
    void makeSound()
    {
        std::cout << name << " says " + noise << std::endl;
    };
private:
protected:
    std::string name;
    std::string noise;
};

//Dog inherits Mammal
class Dog :public Mammal
{
public:
    Dog(std::string dogName): Mammal(dogName)
    {
        noise = "Woof";
        name = dogName;  // I don't know the language well enough, I suspect this line is not required.
    }
    //~Dog() { };
   };
 //Cat inherits Mammal
class Cat :public Mammal
{
public:
    Cat(std::string catName): Mammal(catName)
    {
        noise = "Meow";
        name = catName;
    }
    //~Cat() { };
   };

I may have been slightly inefficient in my coding above, I don't know the language well enough to deal with the abstracted destructor and the abstracted constructor. But Visual Studio did not complain (but I have not run it).

The main would now look like:

int main()
{
    Kennel kennel;

    kennel.ReceiveAnimal(new Cat("Garfield"));
    kennel.ReceiveAnimal(new Dog("Odie"));
    kennel.ReceiveAnimal(new Dog("Pluto"));
    kennel.ReceiveAnimal(new Cat("Felix"));
    kennel.ReceiveAnimal(new Cat("Sylvester"));
    kennel.ReceiveAnimal(new Cat("Scratchy"));
    kennel.ReceiveAnimal(new Dog("Scooby Doo"));
    kennel.ReceiveAnimal(new Cat("Puss in Boots"));
    kennel.ReceiveAnimal(new Dog("Goofy"));
    kennel.ReceiveAnimal(new Dog("Old Yeller"));

    kennel.RollCall();

}

AN advantage of this approach is that if you already have an animal, you can now just pass it through kennel.ReceiveAnimal(myExistingAnimal);

I have done this extended answer to look at pure OOP and code maintainability as if this was a large endeavour (again, Cris's points about the level of effort for this simple example should be considered).

If the Kennel decided to take new animals (e.g. a Fox), then simply add a new class (which can be as simple as the following code):

class Fox :public Mammal
{
public:
    Fox(std::string foxName): Mammal(foxName)
    {
    noise = "Ha Ha Ha! Boom! Boom!";
    } // see my previous notes about inexperience with this language and assuming name will be handled by superclass.
   };

Mammals give birth. Using the full OOP fundamentals, you can modify the superclass with a new method, which means the subclasses will have this new functionality. The bit I don't know because of my inexperience is how to constrain the new method to ensure it returns a new instance of the subclass rather than the superclass.

class Mammal
{
public:
    Mammal(std::string newName){
        name = newName;
    }
    ~Mammal() { };
    void makeSound()
    {
        std::cout << name << " says " + noise << std::endl;
    };
    Mammal giveBirth(std::string newName) {
        return new Mammal(newName);  // pardon my ignorance here but you get the gist
   };

private:
protected:
    std::string name;
    std::string noise;
};

Answer 5

 #pragma once
#ifndef KENNEL_H
#define KENNEL_H

Only use one of these, preferably #pragma once. No need for #ifndef ... #endif.