Basic Custom String Class for C++

Question

EDIT: I don't want to delete the post because I have learned a lot very quickly from it and it might do someone else good, but there is no need for anyone else to spend time answering or viewing this question. The problems were in my programming fundamentals, and that is something that just can't be fixed in a quick response. To all who posted, thanks for the help, quite humbling!

Hey all, I'm working on building my own string class with very basic functionality. I am having difficulty understand what is going on with the basic class that I have define, and believe there is some sort of error dealing with the scope occurring. When I try to view the objects I created, all the fields are described as (obviously bad pointer). Also, if I make the data fields public or build an accessor method, the program crashes. For some reason the pointer for the object is 0xccccccccc which points to no where.
How can a I fix this? Any help/comments are much appreciated.

    //This is a custom string class, so far the only functions are 
    //constructing and appending

#include<iostream>
using namespace std;

class MyString1
{
public:
    MyString1()     
    {   

        //no arg constructor
        char *string;
        string = new char[0];
        string[0] ='\0';
        std::cout << string;
        size = 1;
    }
    //constructor receives pointer to character array
    MyString1(char* chars)
    {
        int index = 0;
        //Determine the length of the array
        while (chars[index] != NULL)
            index++;
        //Allocate dynamic memory on the heap
        char *string;
        string = new char[index+1];
        //Copy the contents of the array pointed by chars into string, the char array of the object
        for (int ii = 0; ii < index; ii++)
            string[ii] = chars[ii];
        string[index+1] = '\0';
        size = index+1;
    }
    MyString1 append(MyString1 s)
    { 
        //determine new size of the appended array and allocate memory
        int newsize = s.size + size;
        MyString1 MyString2;
        char *newstring;
        newstring = new char[newsize+1];

        int index = 0;
        //load the first string into the array
        for (int ii = 0; ii < size; ii++)
        {
            newstring[ii] = string[ii];
            index++;
        }

        for(int jj = 0; jj < s.size; jj++, ii++)
        {
            newstring[ii] = s.string[jj++];
            index++;
        }
        //null terminate
        newstring[newsize+1] = '\0';

        delete string;
        //generate the object for return
        MyString2.string=newstring;
        MyString2.size=newsize;

        return MyString2;
    }
private:
    char *string;
    int size;
};



int main()
{
    MyString1 string1;
    MyString1 string2("Hello There");
    MyString1 string3("Buddy");
    string2.append(string3);
    return 0;
}

EDIT: Thank you everyone so far who has responded and dealing with my massive lack of understanding of this topic. I'll begin to work with all of the answers, but thanks again for the good responses, sorry my question is vague, but there isn't really a specific error, but more of a lack of understanding of arrays and classes.

Answer 1

index doesn't start at 0 in the second while loop in your append function. You should probably be using for loops. Oh, and you're using the wrong form of delete. You should be using delete[] string because string is an array.

There are numerous other style issues, and outright errors, but the thing I mentioned first is the basic error you were encountering.

I would write the append function in this way:

void append(MyString1 s)
{ 
           //determine new size of the appended array and allocate memory
    int newsize = s.size + size;
    char *newstring = new char[newsize+1];

    int destindex = 0;
    for (int index = 0; index < size; ++index) {
       newstring[destindex++] = string[index];
    }
    for (int index = 0; index < s.size; ++index) {
       newstring[destindex++] = s.string[index];
    }
    newstring[destindex] = '\0';

    delete[] string;
    string = newstring;
}

Answer 2

Here's just the mistakes from the first constructor.

    MyString1()     
    {   

//no arg constructor
        char *string;         //defines local variable that hides the member by that name
        string = new char[0]; //sort of meaningless
        string[0] ='\0';      //not enough room for that (out-of-bounds)
        std::cout << string;
        size = 1;             //I don't think you should count null as part of the string
    }

Similar mistakes elsewhere.

Also you should pass parameters in a more careful way.

MyString1(const char* source); //note const

MyString1 append(const MyString1& what); //note const and reference

If the latter is correct, also depends on what it is supposed to do. Based on std::string the expected result would be:

MyString1 a("Hello "), b("world");
a.append(b);
assert(a == "Hello world");

Answer 3

Some comments on your code:

    MyString1()     
    {   

//no arg constructor

Perhaps your instruction requires it, but in general this is the kind of comment that's worse than useless. Comments should tell the reader things that aren't obvious from the first glance at the code.

        char *string;
        string = new char[0];
        string[0] ='\0';

This invokes undefined behavior. Calling new with zero elements is allowed, but you can't dereference what it returns (it may return a null pointer, or it may return a non-null pointer that doesn't refer to any storage). In most cases, you're better off just setting the pointer to NULL.

        std::cout << string;

What's the point of writing out an empty string?

        size = 1;

The string is empty so by normal figuring, the size is zero.

//constructor receives pointer to character array

Still useless.

    MyString1(char* chars)

Since you aren't (or shouldn't be anyway) planning to modify the input data, this parameter should be char const *.

    {
        int index = 0;
//Determine the length of the array
        while (chars[index] != NULL)
            index++;

While this works, "NULL" should really be reserved for use as a pointer, at least IMO. I'd write it something like:

while (chars[index] != '\0')
    ++index;

Unless you're using the previous value, prefer pre-increment to post-increment.

//Allocate dynamic memory on the heap

As opposed to allocating static memory on the heap?

    MyString1 MyString2;

Using the same naming convention for types and variables is confusing.

    while (string[index] != NULL)

Same comment about NULL as previously applies here.

MyString1 append(MyString1 s)

IMO, the whole idea of this function is just plain wrong -- if you have a string, and ask this to append something to your string, it destroys your original string, and (worse) leaves it in an unusable state -- when you get around to adding a destructor that frees the memory owned by the string, it's going to cause double-deletion of the storage of a string that was the subject (victim?) of having append called on it.

I'd consider writing a private "copy" function, and using that in the implementations of some (most?) of what you've shown here.

As a bit of more general advice, I'd consider a couple more possibilities: first of all, instead of always allocating exactly the amount of space necessary for a string, I'd consider rounding the allocation to (say) a power of two. Second, if you want your string class to work well, you might consider implementing the "short string optimization". This consists of allocating space for a short string (e.g. 20 characters) in the body of the string object itself. Since many strings tend to be relatively short, this can improve speed (and reduce heap fragmentation and such) considerably by avoiding doing a heap allocation if the string is short.