How to make a copy function for a class with vector of pointers?

Question

I have a class, Somec, with a vector of pointers to items. Each item in the vector is from another class type, Myitem.

I want to make a copy function for Somec, but I am having some problems.

Also, I have a copy function for Myitem, do I have to use it in the copy function for Somec?

What I have tried so far:

class Somec {
    string Name;
    std::vector<Myitem*> Items;

public:
    Somec(const Somec& somec); // my copy function
};

/// this the clas to my item :
class Myitem {
    Itemk itemk;
public:
    // ... some fuction 
};

// now I want to make a copy function for Somec

Somec::Somec(const Somec& somec) {
    int size = somec.Items.size();
    this->Items = new (vector*) (size); // i get an error here
    this->Name = somec.Name;
    for(int i = 0; i < size; i++) {
        *Items.at(i) = *somec.Items.at(i);
    }
}

UPDATE: I made the copy function just like Remy Lebeau told me to, but when I try to test this function the code stops working. This is how I am testing it:

class Somec {
    string Name;
    std::vector<Myitem*> Items;

public:    
    Somec(const Somec& somec); // my copy function
};

Somec::Somec(const Somec& somec) { // new copy function for somec
    int size = somec.Items.size();
    this->Name = somec.Name;
    for(int i = 0; i < size; i++) {
        Items.push_back(new Myitem(*somec.Items[i]));
    }
}

// create item function
void Somec::createitem(char* name, const int& Time, const int& level) {
    try{
        Myitem* ORitem=new Myitem(name, Time, level);
        Somec::Items.push_back(ORitem);        
    }
    catch(MemoryProblemException& error) {
        throw SomecMemoryProblemException();
    }
}

std::vector<Myitem*> Somec::getAllItems() const {
    return Items;
}

/// this the class to my item :
class Myitem {
    Itemk itemk;
public:
    // ... some fuction 
};

// my copy function to MYitem 
Myitem::Myitem(const Myitem& item){
    Myitem* currie = Myitem::clone_item();
    curritem->item = itemCopy(item.item);
    if (!curritem->item) {
        throw itemMemoryProblemException();
        return;
    }
}

void testCopy() {
    Somec somec("name1")
    somec.createitem((char*) "item1", 30, 1, 2);
    Somec temp(somec);
    int x=0;
    if ( std::equal(somec.getAllItems().begin() + 1, somec.getAllItems().end(), somec.getAllItems().begin()) ) {
        x++;
    }
    ASSERT_TRUE(x==1);
}

What is my problem? I mean, I did the copy function, I think it is true. But why does the code stop working? Am I testing this in the right way?

My createitem function, I am 100% sure about it, actually.

I am just trying to add items to the the vector in Somec and check if this happened correctly. I learned that in C++, we need a copy constructor, so I wrote one, and thought about testing it since this is my first time doing one.

Answer 1

The default compiler-generated copy constructor already does exactly what your copy constructor is doing manually. So it is redundant code.

However, assuming Somec owns the items, then you do need to define a custom copy constructor, and it needs to allocate new Myitem objects and copy the existing data into them, eg:

Somec::Somec(const Somec& somec) {
    this->Name = somec.Name;
    std::size_t size = somec.Items.size();
    this->Items.reserve(size);
    for(std::size_t i = 0; i < size; i++) {
        Items.push_back(new Myitem(*somec.Items[i]));
    }
}

You will have to do something similar in an overriden operator= as well. You can utilize the copy constructor to help you:

Somec& Somec::operator=(const Somec& somec) {
    if (&somec != this) { 
        Somec temp(somec);
        std::swap(this->Name, temp.Name);
        std::swap(this->Items, temp.Items);
    }
    return *this;
}

And, of course, make sure you free the owned items in the destructor:

Somec::~Somec() {
    std::size_t size = somec.Items.size();
    for(std::size_t i = 0; i < size; i++) {
        delete Items[i];
    }
}

See Rule of Three for more details about why this trio is needed.

Answer 2

If you want to copy elements from the vector of the other instance to the vector of your current instance in the copy constructor, then simply do this

class Somec {
    string Name;
    std::vector<Myitem> Items;
public:
    Somec(const Somec& somec); // my copy function
};

i.e. make the vector of values instead of pointers.

If for some reason you have to have to work with pointers, because maybe the copy constructor is deleted or something similar. Then still use this approach, but just don't copy the contents of in the copy constructor, let them get constructed and then assign them

Somec(const Somec& somec) {
    this->Items.resize(somec.Items.resize());
    for (std::size_t i = 0; i < somec.Items.size(); ++i) {
        this->Items[i] = somec.Items[i];
    }
    // or simply
    this->Items = somec.Items;
}