CODEWITHSUNDEEP
c++design-patternsc++11segmentation-fault
tshah06
tshah06

Reputation: 2755

Not able to understand why segmentation fault?

I have implemented below program to understand composite design pattern. I have used several concepts from C++11 as well. But to my bad, this program is giving segmentation fault while running it. I tried to debug with GDB and came to know that there is some problem with getID() function. 

#0  0x08049a12 in Employee::getID (this=0x0) at Composite.cpp:27
27      int getID(){return ID;}

But still i'm not able to understand what is wrong with that function? Appreciate if someone can help.

#include <iostream>
#include <memory>
#include <string>
#include <vector>
#include <algorithm>

using namespace std;

class Employee
{
    protected:
    int ID;
    string Name;
    string Role;

    public:
    Employee(int empID, string empName, string empRole)
    {
        ID=empID;
        Name=empName;
        Role=empRole;
    }
    virtual void showDetails()=0;
    virtual void addWorker(shared_ptr<Employee> newWorker)=0;
    virtual void deleteWorker(shared_ptr<Employee> employee)=0;
    virtual ~Employee(){}
    int getID(){return ID;}
};

class Worker : public Employee
{
    public:
    Worker(int empID, string empName, string empRole)
        : Employee(empID, empName, empRole) {}

    void showDetails()
    {
        cout<<Name<<" ("<<ID<<") "<<Role<<endl;
    }

    void addWorker(shared_ptr<Employee> newWorker){};
    void deleteWorker(shared_ptr<Employee> employee){};
};

class Supervisor : public Employee
{
    private:
    vector<shared_ptr<Employee>> myTeam;

    public:
    Supervisor(int empID, string empName, string empRole)
        : Employee(empID, empName, empRole) {}

    void addWorker(shared_ptr<Employee> newWorker)
    {
        myTeam.push_back(newWorker);
    }

    void deleteWorker(shared_ptr<Employee> employee)
    {
        int pos=0;
        for (auto temp : myTeam)
        {
        if (temp->getID()!=employee->getID())
            ++pos;
        else
            myTeam.erase(myTeam.begin()+pos);
        }
    }

    void showDetails()
    {
        cout<<Name<<" ("<<ID<<") "<<Role<<" ---->"<<endl;
        for (auto worker : myTeam)
        {
        worker->showDetails();
        }
        cout<<endl;
    }
};

int main()
{
    shared_ptr<Employee> Tushar(new Worker(376653,"Tushar Shah","Team mate"));
    shared_ptr<Employee> Ranjeet(new Worker(469725,"Ranjeet Aglawe","Team mate"));
    shared_ptr<Employee> Kiran(new Supervisor(137581,"Kiran Asher","Manager"));
    shared_ptr<Employee> Namita(new Supervisor(122110,"Namita Gawde","Manager"));
    shared_ptr<Employee> Rumman(new Supervisor(122022,"Rumman Sayed","Manager"));
    shared_ptr<Employee> Rajendra(new Supervisor(111109,"Rajendra Redkar","Manager"));
    shared_ptr<Employee> Sameer(new Supervisor(106213,"Sameer Rajadhyax","Group Lead"));

    Kiran->addWorker(Tushar);
    Kiran->addWorker(Ranjeet);
    Sameer->addWorker(Kiran);
    Sameer->addWorker(Namita);
    Sameer->addWorker(Rumman);
    Sameer->addWorker(Rajendra);

    Sameer->showDetails();

    Sameer->deleteWorker(Rumman);
    Sameer->showDetails();

    return 0;
}

Upvotes: 2

Views: 157

Answers (3)

Alex Shtoff
Alex Shtoff

Reputation: 2640

The problem is probably in your deleteWorker method. You are iterating over the container while modifying it.

I suggest one of two possible solutions

  • Find the indices of the items you wish to remove and store them in a vector in accending order. Then iterate the vector in descending order and remove them.
  • Use std::remove_if in conjunction with vector::erase. You can see an example here

Upvotes: 0

John Zwinck
John Zwinck

Reputation: 249133

Your problem is a very common pitfall of C++ and the STL: you're erasing during an iteration without due care. Here:

    for (auto temp : myTeam)
    {
    if (temp->getID()!=employee->getID())
        ++pos;
    else
        myTeam.erase(myTeam.begin()+pos);
    }

It's not OK in general to erase from an STL container whilst iterating over it. Instead, consider either "marking" the elements to be deleted later, or using integer- or iterator-based iteration so you can control it more precisely. I'd probably go with a simple integer-based for loop here, since you've got this "pos" variable anyway.

Upvotes: 3

Sergey Kalinichenko
Sergey Kalinichenko

Reputation: 726539

Change

else
    myTeam.erase(myTeam.begin()+pos);

to

else {
    myTeam.erase(myTeam.begin()+pos);
    break;
}

to fix the crash (demo on ideone; without break, it crashes with SIGSEGV). The problem is that you continue iterating the vector even after erasing one of its elements, which is not allowed.

Since you are going to delete only one worker (assuming that ID is unique), continuing after the element has been erased wasn't a good idea anyway.

Upvotes: 3

Related Questions