What is the problem with the code?

[dullboy]

Here is my code,

class MyString
{
public:
	MyString(const char* str) 
	{
		m_str = new char[strlen(str)+1];
		strcpy_s(m_str,strlen(str)+1, str);
	}

	MyString()
	{
		m_str = new char[1024];
	}

	~MyString()
	{
		delete[] m_str;
	}

	friend MyString operator+(MyString& a, const MyString& b);
    friend ostream& operator<<(ostream& os, MyString& str);
	
private:
	char* m_str;
};

MyString operator+(MyString& a, const MyString& b)
{
	strcat_s(a.m_str, strlen(a.m_str) + strlen(b.m_str) + 1, b.m_str);
	return a.m_str;
}

ostream& operator<<(ostream& os, MyString& str)
{
	os<<str.m_str;
	return os;
}

int _tmain(int argc, _TCHAR* argv[])
{
	MyString s1("Hi");
	MyString s2; 

	s2 = s1 + " there";

	cout<<s2<<endl;
	return 0;
}

Could you point out the problems? Thanks for your inputs.

[Lindley]

What's the point in writing a string class if it has a fixed maximum size (1024) anyway?

[VladimirF]

Could you point out the problems? Thanks for your inputs.

One obvious problem is that secure version of strcat() requires the available size of destination string to prevent buffer overrun; you are passing the “desired” size, which no one allocates.
In your main function, the constructor:

s1("Hi");

will allocate exactly 3 chars, with no room for concatenation. So this line:

s2 = s1 + " there";

will write outside of allocated memory (undefined behavior; likely – crush).
Also, don’t you need to implement an assignment operator for your string? The one generated by compiler will copy your member variables (char* m_str). When the original string object is deleted, your “copied” one will contain a dead pointer.

[ninja9578]

I'm curious as to why you're writing your own string class anyway. std::string is very stable, and if speed is your concern, there are many replacements out there that you're not going to be able to beat without boatloads of assembly.

[STLDude]

And rule of three is missing.

[Philip Nicoletti]

1) As already pointed out, you need to implement the assignment operator

2) you also need to implement the copy constructor

3) the second argument to operator << should be a const reference

4) As also pointed out, operator + does not check if there is enough room
to do the concatenation. And you do not have a way to determine how
much room there actually is. In your example code, s2 will have 1024,
but s1 will only have 3.

[dullboy]

Thanks for your reponse! You said " When the original string object is deleted, your “copied” one will contain a dead pointer". I wander when that the original string object is deleted happens? It looks like no variables are out of scope. Thanks for your inputs.

One obvious problem is that secure version of strcat() requires the available size of destination string to prevent buffer overrun; you are passing the “desired” size, which no one allocates.
In your main function, the constructor:

s1("Hi");

will allocate exactly 3 chars, with no room for concatenation. So this line:

s2 = s1 + " there";

will write outside of allocated memory (undefined behavior; likely – crush).
Also, don’t you need to implement an assignment operator for your string? The one generated by compiler will copy your member variables (char* m_str). When the original string object is deleted, your “copied” one will contain a dead pointer.

[STLDude]

MyString foo("Some string goes here");
MyString bar;
bar = foo;

Crash...

[dullboy]

Thanks for your valuable suggestions, here is my revised code.

class MyString
{
public:
	MyString& operator=(const MyString& rhs)
	{
		if(this != &rhs)			
			strcpy_s(m_str,strlen(rhs.m_str)+1, rhs.m_str);

		return *this;
	}

	MyString(const MyString& rhs)
	{
		m_str = new char[strlen(rhs.m_str)+1];
		strcpy_s(m_str,strlen(rhs.m_str)+1, rhs.m_str);
	}

	MyString(const char* str) 
	{
		m_str = new char[strlen(str)+1];
		strcpy_s(m_str,strlen(str)+1, str);
	}

	MyString()
	{
		m_str = new char[1024];
	}

	~MyString()
	{
		delete[] m_str;
	}

	friend MyString operator+(MyString& a, const MyString& b);
    friend ostream& operator<<(ostream& os, const MyString& str);
	
private:
	char* m_str;
};

MyString operator+(MyString& a, const MyString& b)
{
	int nLen1 = strlen(a.m_str);
	int nLen2 = strlen(b.m_str);

	char* tem = new char[nLen1+1];
	strcpy_s(tem, nLen1+1, a.m_str);

	delete[] a.m_str;

	a.m_str = new char[nLen1+nLen2+1];
	strcpy_s(a.m_str, strlen(tem)+1, tem);

	strcat_s(a.m_str, nLen1+nLen2+1, b.m_str);

	delete[] tem;
	return a.m_str;
}

ostream& operator<<(ostream& os, const MyString& str)
{
	os<<str.m_str;
	return os;
}

int _tmain(int argc, _TCHAR* argv[])
{
	MyString s1("Hi");
	MyString s2; 
 
	s2 = s1 + " there";

	cout<<s2<<endl;
	return 0;
}

If I comment out my implementation of assignment operator, then program will crash on the line of s2=s1+" there". I understand without assignment operator overloading, default assignment operator only does shallow copy. As the result, two pointers(m_str) will point to same address. But at the moment of calling s2 = s2 + " there", it looks like no pointer is deleted. So why does it crash there?
Also is there any improvement on my code? Thanks for your inputs.

[dullboy]

If I use the class defined below by calling,
MyString s1("Hi");
MyString s2;
s2 = "there " + s1;

Then it doesn't work. How can I make s2="there " + s1 possible? Thanks for your inputs.

class MyString
{
public:
	MyString& operator=(const MyString& rhs)
	{
		if(this != &rhs)			
			strcpy_s(m_str,strlen(rhs.m_str)+1, rhs.m_str);

		return *this;
	}

	MyString(const MyString& rhs)
	{
		m_str = new char[strlen(rhs.m_str)+1];
		strcpy_s(m_str,strlen(rhs.m_str)+1, rhs.m_str);
	}

	MyString(const char* str) 
	{
		m_str = new char[strlen(str)+1];
		strcpy_s(m_str,strlen(str)+1, str);
	}

	MyString()
	{
		m_str = new char[1024];
	}

	~MyString()
	{
		delete[] m_str;
	}

	friend MyString operator+(MyString& a, const MyString& b);
    friend ostream& operator<<(ostream& os, const MyString& str);
	
private:
	char* m_str;
};

MyString operator+(MyString& a, const MyString& b)
{
	int nLen1 = strlen(a.m_str);
	int nLen2 = strlen(b.m_str);

	char* tem = new char[nLen1+1];
	strcpy_s(tem, nLen1+1, a.m_str);

	delete[] a.m_str;

	a.m_str = new char[nLen1+nLen2+1];
	strcpy_s(a.m_str, strlen(tem)+1, tem);

	strcat_s(a.m_str, nLen1+nLen2+1, b.m_str);

	delete[] tem;
	return a.m_str;
}

ostream& operator<<(ostream& os, const MyString& str)
{
	os<<str.m_str;
	return os;
}

[Joeman]

MyString operator+(const char* a, const MyString& b)

[STLDude]

Check this FAQ, it has quite bit of info relevant for you.

[dullboy]

I don't want to change the interface of operator+ because the way operator+ takes char* as an argument violates intuition. I use operator to enhance the intuition. Any other ideas? Thanks for your inputs.

MyString operator+(const char* a, const MyString& b)

[STLDude]

MyString s1("Hi");
MyString s2;
s2 = MyString("there ") + s1;

[laserlight]

I don't want to change the interface of operator+.

Why not? It looks like you are giving operator+ the semantics of operator+=, which is a Bad Thing. Furthermore, your implementation of operator+ does unnecessary work: you should just create a buffer for the result, copy over what is necessary, then destroy a's contents and then make a's pointer point to the start of the new result buffer. (Of course, this would be after you have changed this to operator+=.)

EDIT:

because the way operator+ takes char* as an argument violates intuition. I use operator to enhance the intuition.

Joeman's suggestion will allow expressions such as: "hello" + str. I do not see how that violates intuition. But what I had in mind was changing your current operator+ to take its first argument by const reference. This would also allow expressions such as "hello" + str, except that it will likely be less efficient than providing an overload of operator+ with a const char* first parameter.