My String Class

Question

Here's my very basic String class. This is my first attempt, to try to design and write a basic String class in C++, without using the absolute new features provided by newer C++.

class MyString {
public:
        MyString(): pSize{0}, pStr{nullptr} {}
        
        MyString(const char* cstr): 
        pSize{compute_length(cstr)}, pStr{nullptr} 
        {
            pStr = new char[pCapacity];
            std::copy(cstr, cstr + pSize, pStr);
        }
        
        MyString(const MyString& rhs): 
        pSize{rhs.size()}, pStr{nullptr}
        {
            pStr = new char[pCapacity];
            std::copy(rhs.data(), rhs.data() + rhs.size(), pStr);
        }
        
        ~MyString()
        {
            if(pStr){
                delete[] pStr;
            }
        }
        
        size_t size() const { return pSize; }
        size_t capacity() const { return pCapacity; }
        const char* data() const { return pStr; }
        
        MyString& operator=(const MyString& rhs){
            if(this == &rhs)
                return *this;
            
            if(pCapacity < rhs.size()){
                delete[] pStr;
                pSize = rhs.pSize;
                pCapacity = (pCapacity < rhs.capacity()) ? rhs.capacity() : pCapacity;
                pStr = new char[pCapacity];

                std::copy(rhs.data(), rhs.data() + rhs.size(), pStr);
                return *this;
            }
            
            pSize = rhs.size();
            pCapacity = rhs.capacity();
            std::copy(rhs.data(), rhs.data() + rhs.size(), pStr);
            
            return *this;
        }
        
private:
    size_t pSize;
    char* pStr;
    size_t pCapacity = 14;
    
    inline size_t compute_length(const char* cstr)
    {
        if(cstr == "")
            return 0;
        
        size_t i = 0;
        while(cstr[++i] != '\0');
        return i;
    }
}

Any tips, suggestions, and/or feedback!

What are your reasons for prefering raw pointers to unique pointer? — theProgrammer
– theProgrammer, Commented Dec 30, 2020 at 23:25
theProgrammer, the main reason is, that I simply want to do this the hard way, before, I take the simpler approach. — Albin M
– Albin M, Commented Dec 30, 2020 at 23:28

Toby Speight · Accepted Answer · 2021-01-01 16:35:18Z

Trivial: the declaration is not complete - you're missing the final ; after the class definition.

Missing includes:

#include <algorithm>
#include <cstddef>

using std::size_t;  // don't do this in a header!

As noted in the comment, don't put standard library identifiers into the global namespace - just write the type in full throughout the interface.

Don't use a null pointer for an empty string. That's going to result in a lot of special-case code when you implement the rest of the String interface (e.g. operator==(), operator+=, and more).

In this constructor:

    MyString(const char* cstr): 
    pSize{compute_length(cstr)}, pStr{nullptr} 
    {
        pStr = new char[pCapacity];
        std::copy(cstr, cstr + pSize, pStr);
    }

As well as using an array which may be too small, we're assigning to pStr twice. We could do it just once, if we rearrange the order of the data members:

private:
    static constexpr std::size_t default_capacity = 14;
    std::size_t pSize;
    std::size_t pCapacity = default_capacity;
    char* pStr;

public:
    MyString(const char* cstr)
        : pSize{compute_length(cstr)},
          pCapacity{pSize},
          pStr{new char[pCapacity]}
    {
        std::copy_n(cstr, pSize, pStr);
    }

The same problem exists with the copy constructor, and is as easily fixed:

    MyString(const MyString& rhs):
        pSize{rhs.pSize},
        pCapacity{pSize},
        pStr{new char[pCapacity]}
    {
        std::copy_n(rhs.pStr, pSize, pStr);
    }

Actually, I'd re-write all the constructors both in terms of a new (const char*, std::size_t) constructor:

public:
    MyString(const char* s, std::size_t len)
        : pSize{len},
          pCapacity{len},
          pStr{new char[len]}
    {
        std::copy_n(s, len, pStr);
    }

    MyString()
        : MyString{""}
    {}

    MyString(const char* cstr)
        : MyString{cstr, std::strlen(cstr)}
    {}

    MyString(const MyString& rhs)
        : MyString{rhs.pStr, rhs.pSize}
    {}

Well done for remembering the test for self-assignment here:

MyString& operator=(const MyString& rhs){
    if(this == &rhs)
        return *this;

I'm not convinced it's a good idea to retain the string's storage when it's already large enough to accommodate rhs - this means that any string that's ever held a very long string will continue to use that space until it's destroyed. That's not what most users expect when they write s = ""! Even worse, we copy that huge capacity to any other string we construct or assign from this:

MyString s = some_huge_string();   // capacity is now big
s = "";  // capacity is still big
MyString s1 = s;  // s1's capacity is also big, even though it's an empty string!
MyString s2{""};  // s2's capacity is small
s2 = s;           // now it's huge, even though its content is unchanged

Perhaps use a threshold to determine whether the storage should be reduced? I'd go with something like

    if (rhs.pSize > pCapacity
        || rhs.pSize < pCapacity / 2 && rhs.pSize < default_capacity)

And much of the duplication in this function can be reduced:

MyString& operator=(const MyString& rhs){
    if (this == &rhs) {
        return *this;
    }

    if (rhs.pSize > pCapacity
        || rhs.pSize < pCapacity / 2 && rhs.pSize < default_capacity)
    {
        char *new_str = new char[rhs.pSize];
        delete[] pStr;
        pStr = new_str;
        pCapacity = rhs.pSize;
    }

    pSize = rhs.size();
    std::copy_n(rhs.pStr, pSize, pStr);
    return *this;
}

You'll need to write a move-constructor and move-assignment soon - I recommend writing a swap() first, and implementing those in terms of swap().

Modified code

As there were no tests, I added a simple main() to exercise the class.

#include <algorithm>
#include <cstddef>
#include <cstring>

class MyString {
public:
    MyString(const char* s, std::size_t len)
        : pSize{len},
          pCapacity{len},
          pStr{new char[len]}
    {
        std::copy_n(s, len, pStr);
    }

    MyString()
        : MyString{""}
    {}

    MyString(const char* cstr)
        : MyString{cstr, std::strlen(cstr)}
    {}

    MyString(const MyString& rhs)
        : MyString{rhs.pStr, rhs.pSize}
    {}

    ~MyString()
    {
        delete[] pStr;
    }

    std::size_t size() const { return pSize; }
    std::size_t capacity() const { return pCapacity; }
    const char* data() const { return pStr; }

    MyString& operator=(const MyString& rhs)
    {
        if (this == &rhs) {
            return *this;
        }

        if (rhs.pSize > pCapacity
            || rhs.pSize < pCapacity / 2 && rhs.pSize < default_capacity)
        {
            char *new_str = new char[rhs.pSize];
            delete[] pStr;
            pStr = new_str;
            pCapacity = rhs.pSize;
        }

        pSize = rhs.size();
        std::copy_n(rhs.pStr, pSize, pStr);
        return *this;
    }

private:
    static constexpr std::size_t default_capacity = 14;
    std::size_t pSize;
    std::size_t pCapacity = default_capacity;
    char* pStr;
};

#include <iostream>
int main()
{
    MyString s0 = "abc";
    MyString s1 = s0;
    s1 = "";
    s1 = s0;
    std::cout.write(s1.data(), s1.size());
    std::cout << '\n';
}

Doesn't calling self constructor from constructor create overhead and copies? — Albin M
– Albin M, Commented Jan 1, 2021 at 13:37
No, that's how we write a delegating constructor. They both work on the same instance, with no extra copy. — Toby Speight
– Toby Speight, Commented Jan 1, 2021 at 14:46
I am getting CRT memory leak at line: pStr{new char[pCapacity]}, 14 bytes long. — Albin M
– Albin M, Commented Jan 1, 2021 at 16:25

G. Sliepen · Accepted Answer · 2020-12-31 12:07:18Z

There are some bugs

There are several bugs in your code. In the constructor that takes a C string, you compute the length of cstr and set pSize accordingly, but you allocate pCapacity bytes. You should allocate at least pSize bytes. I recommend you get rid of pCapacity and try to make the class work with just pSize first. You make the same mistake in operator=().

Second, the way you compute the length is wrong. The compiler should have warned you about comparing cstr to "", if not turn on compiler warnings. The statement:

if(cstr == "")

Is comparing the pointers, not the contents of the C strings. The correct way is to write if(cstr[0] == '\0'), but even that is not necessary; you should just make the loop below work correctly even for the corner case of a zero length string.

Store a terminating NUL-byte

It is very common to want to access a string object as a regular C string. And the member function data() almost allows you to do that, except that you don't reserve memory for and don't store a NUL byte at the end. I recommend you do that, it makes accessing a MyString as a C string much easier, and will prevent some accidents.

No need to check for NULL pointers before calling `delete`

Calling delete on a NULL pointer is guaranteed to be safe. So:

~MyString()
{
    delete[] pStr;
}

Or even better, as mentioned in the comments, use std::unique_ptr to handle memory for you. Don't reinvent too many wheels at the same time.

Handle out-of-memory conditions correctly

Be aware that calling new might fail if the program has ran out of memory. In that case, an exception will be thrown. You can choose to ignore the exception, but you must do it in such a way that your object is left in a valid state. Consider that operator= might fail calling new, then pStr is left pointing to the old data which is already deleted. If the caller handles the exception, then you have a problem, since as soon as the destructor is called it tries to delete the memory again.

The solution is to try to allocate memory for the copy first before deleting the old memory:

if(pSize < rhs.size()){
    char *newStr = new char[rhs.size()];
    delete[] pStr;
    pStr = newStr;
}

pSize = rhs.size();
...

Hi thx, what bugs are there really? I am using Visual Studio the latest community edition. — Albin M
– Albin M, Commented Dec 30, 2020 at 23:42

Stack Exchange Network

My String Class

2 Answers 2

Modified code

There are some bugs

Store a terminating NUL-byte

No need to check for NULL pointers before calling `delete`

Handle out-of-memory conditions correctly

You must log in to answer this question.

Hot Network Questions

My String Class

2 Answers 2

Modified code

There are some bugs

Store a terminating NUL-byte

No need to check for NULL pointers before calling delete

Handle out-of-memory conditions correctly

You must log in to answer this question.

Related

Hot Network Questions

No need to check for NULL pointers before calling `delete`