9
\$\begingroup\$

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();
}
\$\endgroup\$
  • \$\begingroup\$ Disappointed that there's no Kipper the Dog. :) \$\endgroup\$ – Cris Luengo Feb 21 '18 at 3:00
  • \$\begingroup\$ I also miss the implementation of the AddDog and AddCat methods. Could you please post them? \$\endgroup\$ – Cris Luengo Feb 21 '18 at 3:01
0
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Cris: Kind of torn here - lots of good information on the coding side but I have some different opinions on the code intent side. Using inheritance to make your life easier rather than being dogmatic about Object Orientation is something I agree with - to a point. That point is future maintainability. Under the current exercise, each animal only differs by the sound it makes ("Woof", "Meow", "Neigh") - with proper inheritance, creating a new Animal sub-class means just adjusting a single in-sub-class constant for sound against an abstract function that takes a string argument. \$\endgroup\$ – AJD Feb 23 '18 at 5:42
  • \$\begingroup\$ (cont.) However, you totally destroyed the benefits of OOP in your example code (and you have acknowledge that in a sideways way). In My example, adding a Horse merely means creating one new sub-class - and if abstracted properly (std::cout << name << " says " + sound << std::endl;) it means only adding a single constant string. In your example, adding a Horse means important changes to three places in the code. And that doesn't address what happens if we also want to add an Eat or Move function. \$\endgroup\$ – AJD Feb 23 '18 at 5:48
  • \$\begingroup\$ (cont.) Yes, the exercise wanted AddCat and AddDog - which is valid for a question aimed at those learning OOP in stages. But I also read into this a graduated learning exercise in OOP. So a strong foundation (X is a Y, X has a Z) is more important than some short-cuts to achieve what is, in the end, a contrived answer. \$\endgroup\$ – AJD Feb 23 '18 at 5:51
  • \$\begingroup\$ @AJD, you make some good arguments. Maybe I sounded excessively dismissive about inheritance. There are certainly some excellent applications, it's just that this is not one of them. I understand that Kennel is an exercise to lear about OOP, but, IMO, it teaches the wrong use of inheritance. Here inheritance is used for polymorphism, which probably stems from a C++ design issue. Here is a better way to do polymorphism in C++. -- I'm certainly not this negative about inheritance. \$\endgroup\$ – Cris Luengo Feb 23 '18 at 21:37
  • \$\begingroup\$ Your argument that "if abstracted properly, it means only adding a single constant string" means that there should be no inheritance at all: the animal sounds are data, and data should not be encoded in the program structure. The animals should really be a (dynamically updateable) list of properties. On the other hand, if the animal behavior were really specified through code, then inheritance and virtual functions can be quite powerful. It's not that you only need to create a new class, it's that the user of your library can add a new class, and never touch your code. That is fantastic.(cont) \$\endgroup\$ – Cris Luengo Feb 23 '18 at 21:37
10
\$\begingroup\$

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.

\$\endgroup\$
10
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Good suggestion! I'm not sure how comfortable the horse will be, in a pen that was built for cats and dogs, though... ;-) \$\endgroup\$ – Toby Speight Feb 20 '18 at 9:55
  • \$\begingroup\$ @TobySpeight It's probably a miniature horse :) \$\endgroup\$ – Brian J Feb 20 '18 at 14:39
  • 1
    \$\begingroup\$ Use a smart pointer instead of a raw one to avoid memory leaks \$\endgroup\$ – Alessandro Teruzzi Feb 20 '18 at 15:06
  • \$\begingroup\$ … or better yet, don’t use any (unexposed) pointers here. If you need a polymorphic class, make it manage its own memory by design. This isn’t the most trivial thing to implement, nor what text books show but it’s the better solution in real projects most of the time. \$\endgroup\$ – Konrad Rudolph Feb 20 '18 at 15:29
  • 1
    \$\begingroup\$ @CrisLuengo - easily solved: AddCat( string) just calls AddAnimal(new Cat(string))! If the exercise is to demonstrate knowledge or learning about OOP, then understanding that a Cat is a Animal as opposed to some random object that is thrown in a Kennel is a significant part of that learning. Won't be the first time a question has focussed on an incomplete understanding. \$\endgroup\$ – AJD Feb 21 '18 at 4:21
1
\$\begingroup\$

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;
};
\$\endgroup\$
  • \$\begingroup\$ (I hadn't seen this answer previously!) You need to make ~Mammal() virtual to ensure that the destructor of the correct class is called when destroying through a pointer or handle to the base class. You can even make it pure virtual to make sure no object of class Mammal is created: virtual ~Mammal(); (i.e. no function body). \$\endgroup\$ – Cris Luengo Sep 26 at 22:41
0
\$\begingroup\$
 #pragma once
#ifndef KENNEL_H
#define KENNEL_H

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

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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