Why is there an error in my String class append() method?

Question

I am trying to implement a String class, and I found an error with my append() method. Could you please tell me what I did wrong with it? I am not permitted to use any standard library functionality. I just added <iostream> to print output so I can check the answer.

Here is append():

void append(const String& s){
    char* temp;
    temp = new char[len+s.len];
    for(int i = 0; i < len; i++){
        temp[i] = str[i];
    }
    for(int i = 0; i < s.len; i++){
        temp[len + 1] = s.str[i];
    }
    str = temp;
}

Here is my full code:

#include <iostream>

using namespace std;

class String{
    public:
        char* str = nullptr;
        unsigned int len = 0;

        String() = default;

        String(const char* chars){
            if(chars){
                unsigned int i = 0;
                while(chars[i]){
                    i++;
                }
                len = i;
                str = new char[len];
                for(int j = 0; j < len; j++){
                    str[j] = chars[j];
                }
            }
        };

        String(const String& s){
            if(!s.isEmpty()){
                len = s.len;
                str = new char[len];
                for(int i = 0; i < len; i++){
                    str[i] = s.str[i];
                }
            }
        };

        ~String() noexcept{
            delete[] str;
        };

        String& operator=(const String &s) {
            if (&s != this) {
                String tmp(s);
                char *tmpstr = tmp.str;
                unsigned int tmplen = tmp.len;
                tmp.str = str;
                tmp.len = len;
                str = tmpstr;
                len = tmplen;
            }
            return *this;
        }

        void append(const String& s){
            char* temp;
            temp = new char[len+s.len];
            for(int i = 0; i < len; i++){
                temp[i] = str[i];
            }
            for(int i = 0; i < s.len; i++){
                temp[len + 1] = s.str[i];
            }
            str = temp;
        }

        bool isEmpty() const noexcept{
            return(len == 0);
        }

        unsigned int length() const noexcept{
            return len;
        }

        bool contains(const String& substring) const noexcept{
            if(find(substring)){
                return true;
            }
            return false;
        }

        int find(const String& substring) const noexcept{
            for(int i = 0; i < len - substring.len + 1; i++){
                if(str[i] == substring.str[0]){
                    for(int j = 1; j < substring.len;){
                        if(str[i + j] == substring.str[j]){
                            j++;
                            if(j == substring.len){
                                return i;
                            }
                        }
                        else{
                            break;
                        }
                    }
                }
            }
            return -1;
        }

        const char* toChars() const noexcept{
            char* temp = new char[len + 1];
            for(unsigned int c = 0; c < len; ++c) {
                temp[c] = str[c];
            }
            temp[len] = '\0';
            return temp;
        }
};

int main()
{
    const char* chars = "Boo is snoring1";
    const char* morechars = " and running";

    String s(chars);
    String more(morechars);
    s.append(more);

    cout << s.str << endl;

    return 0;
}

Answer 1

Your second loop is not accessing temp correctly. You are using temp[len + 1], but you need to use temp[len + i] instead. This is why you should use more meaningful names for your variables. 1 and i are not interchangable, but they are very close visually that they are easy to get confused with each other.

Also, you are not freeing the previous str before replacing it with the new data. And you are not updating len after replacing str.

Try this instead:

void append(const String& s){
    char* temp = new char[len + s.len];
    for(int idx = 0; idx < len; ++idx){
        temp[idx] = str[idx];
    }
    for(int idx = 0; idx < s.len; ++idx){
        temp[len + idx] = s.str[idx];
    }
    delete[] str;
    str = temp;
    len += s.len;
}

Outside of that, you have changed your main() since your last question to print out str directly, but str is not null-terminated. So you would need to print it out like this instead:

int main()
{
    String s("Boo is snoring1");
    s.append(" and running");

    cout.write(s.str, s.len);
    cout << endl;

    return 0;
}

Otherwise, go back to printing out the result of toChars() like I showed you earlier:

const char *chars = s.toChars();
cout << chars << endl; 
delete[] chars;

Or else, you should define your own operator<< to print out a String object:

std::ostream& operator<<(std::ostream &os, const String &s)
{
    return os.write(s.str, s.len);
}

...

cout << s << endl; 

Answer 2

Your second loop should temp[len + i] not temp[len + 1]

Answer 3

For starters the function append has a memory leak because the previous pointer is not deleted. And the new value of the data member len is not set.

void append(const String& s){
            char* temp;
            temp = new char[len+s.len];
            for(int i = 0; i < len; i++){
                temp[i] = str[i];
            }
            for(int i = 0; i < s.len; i++){
                temp[len + 1] = s.str[i];
            }
            str = temp;
        }

In the second loop

        for(int i = 0; i < s.len; i++){
            temp[len + 1] = s.str[i];
        }

there is a typo. You mean

temp[len + i] = s.str[i];
          ^^^

Thirdly, you are outputting the string like a C string

cout << s.str << endl

It means that the pointed string shall have the terminating zero character.

The data members str and len shall be private data members. And the data member len shall have the type size_t.

    char* str = nullptr;
    unsigned int len = 0;

Moreover in loops you are using an index of the type int instead of the type unsigned int.

So in fact the class definition in whole is wrong.

As for the function append then it should be defined at least the following way. I assume that the pointed arrays contain strings.

String & append( const String &s )
{
    char* temp = new char[len + s.len + 1];

    size_t i = 0;

    for ( ; i < len; i++ ) temp[i] = str[i];
    while ( ( temp[i] = s.str[i-len] ) != '\0' ) i++;

    delete [] str;

    str = temp;
    len = len + s.len;

    return *this; 
}

Pay attention to that the default constructor shall allocate a memory of the size 1 and set the allocated byte to '\0'. In this case the data member len shall be set to 0.

So check all other member functions. For example the member function find will have undefined behavior when the passed string has the data member len equal to 0.