copy constructor messing up my list program

[davBams]

The copy constructor seems to be generating all sorts of error. it keeps taking me to an header file (new_allocator.h). I can actually do without it since i am not generating any copies per se but that will be me running away from the problem and instead of solving it.

  //Author - Dav Bam , DATE, Dev-c++ 5.11, MODDIFICAION HISTORY
#include <list>
#include <sstream>
#include <cstdio>
#include <cstdlib>
#include <iostream>
#include <strings.h>
//add other needful include files
using namespace std;
//start coding from here
class Student
{
	public:
		Student(char *pName, int ssid) 
		{
			name = new char[strlen(pName)+1];
			if (name != 0)
			{
				strcpy(name,pName);
			}
			id = ssid;
		}
		Student(Student& s)
		{
			name = new char[strlen(s.name)+1];
			if (name != 0)
			{
				strcpy(name,s.name);
			}
		 	id = s.id;
		}
		//assignment operator overload
		Student& operator=(Student& s)
		{
			name = new char[strlen(s.name)+1];
			if (name != 0)
			{
				strcpy(name,s.name);
			}
			id = s.id;
		}
		int getId(){return id;}
		string display()
		{
			ostringstream out;
			out<<name<<" - "<<id<<endl;
			return out.str();
		}
	protected:
		char* name;
		int id;
};
//for the sorting process
bool operator<(Student& s1, Student& s2)
{
	return s1.getId()<s2.getId();
}
list<Student> students;
//main calling function
int main(int nNumberofArgs, char* pszArgs[])
{
	students.push_back(*new Student("Papi John", 003));
	students.push_back(*new Student("James Bond", 007));
	students.push_back(*new Student("Peter Parker", 001));
	students.push_back(*new Student("Alan Shearer", 90));
	students.sort();
	//output sorted classes uses an iterator
	list<Student>::iterator iter = students.begin();
	while (iter!= students.end())
	{
		Student s = *iter;
		cout<<s.display()<<endl;
		iter++;
	}
	cin.get();
	return 0;
}

[MikeyBoy]

The copy constructor seems to be generating all sorts of error.

And yet you haven't seen fit to tell us about any of them.

I'm curious - what did you hope to gain by withholding this information?

[dhayden]

Line 7: string.h should be cstring
Line 14: char *pName should be const char *pName because at lines 62-65, you create Students with string literals, which are const char *
Line 23: Student & should be const Student &.
Lines 35-40: This code will break if you try to assign a student to itself. Always check for self assignment.
Line 40: The operator needs to return a value.

[davBams]

//Author - David Bam , DATE, Dev-c++ 5.11, MODDIFICAION HISTORY
#include <list>
#include <sstream>
#include <cstdio>
#include <cstdlib>
#include <iostream>
#include <cstring>
//add other needful include files
using namespace std;

//start coding from here
class Student
{
	public:
		Student(const char *pName, int ssid) 
		{
			name = new char[strlen(pName)+1];
			if (name != 0)
			{
				strcpy(name,pName);
			}
			id = ssid;
		}
		Student(const Student& s)
		{
			name = new char[strlen(s.name)+1];
			if (name != 0)
			{
				strcpy(name,s.name);
			}
		 	id = s.id;
		}
		//assignment operator overload
		Student& operator=(const Student& s)
		{
			deletename();
			name = new char[strlen(s.name)+1];
			if (name != 0)
			{
				strcpy(name,s.name);
			}
			id = s.id;
			return *this;
		}
		void deletename()
		{
			if (name)
			{
				delete name;
				name = 0;
			}
		}
		~Student()
		{
			deletename();
		}
		int getId(){return id;}
		string display()
		{
			ostringstream out;
			out<<name<<" - "<<id<<endl;
			return out.str();
		}
	protected:
		char* name;
		int id;
};
//for the sorting process
bool operator<(Student& s1, Student& s2)
{
	return s1.getId()<s2.getId();
}
list<Student> students;
//main calling function
int main(int nNumberofArgs, char* pszArgs[])
{
	students.push_back(*new Student("Papi John", 003));
	students.push_back(*new Student("James Bond", 007));
	students.push_back(*new Student("Peter Parker", 001));
	students.push_back(*new Student("Alan Shearer", 90));
	students.sort();
	//output sorted classes uses an iterator
	list<Student>::iterator iter = students.begin();
	while (iter!= students.end())
	{
		Student s = *iter;
		cout<<s.display()<<endl;
		iter++;
	}
	cin.get();
	return 0;
}

Thanks @dhayden.
I have made the changes and it worked. i used the deletename() to clear the existing heap before reassignment, it that okay?
i have have additional questions pls:
1. cstring is the new edition of strings.h i.e like cmath replaced math.h. Am I correct?
2. pls wat do u mean by string literals, is it because the students were created by fixed strings?
3. the program is now a little bit slower tho....

[dhayden]

i used the deletename() to clear the existing heap before reassignment, it that okay?

Calling deletename() will prevent a memory leak, but that isn't what I meant. Think about what happens if you declare a Student s and then do s = s;. It calls your assignment operator with this and s refering to the same object. You call deletename(), which clears name. Then line 37 executes name = new char[strlen(s.name)+1];. But since this and s are the same object, that means that s.name is NULL. So strlen will fail and the program will probably crash.

By the way, since name is allocated as an array, deletename should call delete [] name instead of delete name.

cstring is the new edition of strings.h

Correct.

wat do u mean by string literals

A string literal is an expression like "hello world". The type of such an expression is const char *

3. the program is now a little bit slower tho....

On my system it takes about 15ms to load and run.

Finally, you don't need to create Students on the heap in main. Lines 77-80 should probably be:

        students.push_back(Student("Papi John", 003));
        students.push_back(Student("James Bond", 007));
        students.push_back(Student("Peter Parker", 001));
        students.push_back(Student("Alan Shearer", 90));

[davBams]

@dhayden

Thanks a lot.

i totally agree with all ur points.

but pls how to I prevent self assignment, can it be prevented from the assignment operator or do i just have to look out for it?

and yeah allocating students off the heap is not necessary.

thanks again.