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...

I suggest solving your problems in one go by using std::string.

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. 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. Also, I noticed that in main() you called the destructor for Obj1 explicitly. There is no need for that.

EDIT:
Oh, and your copy constructor and copy assignment operator should take their arguments by const reference.

Quote:

---

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.

Quote:

---

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 :D:D:D

Quote:

---

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?

Quote:

---

Maybe i should check before deleting sth if it's NULL? Is there any problem deleting NULL pointers?

---

No need, it should be safe to delete null pointers. However, you did not set any null pointers in your code.

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!

this
sname = new char[strlen(n)+1];
should be

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

Have you fixed it?

also check all your deletes... they should match new[]...

like in the Name::Setname
should be delete[] name; not delete

probably - better to add the default constructor that will explicitely set the member pointers to null to avoid possible problems with following deletes...

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:

---

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.

More common self-test would be:

Code:

---

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

---

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

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

Quote:

---

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....

Quote:

---

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;

---

Quote:

---

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?

Quote:

---

(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....

Quote:

---

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.

---

Quote:

---

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.

Quote:

---

Originally Posted by kantze

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....

---

One thing that should be different is that all getters (including GetName) should be const member functions. Put the keyword "const" after the function parameter closing bracket. (Any member function that doesn't modify the object should be declared constant.)

Quote:

---

Do you mean something like this ?

Code:

---

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

---

---

Yes.

Quote:

---

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

---

You leak memory because you allocate new memory for name and sname but what happens to the memory they pointed at before?
If you ask about whether you need this-> then the answer is: no, you don't. In a member function the data members belong to the object this function was called on anyway.

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

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;
}

---

better - return sname and name as const char*
You do not want someone outside your class to otherwrite your data with possible memory overrun problems (even better not to expose this pointers at all - returning std::string object build from your internal data, but as I understand you cannot use std::strings...)