copy constructor prolbem(??) in a class Name

This is a discussion on copy constructor prolbem(??) in a class Name within the C++ Programming forums, part of the General Programming Boards category; Hi everyone, I'm trying to create a Name class that holds the name and the surname of a person. Name.h ...

  1. #1
    csd@auth
    Join Date
    Oct 2006
    Location
    Greece
    Posts
    71

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

  2. #2
    C++ Witch laserlight's Avatar
    Join Date
    Oct 2003
    Location
    Singapore
    Posts
    20,980
    I suggest solving your problems in one go by using std::string.
    C + C++ Compiler: MinGW port of GCC
    Version Control System: Bazaar

    Look up a C++ Reference and learn How To Ask Questions The Smart Way

  3. #3
    csd@auth
    Join Date
    Oct 2006
    Location
    Greece
    Posts
    71
    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* ....

  4. #4
    C++ Witch laserlight's Avatar
    Join Date
    Oct 2003
    Location
    Singapore
    Posts
    20,980
    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.
    Last edited by laserlight; 07-22-2007 at 10:56 AM.
    C + C++ Compiler: MinGW port of GCC
    Version Control System: Bazaar

    Look up a C++ Reference and learn How To Ask Questions The Smart Way

  5. #5
    csd@auth
    Join Date
    Oct 2006
    Location
    Greece
    Posts
    71
    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?

  6. #6
    C++ Witch laserlight's Avatar
    Join Date
    Oct 2003
    Location
    Singapore
    Posts
    20,980
    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.
    C + C++ Compiler: MinGW port of GCC
    Version Control System: Bazaar

    Look up a C++ Reference and learn How To Ask Questions The Smart Way

  7. #7
    csd@auth
    Join Date
    Oct 2006
    Location
    Greece
    Posts
    71
    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!

  8. #8
    CSharpener vart's Avatar
    Join Date
    Oct 2006
    Location
    Rishon LeZion, Israel
    Posts
    6,424
    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...
    The first 90% of a project takes 90% of the time,
    the last 10% takes the other 90% of the time.

  9. #9
    csd@auth
    Join Date
    Oct 2006
    Location
    Greece
    Posts
    71
    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 ;

  10. #10
    The larch
    Join Date
    May 2006
    Posts
    3,573
    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?)
    Last edited by anon; 07-22-2007 at 02:11 PM.
    I might be wrong.

    Thank you, anon. You sure know how to recognize different types of trees from quite a long way away.
    Quoted more than 1000 times (I hope).

  11. #11
    csd@auth
    Join Date
    Oct 2006
    Location
    Greece
    Posts
    71
    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....

  12. #12
    Registered User
    Join Date
    Feb 2006
    Posts
    312
    Quote Originally Posted by anon View Post
    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 View Post
    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.

  13. #13
    The larch
    Join Date
    May 2006
    Posts
    3,573
    Quote Originally Posted by kantze View Post
    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.)

    Do you mean something like this ?
    Code:
    if(this == &nm) return *this;
    Yes.

    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.
    I might be wrong.

    Thank you, anon. You sure know how to recognize different types of trees from quite a long way away.
    Quoted more than 1000 times (I hope).

  14. #14
    csd@auth
    Join Date
    Oct 2006
    Location
    Greece
    Posts
    71
    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;
    }

  15. #15
    CSharpener vart's Avatar
    Join Date
    Oct 2006
    Location
    Rishon LeZion, Israel
    Posts
    6,424
    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...)
    The first 90% of a project takes 90% of the time,
    the last 10% takes the other 90% of the time.

Page 1 of 2 12 LastLast
Popular pages Recent additions subscribe to a feed

Similar Threads

  1. template and friend class
    By black_spot1984 in forum C++ Programming
    Replies: 3
    Last Post: 10-21-2008, 05:50 PM
  2. Message class ** Need help befor 12am tonight**
    By TransformedBG in forum C++ Programming
    Replies: 1
    Last Post: 11-29-2006, 10:03 PM
  3. Dikumud
    By maxorator in forum C++ Programming
    Replies: 1
    Last Post: 10-01-2005, 06:39 AM
  4. Warnings, warnings, warnings?
    By spentdome in forum C Programming
    Replies: 25
    Last Post: 05-27-2002, 06:49 PM
  5. class member access denied
    By chiqui in forum C++ Programming
    Replies: 2
    Last Post: 05-27-2002, 02:02 PM

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21