0

I have a method involving copying some custom types from one linked list to another, based on very simple criteria. This is to be done using an assignment operator. At the end of the method, this new list is displayed, then deleted. The issue here, is that deleting the new list also deletes the original list's corresponding nodes, which signals to me that either the assignment operator is not functioning as intended, or the method in question has issues.

The method itself appears to work fine. It successfully navigates the copy of the original list, identifies Students who meet the desired criteria, and adds them to the newList. It then goes on to display the expected contents of newList just fine, and exits the method without error. The issue, then, is that the corresponding nodes in the original list are deleted too, not just those in newList. This can be verified by the fact that displaying the list crashes when reaching one of the selected nodes, or by exiting the program before it crashes -- this saves the original list to a text file, which when opened, shows that the entries have been replaced with incorrect values.

The method in question:

void moreThan80Credits()
{
    Container* tempList = list;
    Container* newList = new Container();
    Container* studentToAdd = new Container();
    Container* head = new Container();

    while(tempList) {
        if(tempList->student->getCredits() > 80) {
            studentToAdd->student = tempList->student;
            if(!newList->student) {
                newList->student = studentToAdd->student;
                head = newList;
            } else {
                while(newList->next) {
                    newList = newList->next;
                }
                newList->next = studentToAdd; // adding to tail
            }
        }

        tempList = tempList->next;
    }

    newList = head; // returning to beginning of newList, to display.

    if(!newList->student) {
        cout << endl << "There are no students with more than 80 credits." << endl;
        return;
    }

    cout << "The students with more than 80 credits are:" << endl;

    while(newList) {
        newList->student->displayInfo();
        Container* removal = newList;
        newList = newList->next;
        delete removal->student; // displaying and deleting, together
        delete removal;
    }

    return;
}

And then the assignment operator:

Student& Student::operator=(const Student& s) {
    if (this == &s) return *this; // self assignment check
    this->name = s.name;
    this->rollNo = s.rollNo;
    this->level = s.level;
    this->credits = s.credits;
    return *this;
}

And the relevant constructors:

Student::Student() { // default constructor
    // empty, everything should be null
}

Student::Student(const Student& that) { // copy constructor
    this->name = that.name;
    this->rollNo = that.rollNo;
    this->level = that.level;
    this->credits = that.credits;
}

As stated before, the deletion section at the end of moreThan80Credits() deletes both copies of the node. I am wondering why this is, and how the code provided can be modified to make this behavior cease. Thank you all.

Seymour Guado
  • 246
  • 2
  • 13
  • 2
    https://stackoverflow.com/questions/4172722/what-is-the-rule-of-three – πάντα ῥεῖ Apr 21 '19 at 20:50
  • Funny, I was reading your similar predicament before asking. In that thread, the only differences I pick up are my operator= not using const (trying that throws oh so many errors), and that setting pointers equal to each other causes issues like mine. I wonder if that is the issue, then? And how to fix it. – Seymour Guado Apr 21 '19 at 20:54
  • To fix it don't use raw pointers, and prefer leaving anything else to the compiler generated constructors and assignment operators. – πάντα ῥεῖ Apr 21 '19 at 20:56
  • My apologies, but how would I then assign them without using "raw pointers?" – Seymour Guado Apr 21 '19 at 20:59
  • 2
    Why do you think you need pointers at all? – πάντα ῥεῖ Apr 21 '19 at 21:00
  • My methods return pointers, so not using pointers here would mean modifying methods, which rely on methods using pointers... so I would love to not, but at this stage I don't know if I could avoid it. I think. – Seymour Guado Apr 21 '19 at 21:04
  • 1
    You could always switch to using a shared_ptr instead of a `raw pointer` then multiple areas could have a copy without being destroyed prematurely. [shared_ptr](https://en.cppreference.com/w/cpp/memory/shared_ptr) – estabroo Apr 21 '19 at 22:15
  • I can give those a try, though clearly this is a glaring gap in my knowledge that is apparently too base for the community to try addressing. I need to know how to fix the issue here, because I'm running into similar problems elsewhere. I've edited my code to include the copy constructor as well. – Seymour Guado Apr 21 '19 at 22:19
  • You ask about the behavior of the deletion code in `moreThan80Credits()`. Well, that's easy -- it ends up calling `Student::~Student()`. We can't help you any further, because you didn't show that destructor code. – Ben Voigt Apr 21 '19 at 22:39
  • What error does making the assignment operator const-correct as `Student& operator=(const Student&)` cause? – Ben Voigt Apr 21 '19 at 22:41
  • Ah, with some tinkering I was able to fix that bit. I shall update my question with that edited section. As for the destructor, there isn't one. I believed one wasn't explicitly necessary, as the only values within the Student class are ints and strings? – Seymour Guado Apr 21 '19 at 22:47

0 Answers0