string class assignment operator overloading in c++

Question

I defined my own string class, MyString. Everything works well until I assign one object to the other by the overloaded operator=. I know where the problem is but I don't know how to fix it. Any helps?

class MyString{
public:
    MyString( const MyString *strIni );
    MyString( const char *str);
    ~MyString();
    MyString& operator=( const MyString &str );
private:
    char *str;
}


MyString::MyString( const MyString *strIni ){
    this->str = new char[strlen(strIni->str)+1];
    strcpy(this->str,strIni->str) ;
};


MyString::MyString( const char *str){
    this->str = new char[ strlen(str) + 1 ];
    strcpy(this->str , str);
};

MyString::~MyString(){
    delete [] this->str ;
    cout << "successfully deleted..." << endl;
};


MyString& MyString::operator=( const MyString &str ){
    // temp obj holding the rhs
    MyString strTmp(str);
    // temp char pointer holding the rhs
    char *cTmp = strTmp.str;
    // temp obj holding this, later release this memory 
    strTmp.str = this->str ;
    // this holding rhs; assignment done.
    this->str = cTmp;
    return *this ;
};


int main(){
    {                                  // line 1
        MyString mystr1("string #1");  // line 2
        MyString mystr2("string #2");  // line 3
        mystr1 = mystr2;               // line 4
    }                                  // line 5
    return 0;
}

The problem of the code is: at line4, after the assignment the pointer in two objects mystr1 and mystr2 both point the same string "string #2". When the program jump out of the brackets at line 5, the destructors are automatically called by sequence: mystr2 and then mystr1. After mystr2 is destructed, the memory of "string #2" has been released. When the destructor of mystr1 is trying to release non-existing memory, the program crashed.

Anybody can help me to fix the overloading member function. When I assign mystr1 = mystr2, i can create a new string instead of making the two pointers pointing the same string.

Thanks a lot!!

---

---

Updates for further questions...... thank tons!!

actually, i am using copy-and-swap in the overloading function. based on @Mateusz Kołodziejski 's advice, i modified it:

MyString& MyString::operator=( const MyString &rhs ){
    if( this != &rhs ){
        // copy using constructor
        MyString strTmp(rhs) ;
        // swap        
        char *cTmp = strTmp.str;
        // strTmp will be destructed, thus the memory in this will be released
        strTmp.str = this->str ;
        // size of rhs
        const int str_size = strlen(rhs.str);
        this->str = new char[str_size+1];
        copy(rhs.str,rhs.str+str_size,this->str);
    }
    return *this ;
};

when the destructors are called, no crash. But if a printout member function is added, there seems another problem:

void MyString::printout(){
    int str_size = strlen(this->str);
    cout << "string size: " << str_size << endl ;
    for( int i=0;i<str_size;i++ ){
        cout << *(this->str + i);
    }
}

in main function:

int main(){
    {                                  
        MyString mystr1("string #1");  
        MyString mystr2("string #2");  
        mystr1.printout();
        mystr2.printout();
        mystr1 = mystr2;  
        cout << "after assignment: " << endl;
        mystr1.printout();
        mystr2.printout();             
    }                                  
return 0;
}

the results are:

string #1
string #2
after assignment...
string #2═²²²²
string #2

seems that mystr1 is not normal...

anybody can explain this for me?

Thank tons!!

Answer 1

You obviously have to fix your operator=() implementation.

#include <algorithm>

MyString& MyString::operator=( const MyString &rhs ) // (1)
{
    if (this != &rhs) // (2)
    {
        delete[] this->str; // (3)
        this->str = NULL;

        const int str_length = strlen(rhs.str);

        this->str = new char[str_length + 1];
        this->str[str_length] = '\0';
        std::copy(rhs.str, rhs.str + str_length, this->str); // (4)
    }

    return *this;
}

1) Use "rhs" (right-hand-side) instead of "str" for your variable name to avoid ambiguity.

2) Always check if your object is not being assigned to itself.

3) Release the old allocated memory before allocating new.

4) Copy over the contents of rhs to this->str, instead of just redirecting pointers.

EDIT:

Added this->str = NULL; to avoid double deletion on possible exception from new, and later object deconstruction.

This is a naïve implementation - you have to be aware that new can throw an exception. Copy-swap idiom would be better here as suggested by @nyarlathotep and described here: Copy-and-swap.

Answer 2

The problem here is that with the declaration

MyString strTmp(str);

in the assignment operator, you call the default implicitly generated copy-constructor. And that constructor will simply just copy the pointer, not create a new pointer and copy the contents of the string.

The constructor you have taking a MyString pointer is not a copy-constructor, a copy-constructor would take a constant reference instead. If you change your constructor taking a pointer to MyString to take a reference instead, then it will work better.