Overloading = C++ more structures

Question

I'm doing program for people register and i have problem with overloading operator =.

I have class CRegister, struct Person and Place.

struct Places{
    char date[11];
    char * street;
    char * city;

Places & operator = (const Places & other){
        delete [] street;
        delete [] city;

        strncpy (date, other.date, 11);

        int len;
        len = strlen(other.street);
        this->street = new char[len];
        strncpy ( this->street, other.street, len );

        len = strlen(other.city);
        this->city = new char[len];
        strncpy ( this->city, other.city, len );

        return *this;
    }
}


struct Person{
    char id[12];
    char * name;
    char * surname;
    Places ** oldPlaces;

    int placesCount;
    int placesSize;
};

Person & Person::operator =(const Person& other){
    for (int i = 0; i < this->placesSize; i++){
         delete this->oldPlaces[i];
    }
    delete [] this->oldPlaces;
    delete [] name;
    delete [] surname;


    placesCount = other.placesCount;
    placesSize = other.placesSize;

    oldPlaces = new Places*[other.placesSize];

    strncpy (id, other.id, 11);

        int len;

        len = strlen(other.name);
        this->name = new char[len];
        strncpy ( this->name, other.name, len );

        len = strlen(other.surname);
        this->surname = new char[len];
        strncpy ( this->surname, other.surname, len );

    for (int i = 0; i < placesCount; i++){
        oldPlaces[i] = other.oldPlaces[i];
    }

    return *this;
}



class CRegister     
 {
   private:
    Person **persons;
    int personCount;
    int personSize;
 };

 CRegister& CRegister::operator =(const CRegister& other){
    for (int i = 0; i < this->personSize; i++){
        delete this->persons[i];
    }
    delete [] this->persons;

    personCount = other.personCount;
    personSize = other.personSize;

    persons = new Person*[other.personSize];

    for (int i = 0; i < personCount; i++){
        persons[i] = other.persons[i];
    }


    return *this; 
 }

However, the code has compiled but Netbeans shows me Run failed. Why?

@CapelliC Umm, what?!? Of course `operator=` **can** be overloaded. — Angew is no longer proud of SO, Apr 12 '13 at 15:58
@CapelliC: of course you can overload op=, and in the rule-of-three case you even should. — PlasmaHH, Apr 12 '13 at 15:58
@CapelliC: Yes it can. http://courses.cms.caltech.edu/cs11/material/cpp/donnie/cpp-ops.html — , Apr 12 '13 at 15:58
Replace the arrays and pointer data members by standard library containers and/or strings, and you probably don't even have to implement your own assignment operators and copy constructors. You can then concentrate on more interesting problems. — juanchopanza, Apr 12 '13 at 15:59
I know i can, but for this case, when i have more structures i dont know why exactly :( — Lukas Hamrla, Apr 12 '13 at 16:01
I think you'll *often* find that when you just **make up syntax**, your code won't work. What ever gave you the impression that the code you've posted was supposed to do anything at all? Refer to your lecture notes and your textbook for examples of how you're supposed to write your code. — Rob Kennedy, Apr 12 '13 at 16:01
This is homework for school and we cant use any stl library such as vector, string, etc... — Lukas Hamrla, Apr 12 '13 at 16:01
Never say just "is not working". Tell what you want it to do, and what happens instead! — Zyx 2000, Apr 12 '13 at 16:09
From a comment below *"I have it there, sorry, i havent written it. It's compiled, but it has run failed"* Please **edit your question** to include the real code and the real error you're trying to fix. — Drew Dormann, Apr 12 '13 at 16:13
Ok, unfortunately, i had to reinstall my PC so i have only Windows and Netbeans show me only Run failed... — Lukas Hamrla, Apr 12 '13 at 16:14
@LukasHamrla Your new code doesn't compile either. Please copy and paste the **actual code** so we're not simply fixing your typos. — Drew Dormann, Apr 12 '13 at 16:18

score 2 · Answer 1 · answered Apr 12 '13 at 15:59

2

    len = strlen(other.city);
    this->city = new char[len];
    strncpy ( this->city, other.city, len );

This does not preserve the length of the string. How will later code know how long this->city is?

answered Apr 12 '13 at 15:59

David Schwartz

179,497
17
214
278

So i have to write len+1, the last position for '\0'? – Lukas Hamrla Apr 12 '13 at 16:09
That would be one way. Using `std::string` would make more sense though. – David Schwartz Apr 12 '13 at 16:09
It is homework and we cant use string unfortunately... – Lukas Hamrla Apr 12 '13 at 16:13
Can you make your own string class? (I hope so, because forcing people to do things in very bad ways teaches them bad habits.) – David Schwartz Apr 12 '13 at 16:14
1

This suffers from the same problem his original code does. `strlen` doesn't count the final `'\0'`, but you _must_ copy it to the destination string. – James Kanze Apr 12 '13 at 16:36
@JamesKanze: What is the "this" you are referring to? – David Schwartz Apr 12 '13 at 16:39
The code you show. You need to allocate `strlen( source ) + ` bytes, in order to have space for the trailing `'\0'`. (Also, you generally _don't_ want to use `strncpy`, because it doesn't guarantee a trailing `'\0'`. And in this case, won't put one in the target string.) – James Kanze Apr 12 '13 at 16:41
@JamesKanze: The code I show is his original code. As I pointed out, it doesn't preserve the length of the string. – David Schwartz Apr 12 '13 at 17:23

score 1 · Answer 2 · edited May 23 '17 at 11:57

1

You are not following the Rule of Three. It could possibly be one reason if not the only reason.

On another note,

Why not simply use std::string?
Why not use copy and swap idiom ?

edited May 23 '17 at 11:57

Community

1
1

answered Apr 12 '13 at 15:59

Alok Save

202,538
53
430
533

score 1 · Answer 3 · answered Apr 12 '13 at 16:35

Since you're basically using C (wrapped in a class): you've made what is probably the most frequent error in C code. strlen returns the number of characters in the string, not counting the final '\0'. strcpy copies the final '\0', so one more character, If you use strncpy (as you do, which you shouldn't), then you will end up with a string which is not '\0' terminated. So with strcpy, you overrun the end of the buffer, and with strncpy, anyone trying to read the string will overrun the end of the buffer (since they will continue until a '\0').

The rule is to allocate one more character than strlen returns:

size_t len = strlen( other.name ) + 1;
name = new char[ len ];
strcpy( name, other.name );

(Although not standard C, many systems have a strdup function which does this. Using malloc, of course, so you would have to free with free, and not delete.)

Beyond that, I will repeat what I said before: your code will leave the object in an inconsistent state if any of the allocations failed. Always do everything which can fail before modifying anything in your class. The swap idiom is classical for operator=, but otherwise, you can use local pointers:

char* newName = NULL;
char* newSurname = NULL;
Places* newPlaces = NULL;

try {
    newName = strdup( other.name);
    newSurname = strdup( other.surname );
    newPlaces = deapCopyPlaces( other.places );
} catch ( ... ) {
    deepDelete( newPlaces );
    delete [] newName;
    delete [] newSurname;
}
//  And only now...
deepDelete( places );
delete [] name;
delete [] surname;
name = newName;
surname = newSurname;
places = newPlaces;

But the swap idiom is far preferrable, because it avoids having to duplicate the all of the error handling code.

Overloading = C++ more structures

3 Answers3