String class implementation

Question

I'm working on a project (it's a language), and for that project, I decided to go with a low-level C++ style, so that means making my own data types. I recently got done making my own string class implementation. This is actually the first time I've made a string class before, so I may be doing a lot of things wrong. As far as I know, I tested it, and it works and does what it's intended to so, but I may be overlooking something or something may not be the best practice.

My goal here was to make a "low-level C++" string class, meaning I'd create everything myself without using any headers.

I have everything in one .h file, and I know that not really the best way to do it, but I'm not really a big fan of splitting up my code when it's only a small class.

Here's an overview of the methods and what they do, and why I chose them (note that this is not the actual .h file, I'm just showing this to provide some context and an overview of what I'm doing):

class string
{
public:
    string();
    string(const char* buffer);
    string(const string& buffer);

    ~string();

public:
    string& operator=(const char* buffer);
    string& operator=(const string& buffer);

    void operator+=(char buffer);
    void operator+=(const char* buffer);
    void operator+=(const string& buffer);

    bool operator==(const char* buffer) const;
    bool operator==(const string& buffer) const;
    bool operator!=(const char* buffer) const;
    bool operator!=(const string& buffer) const;

    char operator[](int index) const;
    char& operator[](int index);

public:
    int length() const;

    // returns the actual string
    const char* get() const;

private:
    int str_len(const char* buffer) const;

    // given a block of memory 'dest', fill that with characters from 'buffer'
    void str_cpy(char* dest, int dest_size, const char* buffer);
    void str_cpy(char* dest, int dest_size, const string& buffer);

    // allocate a given size of memory
    char* str_alc(int size);

private:
    int size;
    char* str;
};

So as you can see, it's not really anything special, just some basic functions that should be enough for my project. A few comments on the code:

I chose to add a get() method instead of something like operator const char*() since I feel like the operator overloading would be enough, and I want to make accessing the actual string more explicit.

Also a note on the private methods, those are basically very similar to the methods that can be found in the <string.h> header, like strncpy() and str_len().

Here's the actual string.h file:

#pragma once

namespace night { // 'night' is the project I'm working on

class string
{
public:
    string()
    {
        size = 0;
        str = str_alc(1);
    }

    string(const char* buffer)
    {
        size = str_len(buffer);
        str = str_alc(size + 1);

        str_cpy(str, size + 1, buffer);
    }

    string(const string& buffer)
    {
        size = buffer.size;
        str = str_alc(size + 1);

        str_cpy(str, size + 1, buffer);
    }

    ~string()
    {
        delete[] str;
    }

public:
    string& operator=(const char* buffer)
    {
        delete[] str;

        size = str_len(buffer);
        str = str_alc(size + 1);

        str_cpy(str, size + 1, buffer);

        return *this;
    }

    string& operator=(const string& buffer)
    {
        delete[] str;

        size = buffer.size;
        str = str_alc(size + 1);

        str_cpy(str, size + 1, buffer);

        return *this;
    }

    void operator+=(char buffer)
    {
        char* temp = str_alc(size + 2);
        str_cpy(temp, size + 2, str);

        temp[size] = buffer;
        temp[size + 1] = '\0';

        delete[] str;

        size += 1;
        str = temp;
    }

    void operator+=(const char* buffer)
    {
        size += str_len(buffer);

        char* temp = str_alc(size + 1);

        str_cpy(temp, size + 1, str);
        str_cpy(temp, size + 1, buffer);

        delete[] str;
        str = temp;
    }

    void operator+=(const string& buffer)
    {
        size += buffer.size;

        char* temp = str_alc(size + 1);

        str_cpy(temp, size + 1, str);
        str_cpy(temp, size + 1, buffer);

        delete[] str;
        str = temp;
    }

    bool operator==(const char* buffer) const
    {
        if (size != str_len(buffer))
            return false;

        for (int a = 0; a < size; ++a)
        {
            if (str[a] != buffer[a])
                return false;
        }

        return true;
    }

    bool operator==(const string& buffer) const
    {
        return operator==(buffer.str);
    }

    bool operator!=(const char* buffer) const
    {
        return !operator==(buffer);
    }

    bool operator!=(const string& buffer) const
    {
        return !operator==(buffer.str);
    }

    char operator[](int index) const
    {
        if (index < 0 || index >= size)
            throw "[error] index is out of range";

        return str[index];
    }

    char& operator[](int index)
    {
        if (index < 0 || index >= size)
            throw "[error] index is out of range";

        return str[index];
    }

public:
    int length() const
    {
        return size;
    }

    const char* get() const
    {
        return str;
    }

private:
    int str_len(const char* buffer) const
    {
        int length = 0;
        for (int a = 0; buffer[a] != '\0'; ++a)
            length += 1;

        return length;
    }

    void str_cpy(char* dest, int dest_size, const char* buffer)
    {
        int start = 0;
        while (dest[start] != '\0')
            start += 1;

        if (dest_size - start < str_len(buffer))
            throw "[fatal error] function 'void str_cpy(char* dest, const char* buffer)' does not have enough space";

        for (int a = 0; a < str_len(buffer); ++a)
            dest[start + a] = buffer[a];

        dest[start + str_len(buffer)] = '\0';
    }

    void str_cpy(char* dest, int dest_size, const string& buffer)
    {
        int start = 0;
        while (dest[start] != '\0')
            start += 1;

        if (dest_size - start < buffer.size)
            throw "[fatal error] function 'void str_cpy(char* dest, const string& buffer)' does not have enough space";

        for (int a = 0; a < buffer.size; ++a)
            dest[start + a] = buffer.str[a];

        dest[start + buffer.size] = '\0';
    }

    char* str_alc(int size)
    {
        char* buffer;

        try {
            // set the new string to contain null-terminators by default
            buffer = new char[size]{ '\0' };
        }
        catch (...) {
            throw "[fatal error] function 'char* str_alc(int size)' cannot allocate enough memory";
        }

        return buffer;
    }

private:
    int size;
    char* str;
};

} // namespace night

And just as an example, here's how you would use it:

int main()
{
    night::string test = "class";

    test += ' ';
    test += "string";

    std::cout << test.get() << '\n';

    night::string test1 = "string class";

    test = test1;
    test[0] = 'S';
    test[7] = 'C';

    std::cout << test.get() << '\n';

    night::string test2 = "String Class";

    std::cout << (test == test2) << '\n';
    std::cout << (test != test2) << '\n';
}

Here's my primary area of concern:

1. Do I need a move constructor and move assignment operator? I know those aren't necessary, but would they make a big difference in this case?

2. Are the private methods efficient? Could they be improved?

3. Is the method str_alc() good? Like is it good practice to wrap new in a try-catch statement? And should I fill the string with \0s by default? Or is that causing more harm than good?

Also a minor question I have is if the parameter name buffer is the right choice? I'm not really sure what to call the parameters...

Any other feedback is also highly appreciated!

Thanks!

Answer 1

Why do you convert an exception that has meaning into a meaningless string?

char* str_alc(int size)
{
    char* buffer;

    try {
        // set the new string to contain null-terminators by default
        buffer = new char[size]{ '\0' };
    }
    catch (...) {
        throw "[fatal error] function 'char* str_alc(int size)' cannot allocate enough memory";
    }

    return buffer;
}

How the error is reported will ultimately depend on where it is caught. You should not simply re-throw a string. Catch the exception at the point you are reporting errors and convert to an appropriate error message at that point. Or throw a more meaningful exception type (not a string).

Also if you are going to make this check then simply use the non-throwing version of new and then validate the buffer is not null and throw your new exception.

---

Don't reinvent exiting functions:

int str_len(const char* buffer) const

There is already a C-function for this and I guarantee that it is NOT slower than your version and more than likely an order of magnitude faster.

void str_cpy(char* dest, int dest_size, const char* buffer)

Again there are already C-String copying functions. If you are going to re-invent them use the C++ algorithms to copy the bytes around rather than manually writing loops.

---

If you are comparing two string objects. You devolve into comparing a string object to a C-String as the most general case.

bool operator==(const char* buffer) const
{
    if (size != str_len(buffer))
        return false;

    for (int a = 0; a < size; ++a)
    {
        if (str[a] != buffer[a])
            return false;
    }

    return true;
}

bool operator==(const string& buffer) const
{
    return operator==(buffer.str);
}

bool operator!=(const char* buffer) const
{
    return !operator==(buffer);
}

bool operator!=(const string& buffer) const
{
    return !operator==(buffer.str);
}

As a result you are computing the string length for an object that you already know the string length for!

---

You have implemented a checked operator[]:

char operator[](int index) const
{
    if (index < 0 || index >= size)
        throw "[error] index is out of range";

    return str[index];
}

char& operator[](int index)
{
    if (index < 0 || index >= size)
        throw "[error] index is out of range";

    return str[index];
}

In C++ the operator[] is usually unchecked and used in situations where you have already established that the access is within bounds and thus the check is redundant.

In C++ we normally also provide an unchecked version so you don't have to do a manual check. In C++ we call this version at().

for(int loop = 0; loop < str. length(); ++loop) {
    std::cout << str[loop];  // Why do I need the index
                             // checked here (every loop)
                             // I have already established that
                             // loop is within bounds by checking
                             // it against the length of the string.
} 

---

You have not implemented move semantics.

---

You have not implemented a reserve size. There is a difference between current length and maximum length before a resize is required.

---

Your assignment operator is not exception safe.

string& operator=(const char* buffer)
{
    delete[] str;                   // you have modified the object here

    size = str_len(buffer);
    str = str_alc(size + 1);        // This can throw. If it does
                                    // your object is in a bad state
                                    // the member str is pointing at
                                    // memory that has been released
                                    // back to the runtime. Any
                                    // use of this will be broken.
                                    //
                                    // You have to hope that that exception
                                    // is not caught and the application
                                    // exits.

    str_cpy(str, size + 1, buffer);

    return *this;
}

The correct way to this is to implement the copy and swap idiom.

string& operator=(const char* buffer)
{
    string   tmp(buffer);   // safely create a copy.

    // Now that you have done the copy swap this with tmp
    std::swap(size,   tmp.size)
    std::swap(buffer, tmp.buffer);

    return *this;
}
// destructor of tmp is called here.
// it will release the buffer that you just placed into the object 

---

The standard library version of this std::string implements a nice short string optimization on top of the basic dynamic memory allocation version that you have implemented.

Answer 2

I might be missing something but why not std::string? I fail to understand why you cannot use STL or say other open source libraries. You gave some explanation but I fail to understand it. Utilizing STL and open source libraries will save you a ton of development and debugging time.

For you string implementation - Major Issues:

1. Adding a single character results in a reallocation which is terrible in terms of memory and performance. Normally, one holds a reserve size and increases it exponentially (x2 or x1.5 each time). So you won't need to apply reallocation each time somebody adds a single character or more at times.

2. It lacks short string optimization. When the string is short enough, say under 32 characters, then you shouldn't make a dynamic allocation and instead store the data locally. For this purpose you'll likely need extra buffer in the string class. This is important as most strings are fairly short.

Besides, these issues you should support more or less the same features that std::string supports. Take a look at its API on cppreference.

Answer 3

Having your strings both null-terminated and having an explicit size is a Bad Idea. C++ std::string, entirely not accidentally, doesn't do that.

You can allocate an extra character and set it to zero for ease of conversion to C-style strings. While converting from or comparing with C strings, you can (and should) test for the null terminator in the C string. Don't ever look for the null terminator in any other place of your code. Use size.

You also forgot to implement move semantics.