C++ - implementation of string class

Question

I'm trying to implement string class. Here is what I have done:

#include <iostream>
#include <cstring>

using namespace std;

class MyString{
    private:
    char * content;
    int length;
    public:
    MyString ();
    MyString ( const char * );
    ~MyString ();
    MyString ( const MyString & );
    void print ( void );
    void operator = ( const MyString );
};

MyString :: MyString () {
    content = 0;
    length = 0;    
}

MyString :: MyString(const char *n) {
    length = strlen (n);
    content = new char [ length ];
    for ( int i = 0 ; i < length ; i++ ){
    content [i] = n [i];
    }
    content [length] = '\0';
    }


MyString :: ~ MyString () {
    delete [] content;
    content = 0;
}


MyString :: MyString ( const MyString & x ) {
    length = x.length;
    content = new char [length];
    for( int i = 0 ; i < length ; i++ ){
    content [i] = x.content [i];
    }
    content [length] = '\0';
}

void MyString :: print( void ) {
    cout <<""<< content << endl;
}

void MyString :: operator = ( const MyString x ) {
    length = x.length;
    content = new char [length];
    for( int i = 0 ; i < length ; i++ ){
    content [i] = x.content [i];
    }
    content [length] = '\0';
}

int main() {
    MyString word1 ("stackoverflow");
    MyString word2;
    word2 = word1;
    word1.print();
    word2.print();
}

I compiled it and this is what I get:

stackoverflow

stackoverflow

Process returned 0 (0x0) execution time : 0.050 s Press any key to continue.

Although it looks correct according to result above, I wonder is it really correct? I'm not so familiar with C-style strings so I'm concerned for example about line:

content [length] = '\0';

Since C-style strings has null terminator at end, I wanted to terminate my array but is this correct way to do it? I used dynamic memory allocation and I also wonder did I free resources properly? Are there some memory leaks? Thanks in advance.

EDIT1: I also overloaded opeartor + (I want to join "MyStrings"), here is code:

MyString MyString :: operator + ( const MyString & x ){
    MyString temp;
    temp.length = x.length + length;
    temp.content = new char [ temp.length +  1 ];
    int i = 0, j = 0;
    while ( i < temp.length ) {
    if (i < length ) {
    temp.content [i] = content [i];
    }
    else {
    temp.content [i] = x.content [j];
    j ++;
    }
    i ++;
    }
    temp.content [ temp.length ] = '\0';
    return temp;
    }

Here is main program:

int main()
   {
   MyString word1 ( "stack" );
   MyString word2 ( "overflow" );
   MyString word3 = word1 + word2;
   word3.print();
   word3 = word2 + word1;
   word3.print();
   }

And here is result:

stackoverflow

overflowstack

Process returned 0 (0x0) execution time : 0.040 s Press any key to continue.

I hope there are no problems with this code :)

EDIT2: Here is implementation of + operator using for loops, instead of while:

MyString MyString :: operator + (const MyString & x){
    MyString temp;
    temp.length = x.length + length;
    temp.content = new char [temp.length+1];
    for( int i = 0 ; i < length ; i++ ){
    temp.content[i] = content[i];
    }
    for( int i = length , j = 0 ; i <temp.length ; i++, j++){
    temp.content[i] = x.content[j];
    }
    content[temp.length] = '\0';
    return temp;
}

It's maybe better now because there is no if :)

Answer 1

You are trying to assign content[length] a value, but you haven't allocated enough memory for content[length] to be accessed. If length == 10, then you can access content[0] thru content[9], but not content[10].

This can be fixed of course by removing the line content[length] = \0 from both constructors, or if you want to append \0 you should increase the value of length by 1.

Have you considered just using std::string internally?

Edit: @Thane Plummer was first to point this out in the comments!

Answer 2

A few other notes and suggestions because there are are least two more gotchas waiting to leap out and strike.

#include <iostream>
#include <cstring>

// using namespace std; DANGER! namespace std is huge. Including all of it can
// have tragic, unforeseen consequences. Just use what you need.
using std::cout;
using std::endl;


class MyString
{
private:
    char * content;
    int length;
// will use clone to reduce duplication in the copy constructor and operator =
    void copy(const MyString & source);
public:
    MyString();
// it is nice to name the variables in the definition. The header may be the
// only documentation the user gets.
    MyString(const char * source);
    ~MyString();
    MyString(const MyString &source);
    void print(void);
// changed prototype to match the expected format operator= format
    MyString & operator =(const MyString &source);
//OP asked about this in a previous question.
    friend std::ostream & operator<<(std::ostream & out,
                                     const MyString& towrite);
};

MyString::MyString()
{
//    content = 0; 
//destructor needs something to delete[]. If content isn't set to something,
//you'll get a big ka-BOOM! when the MyString is destroyed

    content = new char[1];
    content[0] = '\0'; //this has the advantage of printing an empty MyString 
                       // without crashing
    length = 0;
}


MyString::MyString(const char *source) // Variable names should describe their purpose
{
    //DANGER: strlen will fail horribly if passed an unterminated string. At a
    // loss at the moment for a good, safe solution. Look into strnlen, but
    // even it can't help you here.
    length = strlen(source);
    content = new char[length + 1]; //Needed one extra character to fit the NULL
/* If we got this far without dying, strcpy is no threat which makes this redundant:
    for (int i = 0; i < length; i++)
    {
        content[i] = n[i];
    }
    content[length] = '\0';
*/
    strcpy(content, source);
}

MyString::~MyString()
{
    delete[] content;
//    content = 0; string is gone. No need to clear this
}

void MyString::copy(const MyString & source)
{
    length = source.length;
    content = new char[length + 1];
// assuming that the source MyString is correctly formatted this is once again safe.
    strcpy(content, source.content);
}

MyString::MyString(const MyString & source)
{
    copy(source); // use the copy method
}

void MyString::print(void)
{
    cout << "" << content << endl;
}

MyString &MyString::operator =(const MyString &source)
{
    copy(source); // use the copy method again.
    return *this; // allows chaining operations
}

std::ostream & operator<<(std::ostream & out,
                          const MyString& towrite)
{
    out << towrite.content;
    return out;
}

int main()
{
    MyString word0;
    MyString word1("stackoverflow");
    MyString word2;
    word2 = word1;
    MyString word3(word2); //testing copy constructor
    word1.print();
    word2.print();
    cout << word3 << endl; //testing outstream overload
    // test output of empty string
    word0.print(); 
    cout << word0 << endl; 
}

Edit:

Realized after posting that since we know the lengths of the strings, there are significant performance gains from using memcpy(content, source.content, length+1); in place of strcpy.

Answer 3

There are two errors. One has already been stated by Thane Plummer in the comments and by Tas in the answers:

MyString :: MyString(const char *n) {
  length = strlen(n);
  content = new char [length];
  for( int i = 0 ; i < length ; i++ ){
    content [i] = x.content [i];
  }
  content [length] = '\0';
}

if your string is the null terminated "abc\0", strlen will return 3 and not 4, so you'll only allocate 3 chars instead of 4 (edit: and to be complete, as previously stated, you indeed start to index from 0 and not 1, so content[length] will always overflow, even if you increase length)

The other error is less grave (and is actually legal but odd c++):

void operator = ( const MyString );

The copy assignment operator should take a const reference rather than a const value (otherwise you may uselessly call the copy constructor), and return a reference rather than void (so that you can chain some calls). The correct declaration is:

MyString& operator=(const MyString&);

The correct implementation is:

MyString& MyString::operator=(const MyString& x) {
  length = x.length;
  delete[] content;
  content = new char [length];
  for( int i = 0 ; i < length ; i++ ){
    content [i] = x.content [i];
  }
  // actually not needed since x.content should already be null-terminated
  // content[length - 1] = '\0';
  return *this;
}