Doubly Linked List Copy Constructor And Overloading = Operator

[zspar]

We were given Student class and Student Roll class (header files included below) The only things I cant figure out so far are how to overload the = operator for StudentRoll, and the copy constructor for studentRoll. Heres what I have, any suggestions are appreciated.

//Student.h 
#ifndef STUDENT_H
#define STUDENT_H

#include <string>

class Student {

 public:
  Student(const char * const name, int perm);

  int getPerm() const;
  const char * const getName() const;

  void setPerm(const int perm);
  void setName(const char * const name);

  Student(const Student &orig);
  ~Student();
  Student & operator=(const Student &right);

  std::string toString() const;

 private:
  int perm;
  char *name; // allocated on heap                                                                                                                                                   

};


#endif


//studentRoll.h
#ifndef STUDENTROLL_H
#define STUDENTROLL_H

#include <string>
#include "student.h"

class StudentRoll {

 public:
  StudentRoll();
  void insertAtTail(const Student &s);
  std::string toString() const;

  StudentRoll(const StudentRoll &orig);
  ~StudentRoll();
  StudentRoll & operator=(const StudentRoll &right);

 private:
  struct Node {
    Student *s;
    Node *next;
  };
  Node *head;
  Node *tail;
};


#endif

//studentRoll.cpp
#include <string>
#include "studentRoll.h"
#include <sstream>
#include <iostream>
StudentRoll::StudentRoll() {
  head = tail = NULL;
}

void StudentRoll::insertAtTail(const Student &s) {
    /*  tail -> next = new Node;
    tail = tail -> next;
    tail -> s = s;
    tail -> next = null;

    */
    Node* n = new Node;
    n -> s = new Student(s);

    n -> next = NULL;

    if (head == NULL){
        head = n;
    }
    if(tail != NULL){
        tail -> next = n;
    }
    n -> next = NULL;
    tail = n;
}

std::string StudentRoll::toString() const {
    std::ostringstream oss;
    oss << "[";
    Node *p = head;
    while (p){
        oss << p -> s -> toString();
        if(p != tail) oss << ",";
        p = p -> next;
    }
    oss << "]";
    delete p;
    return oss.str();

}

StudentRoll::StudentRoll(const StudentRoll &orig) {
    Node *p1 = 0;
    Node *p2 = 0;

    if (orig.head == NULL){
        head = NULL;
        tail = NULL;
    }
    else{

        head = new Node;
        head -> s = new Student(*(orig.head -> s));
        std::cout << "got here";
        p1 = head;
        p2 = orig.head -> next;
    }
    while(p2){
        p1 -> next = new Node;
        p1 = p1 -> next;
        p1 -> s = new Student(*(orig.head -> s));

        p2 = p2 -> next;
    }
    p1 -> next = NULL;
    tail = p1;
    delete p1;
    delete p2;

}

StudentRoll::~StudentRoll() {
    Node *current = head;
    while ( current != 0 ){
        Node* next =  current->next;
        delete current;
        current = next;
    }

}


StudentRoll & StudentRoll::operator =(const StudentRoll &right ) {
  // The next two lines are standard, and you should keep them.
  // They avoid problems with self-assignment where you might free up 
  // memory before you copy from it.  (e.g. x = x)

  if (&right == this) 
    return (*this);

  // TODO... Here is where there is code missing that you need to 
  // fill in...

        Node *p1 = 0;
        Node *p2 = 0;

        if (right.head == NULL){
                head = NULL;
                tail = NULL;
        }
        else{

                head = new Node;
                head -> s = new Student(*(right.head -> s));

                p1 = head;
                p2 = right.head -> next;
        }
        while(p2){
                p1 -> next = new Node;
                p1 = p1 -> next;
                p1 -> s = new Student(*(p2 -> s));

                p2 = p2 -> next;
        }
        p1 -> next = NULL;
        tail = p1;
        delete p1;
        delete p2;  
  // KEEP THE CODE BELOW THIS LINE
  // Overloaded = should end with this line, despite what the textbook says.
  return (*this); 

}

This was my most recent attempt at implementing the copy constructor:

StudentRoll::StudentRoll(const StudentRoll &orig) {
  head = NULL;
  Node* n = orig.head;
  while (n != NULL) {
    insertAtTail(n -> s);
    n = n -> next;
}

Any suggestions are appreciated.

[2kaud]

What is the issue with the copy constructor? insertAtTail() sets head and tail as appropriate. There is an edge case where the list is empty (ie head is null) in that case insertAtTail() isn't called and so tail isn't set.

Couple of comments.

For a pointer rather than set to NULL its recommended to use nullptr.

The while loop could be written as a for loop

StudentRoll::StudentRoll(const StudentRoll &orig) {
    head = tail = nullptr;

    for (auto n = org.head; n != nullptr; n = n->next)
        insertAtTail(n->s);
}

Once the copy constructor is working and you have a correct destructor, coding an operator=() is quite trivial. Consider

StudentRoll& operator=(StudentRoll right) {
    swap(head, right.head);
    swap(tail, right.tail);

    return *this
}

Note the change to the definition of operator=().

PS The StudentRoll::tostring() has a major bug!

PPS If you absolutely cannot change the operator=() definition and need to keep the supplied bits in the function, then consider

StudentRoll & StudentRoll::operator =(const StudentRoll &right ) {
    if (&right == this) 
        return *this;

    StudentRoll temp(right);
    
    swap(temp.head, head);
    swap(temp.tail, tail);

    return *this;
}

[zspar]

What is the issue with the copy constructor? insertAtTail() sets head and tail as appropriate. There is an edge case where the list is empty (ie head is null) in that case insertAtTail() isn't called and so tail isn't set.

Couple of comments.

For a pointer rather than set to NULL its recommended to use nullptr.

The while loop could be written as a for loop

StudentRoll::StudentRoll(const StudentRoll &orig) {
    head = tail = nullptr;

    for (auto n = org.head; n != nullptr; n = n->next)
        insertAtTail(n->s);
}

Once the copy constructor is working and you have a correct destructor, coding an operator=() is quite trivial. Consider

StudentRoll& operator=(StudentRoll right) {
    swap(head, right.head);
    swap(tail, right.tail);

    return *this
}

Note the change to the definition of operator=().

PS The StudentRoll::tostring() has a major bug!

PPS If you absolutely cannot change the operator=() definition and need to keep the supplied bits in the function, then consider

StudentRoll & StudentRoll::operator =(const StudentRoll &right ) {
    if (&right == this) 
        return *this;

    StudentRoll temp(right);
    
    swap(temp.head, head);
    swap(temp.tail, tail);

    return *this;
}

Thanks for the suggestions!
When I try to use the insertAtTail function it tells me error: no matching function call to 'StudentRoll::insertAtTail(Student*&)'

Also what is the major bug you found in toString()? All of the tests for that function have passed.

[2kaud]

Also what is the major bug you found in toString()? All of the tests for that function have passed.

delete p;

Well perhaps not major... but the delete shouldn't be there!

insertAtTail function it tells me error: no matching function call to 'StudentRoll::insertAtTail(Student*&)

As you haven't given the implement of student class I can't try it. But try

insertAtTail(*(n->s));

[2kaud]

There is also a problem with the destructor.

In insertAtTail(), there is

n -> s = new Student(s);

but in the destructor this allocated memory isn't deleted. Consider

StudentRoll::~StudentRoll() {
    Node *current = head;
    while ( current != 0 ){
        Node* next =  current->next;
        delete current->s;
        delete current;
        current = next;
    }
}

Also, the thread title refers to doubly linked list, but the code is for a singly forward linked list?

Also note that in insertAtTail() there is an extra superfluous n_next = NULL;

[2kaud]

I've provided an implementation of class Student so I can get it to compile OK.

Note it would be better if the struct Node had its own destructor - then you wouldn't need to explicitly delete student in the destructor of StudentRoll. Consider

struct Node {
		Student *s = nullptr;
		Node *next = nullptr;

		~Node() {
			delete s;
		}
...
StudentRoll::~StudentRoll() 
{
	for (auto current = head; current != nullptr;) {
		const auto next = current->next;
		delete current;
		current = next;
	}
}

This will still call Student destructor from StudentRoll destructor because now StrudentRoll destructor will implicitly automatically call Node destructor - which calls Student destructor.

Also, there are no class functions for move constructor and move assignment. As swap is now used, it is common and useful to provide a specific swap function for the classes.

Togther with these and previous comments, consider

#pragma warning (disable : 4996)

#include <string>
#include <iostream>
#include <cstdlib>
#include <algorithm>
using namespace std;

class Student {
public:
	Student(const char* const name, int perm);
	Student(const Student& orig);
	Student(Student&& orig);
	~Student();

	int getPerm() const;
	const char* const getName() const;

	void setPerm(const int perm);
	void setName(const char* const name);

	Student& operator=(const Student& right);
	Student& operator=(Student&& right);

	string toString() const;

private:
	int perm = 0;
	char *name = nullptr;

	void swap(Student& orig);
};

class StudentRoll {
public:
	StudentRoll() = default;
	StudentRoll(const StudentRoll &orig);
	StudentRoll(StudentRoll&& orig);
	~StudentRoll();

	void insertAtTail(const Student &s);
	string toString() const;

	StudentRoll& operator=(const StudentRoll &right);
	StudentRoll& operator=(StudentRoll&& right);

	void display() const;

private:
	struct Node {
		Student *s = nullptr;
		Node *next = nullptr; 

		~Node() {
			delete s;
		}
	};

	Node *head = nullptr;
	Node *tail = nullptr;

	void swap(StudentRoll& right);
};

Student::Student(const char* const n, int p) : perm(p), name(new char[strlen(n) + 1])
{
	strcpy(name, n);
}

Student::Student(const Student &orig) : perm(orig.perm), name(new char[strlen(orig.name) + 1])
{
	strcpy(name, orig.name);
}

Student::~Student()
{
	delete name;
}

int Student::getPerm() const
{
	return perm;
}

const char* const Student::getName() const
{
	return name;
}

void Student::setPerm(const int p)
{
	perm = p;
}

void Student::setName(const char* const n)
{
	delete name;
	name = new char[strlen(n) + 1];
	strcpy(name, n);
}

Student& Student::operator=(const Student& right)
{
	if (&right == this)
		return *this;

	Student temp(right);

	swap(temp);
	return *this;
}

string Student::toString() const 
{
	return name;
}

void Student::swap(Student& orig)
{
	std::swap(perm, orig.perm);
	std::swap(name, orig.name);
}

Student::Student(Student&& orig)
{
	swap(orig);
}

Student& Student::operator=(Student&& right)
{
	swap(right);
}

void StudentRoll::insertAtTail(const Student &s) 
{
	auto n = new Node;

	n->s = new Student(s);

	if (head == nullptr)
		head = n;

	if (tail != nullptr)
		tail->next = n;

	tail = n;
}

string StudentRoll::toString() const
{
	string str = "[";

	for (auto p = head; p != nullptr; p = p->next) {
		str += p->s->toString();
		if (p != tail)
			str += ",";
	}

	str += "]";
	return str;
}

StudentRoll::StudentRoll(const StudentRoll &orig)
{
	head = tail = nullptr;

	for (auto n = orig.head; n != nullptr; n = n->next)
		insertAtTail(*(n->s));
}

StudentRoll::~StudentRoll() 
{
	for (auto current = head; current != nullptr;) {
		const auto next = current->next;
		delete current;
		current = next;
	}
}

StudentRoll & StudentRoll::operator =(const StudentRoll &right)
{
	if (&right == this)
		return *this;

	StudentRoll temp(right);

	swap(temp);

	return *this;
}

void StudentRoll::display() const
{
	for (auto l = head; l != nullptr; l = l->next)
		cout << l->s->getName() << endl;

	cout << endl;
}

void StudentRoll::swap(StudentRoll& right)
{
	std::swap(head, right.head);
	std::swap(tail, right.tail);
}

StudentRoll::StudentRoll(StudentRoll&& orig)
{
	swap(orig);
}

StudentRoll& StudentRoll::operator=(StudentRoll&& right)
{
	swap(right);
}

int main()
{
	StudentRoll sr;

	sr.insertAtTail(Student("name1", 1));
	sr.insertAtTail(Student("name2", 2));
	sr.insertAtTail(Student("name3", 3));
	sr.insertAtTail(Student("name4", 4));

	cout << "sr" << endl;
	sr.display();

	StudentRoll sr1(sr);

	cout << "sr1" << endl;
	sr1.display();

	sr.insertAtTail(Student("name5", 5));
	sr.insertAtTail(Student("name6", 6));

	cout << "sr" << endl;
	sr.display();

	sr1 = sr;
	cout << "sr1" << endl;
	sr1.display();
}