Copy-constructor doesn't work, can't modify the copy

Question

I need help, because I've been trying to solve this all day, but I really don't know how to do it. The problem is that I made a copy- constructor; it copies well, but when I want to modify the copy by adding an element at the end it doesn't work, the elements aren't printed.

It works when I add an element at the beginning and when I try to add an element by using the method add_at, it add the element in the both lists. I really don't know where the problem is; is it in the constructor or in the methods?

You can think that my code is strange but I had to make add methods like this...

Here's the code:

.h file

class CList
{
    SElement* first;
    SElement* last;
    int lenght;
    int value;

    public:
        CList();
        CList(CList const& clist);
        ~CList();

        // add methods
        void add(int value);
        void add_first(int value);
        void add_at(int at, int value);
        void addObject(void* object);
        void addObject_first(void* object);
        void addObject_at(int at, void* object);

        // delete methods
        void del_last();
        void del_first();
        void del_index(int index);
        void del_element(void* value);
        void del_ielement(int& value);

        // getery
        int iget_lenght();
        int iget_element(int const index);
        SElement* sget_element(int index);
        void* get_element(int index);

        void set_element(int const index, int& value);

        void pop();
        void push(int value);

        void printCList();

    private:
        void priv_push(int & value);
};

methods concerned in the .cpp

CList::CList()
{
    first = new SElement;
    last = new SElement;
    first = last = NULL;
    lenght = 0;
    value = 0;
}

CList::CList(CList const& clistcopy)
{
    this->lenght = clistcopy.lenght;
    this->value = clistcopy.value;
    first = new SElement;
    last = new SElement;
    // pointer = last = NULL;
    *first = *clistcopy.first;
    *last = *clistcopy.last;
}

CList::~CList()
{
    SElement* el;
    while (first)
    {
        el = first->next;
        delete first;
        first = el;
    }
    delete last;
}

// ADD FIRST
void CList::addObject_first(void* object)
{
    SElement* el = new SElement;

    el->next = first;
    el->previous = NULL;
    el->object = object;

    if (first != NULL)
        first->previous = el;
    first = el;

    if (!last) last = first;
    lenght++;
}
void CList::add_first(int wartosc)
{
    value = wartosc;
    int* wsk = &value;
    int* pnt = new int;
    *pnt = *wsk;
    addObject_first(pnt);
}

// ADD LAST
void CList::addObject(void* object)
{
    if (!last)
    {
        SElement* el = new SElement;
        el->object = object;
        el->next = NULL;
        el->previous = NULL;
        first = last = el;
    }
    else
    {
        SElement* el = last;
        el->next = new SElement;
        el = el->next;
        el->next = NULL;
        el->previous = last;
        el->object = object;
        last = el;
        el = NULL;
    }

}
void CList::add(int wartosc)
{
    value = wartosc;
    int* wsko = &value;
    int* pnt = new int;
    *pnt = *wsko;
    addObject(pnt);
}

// ADD AT
void CList::addObject_at(int index, void* value)
{
    SElement* el2 = new SElement;
    if (index - 1 == 0)
        addObject_first(value);
    else if (index - 1 == lenght)
        addObject(value);
    else if (index < 0 || index > lenght)
        cout << "blad : nie mozesz dodac na indeks poza lista" << endl;
    else
    {
        SElement* el = sget_element(index - 1);
        el2->next = el->next;
        el2->previous = el;
        el2->object = value;
        el->next = el2;
        el->next->previous = el2;
        if (!(el2->next)) last = el2;
        lenght++;
    }
}

void CList::add_at(int at, int wartosc)
{
    value = wartosc;
    int* wsko = &value;
    int* pnt = new int;
    *pnt = *wsko;
    addObject_at(at, pnt);
}
void CList::printCList()
{
    if (first == NULL)
        cout << "Lista jest pusta" << endl;
    else
    {
        SElement* el;
        el = first;
        while (el->next != NULL)
        {
            cout << el->get_value();
            cout << " ";
            el = el->next;
        }
        cout << el->get_value();
    }
    cout << endl;
}

the main

int main()
{
    CList list = CList();

    list.add_first(1);
    list.add_first(0);
    list.add_first(2);
    list.add_first(4);
    list.add(5);
    list.add(10);

    CList list2 = CList(list);

    list.printCList();
    list2.printCList();
    list2.add(100);
    list2.add(150);

    cout << list.iget_lenght()  << endl;
    cout << list2.iget_lenght() << endl;
    list.printCList();
    list2.printCList();

    system("pause");
    return 0;
}

[OT]: memleak in `CList::CList()`, typo: _lenght_ -> _length_ — Jarod42, Jan 05 '14 at 00:09
You should also implement operator=, but I think you are not using it in your example. — marcinj, Jan 05 '14 at 00:12
Your copy constructor only recreates `first` and `last` and not the entire list. — Jarod42, Jan 05 '14 at 00:12
@sincrow: yes you may do something similar to your print method, but adding the elements into the new list instead. — Jarod42, Jan 05 '14 at 00:21
@AlanStokes I initiate first and last with NULL, is there a problem or a better way to do it ? — sincrow, Jan 05 '14 at 00:22
@Jarod42 So if I understand after having created my new pointers(first and last), I similar loop like this in my print method? (i will try it tomorow noe i go sleep^^) — sincrow, Jan 05 '14 at 00:24

kuroi neko · Accepted Answer · 2014-01-05T00:41:50.897

It seems to me your you should cleanup your code before trying to pinpoint the problem.

For instance:

CList::CList()
{
    first = new SElement; // <-- these two lines
    last = new SElement;  // <-- are useless
    first = last = NULL;
    lenght = 0;
    value = 0;
}

Besides, the semantics of your copy constructor are wrong. Here you simply copy the head and tail pointers, but you don't copy the objects inside the list. More precisely, you seem to try to copy them, but your code is also wrong there:

first = new SElement;      // allocate an SElement
*first = *clistcopy.first; // copy model's element into it
// since you don't assign first to a permanent variable, it is lost forever

Anyway, even with a proper head/tail copy, your copy would still reference the objects stored inside the model.
Modifying your "copy" will also change the elements of the original list, which is plain wrong.
Besides, since the length and value fields have indeed been duplicated, they will become inconsistent with the state of the linked elements as soon as you modify either of the list.

To make a real copy constructor, you need to browse through the list and duplicate each element. For instance:

CList::CList(CList const& clistcopy)
{
    // create an empty list
    this->lenght = 0;
    this->value = 0;
    this->first = this->last = NULL;

    // browse through all elements of the model
    for (SElement * elt = clistcopy->first ; elt != NULL ; elt = elt->next)
    {
        // create a copy of each element
        elt_copy = new SElement (elt);

        // and insert it into the copy of the list
        this->add (elt_copy);
    }
}

EDIT: this is just pseudo-C++ to illustrate the principle. I doubt it will even compile in its curent state.

I don't know if that will be enough to solve all your problems, but that will certainly remove a major design flaw from your copy constructor.

The idea for the copy is here, the detail of the implementation is wrong... (As I assume that it is a homework, I don't correct it now). — Jarod42, Jan 05 '14 at 00:34
@Jarod42 Agreed, this is more like pseudo-code off the top of my head. I would be surprised if it even compiled :). — kuroi neko, Jan 05 '14 at 00:39

score 0 · Answer 2 · answered Jan 05 '14 at 00:25

0

There are deep copies and shallow copies. You have made something that is halfway between, and has the disadvantages of both.

In a deep copy (which is what you seem to have in mind), the entire linked list would be copied.

In a shallow copy, only the pointers into the list would be copied, so that the two CLists would share one linked list.

In your version you copy the first and last elements (recklessly assuming that they exist) and share the rest.

Read up on copying a linked list, and implement a deep copy.

answered Jan 05 '14 at 00:25

Beta

96,650
16
149
150

well in that case a shallow copy would rather be equivalent to taking a reference to the original list. If you duplicate the head and the tail without duplicating the elements, both copies will become inconsistent as soon as one of them modifies its `head` or `tail`. The inconsistency will be made obvious even quicker by the `length` counter, that will diverge at the first element insertion/removal. – kuroi neko Jan 05 '14 at 00:36
@kuroineko: 1) Yes or no, depending on what you mean by a reference to the list. 2) Probably not, depending on what kind of modification and whether you are using "head" and "tail" to refer to elements or pointers. 3) Yes. (You have described some shortcomings of shallow copies in this context; I agree, they're troublesome.) – Beta Jan 05 '14 at 02:13
I meant a plain C++ reference, i.e. a second reference to the same object. It seems to me the CList object makes no sense as a standalone entity. Imagine you clone the list when there is only one element inside. You remove the element using one of the list, and then delete it (why not, since you are supposed to own it once you got it out of the list). Now the other copy's head and tail will point to the still warm corpse of that element. See what I mean? But anyway, I think we are broadly agreed on the dangers of unsufficiently defined copy semantics :). – kuroi neko Jan 05 '14 at 02:46
@kuroineko: I understand what you mean, but your language is unclear. The list is not an object, and you cannot have a reference (or pointer) to it. "Clone" generally means a deep copy. And (since you seem to be using "head" and "tail" to mean pointers) modifying a *pointer* is not the same as modifying *the thing the pointer points to*. – Beta Jan 05 '14 at 02:55
Well yes you're right, I should have said from the start that this reference had nothing to do with the copy constructor. I mean, either you simply reference the list twice (with no copy constructor at all) or you do a deep copy in the copy constructor. It seems to me there is no middle term in this particular case. I also inadvertently used `head`/`tail` while the code uses `first`/`last`. I meant the actual properties of the `CList` object that are *pointers* to the 1st and last elements of the list. these pointers will indeed evolve according to the list operations, right? – kuroi neko Jan 05 '14 at 03:03

Copy-constructor doesn't work, can't modify the copy

2 Answers2