8
\$\begingroup\$

I am learning about polymorphism in C++ and decided to implement an example. Is there anything I need to fix in my code? This compiles and runs.

#include <iostream>


using std::cout;
using std::endl;
using std::cin;


class Employee
{
public:
    Employee():salary(1000){}
    virtual ~Employee(){}
    virtual void Talk() const {cout << "Employee says: ";}

protected:
    int salary;
};

class Programmer : public Employee
{
public:
    void Talk() const {cout << "I am a Programmer.\n";}
    void Compiling() const {cout << "My code is compiling...\n";}
};

class SalesPerson : public Employee
{
public:
    void Talk() const {cout << "I am a SalesRep.\n";}
    void Selling() const {};
};


int main()
{
    const int numEmployees = 3;
    Employee* Company[numEmployees];
    Employee* pEmployee;

    int choice,i;

    for(i=0; i<numEmployees; i++)
    {
        std::cout << "(1) Programmer (2) SalesRep: ";
        cin >> choice;
        if(choice == 1)
            pEmployee = new Programmer;
        else
            pEmployee = new SalesPerson;

        Company[i] = pEmployee;
    }

    cout << endl;

    for(i = 0; i<numEmployees;i++)
    {
        Company[i]->Talk();

        Programmer *pCoder = dynamic_cast<Programmer *> (Company[i]);

        if(pCoder)
            pCoder->Compiling();


        cout << endl;
    }

    return 0;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Is Selling() supposed to be empty? What is it doing? \$\endgroup\$ – Quaxton Hale Oct 6 '14 at 6:24
3
\$\begingroup\$

Some things I noticed at once:

There is something wrong in this for-loop:

for(i = 0; i<numEmployees;i++)
{
    Company[i]->Talk();

    Programmer *pCoder = dynamic_cast<Programmer *> (Company[i]);

    if(pCoder)
        pCoder->Compiling();


    cout << endl;
}
  1. There is a memory leak. You need to add delete Company[i]

  2. You aren't using the Employee object polymorphically.

Why are you doing this?

Programmer *pCoder = dynamic_cast<Programmer *> (Company[i]);

You are trying to get at a method that doesn't seem appropriate for the base class. Compiling() is a method specific to the derived class Programmer What you are doing is a bit "hacky". It works because the base pointer is checked at runtime. You added an if-statement, so I assume you are at least aware.

In the future, do not access methods that are specific to derived classes through the base pointer.

You are using std::cout. I would keep it consistent and remove std:: from std::cout << "(1) Programmer (2) SalesRep: ";.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Adding delete Company[i] is a bad advice. std::unique_ptr would be much better. \$\endgroup\$ – nwp Oct 6 '14 at 14:42
6
\$\begingroup\$

Problems with your interface design:

The Employee interface you've designed is clearly lacking abstraction, since you have a dynamic_cast in the program to test if an employee is of a specific type.

Employee should instead have a virtual method such as DoWork() which in turn will call Compiling() for Programmer, Selling for a SalesPerson and so forth:

class Employee 
{
public:
    ....

    // Virtual function with no default implementation (= 0)
    virtual void DoWork() const = 0;
};

class Programmer 
{
public:
    void DoWork() const { Compiling(); }
};

class SalesPerson
{
public:
    void DoWork() const { Selling(); }
};

The constructor of Employee sets salary to 1000 unconditionally. Perhaps it would be better to take salary as a constructor parameter so you can easily specify different salaries:

Employee(double salary) : salary(salary) { }

BTW, double seems more appropriate for a monetary value, as it is likely to include decimal places. int is for whole numbers only.

The destructor of Employee is empty, so it should be made a default:

virtual ~Employee() = default;

Memory management tidbits:

It is generally better to avoid manual memory management, as it is error prone, not exception safe, and slipping a memory leak is very easy. In fact, in your current example, the objects you've allocated with new are not being destroyed. Remember that C++ is not garbage collected. You would have to delete them somewhere.

You should ideally be using an automatic pointer to manage your dynamic instances. main() could be reworked in the following way:

#include <array>
#include <memory>

int main()
{
    std::array<std::unique_ptr<Employee>, 3> company;

    int choice = 0;
    for (int i = 0; i < company.size(); i++)
    {
        std::cout << "(1) Programmer (2) SalesRep: ";
        cin >> choice;

        if (choice == 1)
        {
            company[i].reset( new Programmer );
        }
        else
        {
            company[i].reset( new SalesPerson );
        }
    }

    ....
}

You should note in the example above the use of std::unique_ptr and std::array. The unique_ptr is a managed pointer that will delete the object automatically when you leave the function scope. This way you don't have to worry about memory leaks. std::array is a fixed size array which behaves just like a plain C array, but with some extra member methods and debug bounds checking. It also knows its length, so we no longer need to keep that numEmployees constant around.

| improve this answer | |
\$\endgroup\$
2
\$\begingroup\$

Adding what EngieOP suggested Programmer *pCoder = dynamic_cast<Programmer *> (Company[i]);
is not proper, instead use a common method to identify the class, e.g. have a method named employee_identification which just returns what type of employee he/she is.

For example:

class Employee
{
public:
    Employee():salary(1000){}
    virtual ~Employee(){}
    virtual void Talk() const {cout << "Employee says: ";}
    virtual int employee_identification(){
            return 0; //ENUM can be used for better description.
        };

protected:
    int salary;
};

class Programmer : public Employee
{
public:
    void Talk() const {cout << "I am a Programmer.\n";}
    void Compiling() const {cout << "My code is compiling...\n";}
    int employee_identification(){
            return 1; // (1) Programmer.
        };
};

class SalesPerson : public Employee
{
public:
    void Talk() const {cout << "I am a SalesRep.\n";}
    void Selling() const {};
    int employee_identification(){
            return 2; // (2) SalesPerson.
        };
};

Now after you can change main as follows,

switch (pCoder->employee_identification())
{
     case 1:
        pCode->Compiling();
        break;
     case 2:
        pCode->Selling();
        break;
     default:
         cout<<"Wrong employee id\n"
}
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Even better than the switch, the top level interface Employee should have a method DoWork() or similar that calls either Compiling(), Selling(), etc. \$\endgroup\$ – glampert Oct 6 '14 at 14:01
  • \$\begingroup\$ Yes that's also better idea. \$\endgroup\$ – ART Oct 6 '14 at 14:16
  • \$\begingroup\$ The dynamic_cast is still a bit better than the employee_identification() "trick" (especially with an undocumented enum). At least it pollutes less classes, and can be searched for. But a real abstract method DoWork() like suggested is much better. \$\endgroup\$ – Gnurfos Oct 6 '14 at 14:42
  • \$\begingroup\$ DoWork() better for sure, but if we use dynamic cast how do we know what type of object is present in company[i] in second for loop? \$\endgroup\$ – ART Oct 6 '14 at 14:48
  • \$\begingroup\$ Downcasting with dynamic_cast (on pointers) returns 0 on failure. The following "if" is how we know the type (or whether it is a Programmer at least). \$\endgroup\$ – Gnurfos Oct 6 '14 at 14:51

Your Answer

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