String problem

[Sanix]

I had to program a string class as exercise. I should provide a method concat.

Whenever I use it the program stops working without showing some error or something else. It just quits and does not process the following lines after I use the concat method.
I played around and figured out that when I change the signature to
MyString& concat(MyString& my);

it works but by copy it does not with the following:
MyString& concat(MyString my);

I don't understand that.

My header file:
#ifndef MYSTRING_H_
#define MYSTRING_H_

class MyString {
private:
	char* characters;
	int size;

	int length(char* c);
public:
	MyString();
	MyString(char* c);
	MyString(const MyString& my);

	virtual ~MyString();

	char* toCString();

	MyString& concat(MyString my);

	int getLength();
};

#endif /* MYSTRING_H_ */


MyString class:

#include <iostream>
#include "MyString.h"

MyString::MyString() {
	//Init char with size 0
	characters = new char[1];
	size = 0;
	characters[0] = '\0';
}

MyString::MyString(const MyString& my) {
	characters = my.characters;
	size = my.size;
}

int MyString::getLength() {
	return size;
}

MyString::MyString(char* c) {
	characters = c;
	size = length(c);
}

char* MyString::toCString() {
	return characters;
}

int MyString::length(char* c) {
	int i = 0;
	while(c[i] != '\0') {
		i++;
	}

	return i++;
}


MyString& MyString::concat(MyString my) {
	char* c = my.toCString();

	int l1 = getLength();
	int l2 = length(c);

	size = l1 + l2;

	char* temp = new char[size+1];	//+1 for \0



	for(int i = 0; i < l1; i++) {
		temp[i] = characters[i];
	}
	for(int i = 0; i < l2; i++) {
		temp[l1+i] = c[i];
	}
	temp[size] = '\0';	//Termination


	//Copy string
	characters = temp;


	return *this;
}



MyString::~MyString() {
	//free memory
	delete[](characters);
}

[Lindley]

Your copy constructor is incorrect. Right now, once you use it you'll have two MyString objects pointing at the same physical character array in memory. One one them is destroyed, that array gets deleted......leaving the second one pointing to invalid memory.

Also, you need to define operator=.

[cilu]

Here are some things: why do you have a length() function that takes a char* and returns its length? Shouldn't it refer to the current object? So is shouldn't have any parameters. Moreover, why don't you use strlen() to return the length? And even more, you already know the length from your variable size. You only have to return that value.

Then, in concat() you keep allocing new memory, but you never release the old memory. You should do that just before this assignment:

characters = temp;

Now, the real problem is this: whenever you handle memory allocation manually in a class (i.e. you have pointers) you must implement a copy constructor and an assignment operator and do a deep copy, as oppose to the implicit shallow copy that only copies the values of the variables and not the object they point to. You have implemented a copy constructor:

MyString::MyString(const MyString& my) {
	characters = my.characters;
	size = my.size;
}

But all you do there is a shallow copy: you copy the value of the pointer (i.e. the address it points to), not the object that it points to. When you destroy the string, the first string will release the memory and the second will crash when attempting to destroy something that was already destroyed.

Here is how a deep copy constructor should look:

MyString::MyString(const MyString& my) {
  size = my.size; 
  characters = new char[size+1];
  strcpy(characters, my.characters);
}

I suggest you reimplement this constructor MyString(char* c) in the same manner, and an assignment operator. Then you can fix the concat function too.

[Lindley]

There's a destructor there.

[cilu]

There's a destructor there.

Oops, the scroll didn't work well. . I have adjusted my post.

[Sanix]

@Lindley
Thanks
@cilu
Thanks a lot for your explanations! I made the changes and it works now. I was not supposed to use existing string functions, that's why I didn't use strlen and so on.
The getLength method refers to the current object. The private one is needed in the concat method, since I need it for the MyString object from the parameter and cannot use strlen.