Thread: copy constructor prolbem(??) in a class Name

Hi everyone,
I'm trying to create a Name class that holds the name and the surname of a person.
Name.h

Code:

#ifndef Name_h
#define Name_h

class Name
{
      public:
         Name(Name& nm);
         Name(char* n = "nothing" ,char* sn = "nothing");
         ~Name(void);
         void  Setname(char* n);
         void  Setsname(char* sn);
         char* Getname(void);
         char* Getsname(void);
         void  Print(void);
         Name& operator =(Name& nm);
         
      private:
            char* name;         // name 
            char* sname;       // surname
};

#endif

Name.cpp

Code:

#include "Name.h"
#include <cstring>
#include <iostream>
using namespace std;

Name::Name(char* n ,char* sn)
{
     cout << "Constructor Called!" << endl;
                 
     name  = new char[strlen(n)+1];
     sname = new char[strlen(n)+1];
     
     strcpy(name, n);
     strcpy(sname, sn);
}

Name::~Name(void)
{
    cout << "Destructor Called!" << endl;
    delete name;
    delete sname;
}

Name::Name(Name& nm)
{
    cout << "Copy Constructor called" << endl;
        
    this->name = new char[strlen(nm.Getname()) + 1];
    this->sname = new char[strlen(nm.Getsname())+1];
    
    strcpy(name, nm.Getname());
    strcpy(sname, nm.Getsname());
}


void Name::Print(void)
{
     cout << "   Name = " << name << endl
          << "Surname = " << sname << endl;
}

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

void Name::Setsname(char* sn)
{
     delete sname;
     sname = new char[strlen(sn) + 1];
     strcpy(sname, sn);
}

char* Name::Getname(void)
{
      return name;
}

char* Name::Getsname(void)
{
      return sname;
}
//==============================================================================
Name& Name::operator =(Name& nm)
{
    delete this->name;
    delete this->sname;
    
    this->name = new char[strlen(nm.Getname()) + 1];
    this->sname = new char[strlen(nm.Getsname())+1];
    
    strcpy(name, nm.Getname());
    strcpy(sname, nm.Getsname());

   return *this;
}

main.cpp

Code:

#include <iostream>
#include "Name.h"
using namespace std;


int main(void)
{
   
    Name Obj1;
    
    Obj1.Setname("Abc");
    Obj1.Setsname("Defghi");
    Obj1.Print();
    
    Name Obj2(Obj1);
    Obj2.Print();    
    Obj1.Print();
    Obj1.~Name();
    Obj2.Print();
    
    Name Obj3;
    
    Obj3 = Obj2;
    
    Obj3.Print();
    
    cin.get();
    return 0;
}

It runs in Dev-C++ with no errors but in VS Studio 2005 it runs, shows me the results normally, i press enter to close the window and at that time it shows me a message "debug assertion failed..."

Can you please help me? I have no idea what to do...

Well , yes it would be very nice if I could use strings for this exercise.
But (I forgot to say) , my teacher doesn't let us do that and (for practice reasons, i suppose) we use char* ....

Well, one problem is that you use new[] for the strings, but then use delete instead of delete[]. new[] must be paired with delete[] and new with delete.

You're right , i fixed this.

In your constructor that takes two strings, you allocate the size for sname based on a strlen() of name. This can be problematic since name and sname are probably not guaranteed to be of equal length.

You are very observant! I'm looking in my code a lot of time and i couldn't see the 's' missing

Also, I noticed that in main() you called the destructor for Obj1 explicitly. There is no need for that.

I want to test if by deleting one object, i delete the other , which is wrong and occurs with the default copy constructor.

Unfortunately the problem still exists. Maybe i should check before deleting sth if it's NULL? Is there any problem deleting NULL pointers?

I'm very happy, I think I solved my problem!! (with your great help of course )

At first, I commented the line where i explicitly deleted Obj1. I saw that the error message was not shown any more. So I supposed something is wrong with the destructor.
I remembered I readed from a book that I should set to zero the deleted pointers , so here is my new destructor

Code:

Name::~Name(void)
{
    cout << "Destructor Called!" << endl;
    delete [] name;
    delete [] sname;
	name = 0;
	sname = 0;
}

Everything works and looks pretty fine!

Thanks for all your help and your time laserlight!

Yes , I fixed the line

Code:

 sname = new char[strlen(sn)+1];

but if forgot to change the

Code:

delete name;

to

Code:

delete [] name;

in the Name::Setname! Thx for the info , another bug , corrected!

Oh , and I also changed operator = to this

Code:

Name& Name::operator =(Name& nm)
{
	if( !strcmp(name, nm.Getname())  && !strcmp(sname, nm.Getsname()))
	   return *this;	                                                  
         
    this->name = new char[strlen(nm.Getname()) + 1];
    this->sname = new char[strlen(nm.Getsname())+1];
    
    strcpy(name, nm.Getname());
    strcpy(sname, nm.Getsname());

	return *this;
}

in case we have an expression like this TheSame = TheSame ;

Firstly the signature should be:
Code:

Code:

Name& Name::operator =(const Name& nm)

Assigning shouldn't change what is being assigned. So don't break compatibility with const-correct code that expects this.

You're right but when I do this , there are six error messages
"cannot convert 'this' pointer from 'const Name' to 'Name &' " for every line in the operator= body....

More common self-test would be:

Code:

    if (this == &other) //they are the same

Do you mean something like this ?

Code:

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

Oh, and you leak memory here. this->name and this->sname already point to some memory and you don't free that.

Can you make it more clear, i don't understand! Should I use name and sname instead?

(By the way, why not name your variables and functions with full words, e.g surname or last_name?)

I agree , i will change the sname when i finish and i'll try to give better names generally....

Originally Posted by anon

Oh, and you leak memory here. this->name and this->sname already point to some memory and you don't free that.

Originally Posted by kantze

Can you make it more clear, i don't understand! Should I use name and sname instead?.

Look at your operator=()

Code:

Name& Name::operator =(Name& nm)
{
	if( !strcmp(name, nm.Getname())  && !strcmp(sname, nm.Getsname()))
	   return *this;	                                                  
         
    this->name = new char[strlen(nm.Getname()) + 1];
    this->sname = new char[strlen(nm.Getsname())+1];
    
    strcpy(name, nm.Getname());
    strcpy(sname, nm.Getsname());

	return *this;
}

you're allocating memory to name and sname, however, you haven't deleted their existing contents first, so you've got a memory leak.

Ok, i understand now why i was leaking memory (you both made it clear) and I changed my code ( hope it's perfect now ).

Code:

Name& Name::operator =(const Name& nm)
{
	if(this == &nm) 
        return *this; 
     
	delete [] name;
	delete [] sname;
     
    
    name = new char[strlen(nm.Getname()) + 1];
    sname = new char[strlen(nm.Getsname())+1];
    
    strcpy(name, nm.Getname());
    strcpy(sname, nm.Getsname());

	return *this;
}

Code:

char* Name::Getsname(void) const
{
      return sname;
}

char* Name::Getname(void) const
{
      return name;
}