C++ String Class Reimplementation

Question

I started learning C++ about a week ago and I was hoping somebody can critique my code. I decided to implement my own basic String class just for the sake of applying all that I've learned about pointers, C-style strings, and move semantics. I know there is already such a class in the standard library but as I said, I only did this to apply what I currently know.

Anyhow, here is the string_re.h header file:

#pragma once

class String {
private:
    char* m_Buffer;
    int m_Size;

public:
    String();
    String(const char*);
    String(String&);
    ~String();

    String& operator=(String&);
    String& operator=(String&&);
    String& operator+=(const String&);
    String operator+(const String&);

    friend String operator+(const char*, const String&);
    friend int len(String&);

    void print();
};

String operator+(const char*, const String&);
int len(const char*);
int len(String&);
void copy(char*, const char*, int);

And here is the string_re.cpp implementation file:

#include <cstdio>
#include "string_re.h"

String::String() {
    m_Buffer = nullptr;
    m_Size = 0;
}

String::String(const char* c) {
    m_Size = len(c);
    m_Buffer = new char[m_Size + 1];
    copy(m_Buffer, c, 0);
}

String::String(String& s) {
    m_Size = s.m_Size;
    m_Buffer = new char[m_Size + 1];
    copy(m_Buffer, s.m_Buffer, 0);
}

String::~String() {
    delete[] m_Buffer;
}

String& String::operator=(String& s) {
    delete[] m_Buffer;
    m_Size = s.m_Size;
    m_Buffer = new char[m_Size + 1];
    copy(m_Buffer, s.m_Buffer, 0);
    return *this;
}

String& String::operator=(String&& s) {
    delete[] m_Buffer;
    m_Size = s.m_Size;
    m_Buffer = s.m_Buffer;

    s.m_Size = 0;
    s.m_Buffer = nullptr;
    return *this;
}

String& String::operator+=(const String& s) {
    int size = m_Size + s.m_Size;
    char* new_buffer = new char[size + 1];
    copy(new_buffer, m_Buffer, 0);
    copy(new_buffer, s.m_Buffer, m_Size);

    delete[] m_Buffer;
    m_Buffer = new_buffer;
    m_Size = size;
    return *this;
}

String String::operator+(const String& s) {
    String tmp(*this);
    tmp += s;
    return tmp;
}

String operator+(const char* c, const String& s) {
    String tmp(c);
    tmp += s;
    return tmp;
}

void String::print() {
    printf("%s\n", m_Buffer);
}

int len(const char* c) {
    int size = 0;
    while (c[size] != '\0') size++;
    return size;
}

int len(String& s) {
    return s.m_Size;
}

void copy(char* c1, const char* c2, int start) {
    if (c2 == nullptr) return;
    while (*c2 != '\0') c1[start++] = *c2++;
    c1[start] = *c2;
}

Yes, I know there is already a memcpy and a strlen function but I decided to implement my own anyways. All my String class does is concatenate other Strings and nothing more. Also, I feel like there's a better way to implement the + operator function but I just can't wrap my head around it. Any feedback would be much appreciated!

Answer 1

1. Support embedded zero.
   Currently, you are half-way to having a counted string instead of a 0-terminated string, going the rest of the way is just cleanup (copying a sequence of given length instead of a c-string), adding a ctor accepting a length, and fixing .print().

2. Use the standard algorithms, specifically std::copy() and std::strlen().
   Yes, you decided to roll your own for the fun of it, but I mention it anyway for completeness.

3. Make the default ctor trivial, thus turning the type trivial.
   This is done by adding default initializers to all member variables and explicitly defaulting the ctor in the class definition.

        char* m_Buffer = 0;
        int m_Size = 0;
    public:
        String() = default;
   

4. Copy ctor and copy assignment should accept their arguments by constant reference, or they won't accept constant sources. Actually, mark anything which you won't mutate const for easier reasoning and compiler checking, including operator+().

5. Use the member initializer list.
   In many cases, using it is necessary to get it to compile, to get it to work right, or to make it efficient.

   In your case, remember that if allocating in the ctor body fails (not ctor init list!), the corresponding dtor will be run and expect some valid state.

6. Consolidate your ctors using delegating ctors. No need for duplicating code. In your case, use it for ctor chaining to delegate from the other two ctors to a common implementation.

        String(const char* s) : String(s, strlen(s)) {}
        String(const String& s) : String(s.m_Buffer, s.m_Size) {}
        String(const char* s, std::size_t n) : m_Buffer(new char[n + 1]), m_Size(n) {
            std::copy_n(s, n, m_Buffer);
            m_Buffer[n] = '\0';
        }
   

7. Swapping is a fundamental operation which is also very useful for the implementation. Provide it.

        void swap(String& s) noexcept { swap(*this, s); }
        friend void swap(String& a, String& b) noexcept {
            std::swap(a.m_Buffer, b.m_Buffer);
            std::swap(a.m_Size, b.m_Size);
        }
   

8. Move-assignment and move-construction should be simple and efficient, but at the least always succeed.

        String(String&& s) noexcept { swap(s); }
        String& operator=(String&& s) noexcept { swap(s); return *this; }
   

9. Self-assignment is a thing.
   While optimizing for it is generally self-defeating, it should work.
   I suggest leveraging ctor and swap or move-assignment.

        String& operator=(const String& s) { return *this = String(s); }
   

10. Use the standard interface.

    Thus the size should be returned by member-function .size(), and printing should be done to any stream using operator<<.

Answer 2

• Consider putting your class inside a namespace. This ensures that it will not conflict with any other library that also defines a String class.

• The general convention is to use size_t to define a variable that models size or index in an array. See this SO answer.

• Follow the rule of 3/5/0. This means defining a move constructor for your class.

String(String&& s)
{
   ...
}

• Prefer using a member initialization list instead of assigning values to member variables inside the constructor body. For example,

String::String(): m_Buffer(nullptr), m_Size(0) {
    
}

This is slightly more efficient, as it avoids default construction of the object followed by an assignment. (See this SO answer). In the above case, it probably doesn't make much of a difference in terms of performance, but it's good practice to do it anyway.

• The C++ way of doing print is by overloading operator<<. For example,

ostream& operator<<(ostream& o, const String& s) {
   o << s.m_Buffer;
   return o;
}

Also, prefer to use C++'s std::cout or the newly introduced std::format instead of <cstdio>.

• There's a subtle bug in your assignment operator. Consider if a user does the following (Compiler Explorer):

String s("Hello world!");
s = s;

This will invoke operator=, which will first delete[] m_Buffer; however, the subsequent copy(..) will result in a segfault since you're trying to access deleted memory.

A better way would be to first allocate a new array, copy the contents into the new array, swap the array and m_Buffer pointers, and then delete the old pointer.

General Design

• In general, header file's should contain the interface to your class (exceptions always exist). In your case, len and copy are implementation details that user's shouldn't care about; consider them making them static or putting them inside an anonymous namespace in your translation unit (the .cpp file).

• Currently, you're allocating a new array on the heap for every operator+=. This can be wasteful, especially if you're doing multiple small concatenations. Most implementations allocate more memory than needed, represented by a capacity field. Concatenation then becomes,

if current_size + size_to_be_added < capacity {
    concat to already existing buffer
} else {
    set capacity to new value (typically 2 * existing_capacity)
    allocate buffer with new capacity
    copy data from old buffer to new buffer
}

Answer 3

OK. Trying not to copy what @Rish and @Deduplicator mentioned.

This does not provide the strong Exception guarantee.

String& String::operator=(String& s) {
    delete[] m_Buffer;
    m_Size = s.m_Size;
    m_Buffer = new char[m_Size + 1];
    copy(m_Buffer, s.m_Buffer, 0);
    return *this;
}

Your code should either work as expected. OR it should fail with an exception BUT also leave the object in the original state.

The issue here is that you delete[] m_buffer before you know that the new char[m_Size + 1] is going to work. If the new fails and throws your object is left in an inconsistent BAD state. If you happen to catch that exception your program is doomed to crash in a way that is impossible to debug.

---

Both the move operators should be marked noexcept

String& String::String(String&& s) noexcept // <-- Here
{
    // Not provided!
}
String& String::operator=(String&& s) noexcept // <-- Here
{
    delete[] m_Buffer;
    m_Size = s.m_Size;
    m_Buffer = s.m_Buffer;

    s.m_Size = 0;
    s.m_Buffer = nullptr;
    return *this;
}

This actually provides benefits when you put your object in a standard container. The standard container must be able to provide "Strong Exception Guarantee" on several operations. It will use move if it can but this requires the move operators to be marked as noexcept to use this optimization. If they are not marked as noexcept then the container must use Copy versions to maintain the "Strong Exception Guarantee" which is not always as efficient.

---

The Assignment operator (both Move/Copy) is better implemented in a single method:

String& String::operator=(String&& s) {
    // -----
    delete[] m_Buffer;                  // Notice this is in the destructor.
    // -----
    m_Size = s.m_Size;                  // Notice this is in the
    m_Buffer = s.m_Buffer;              // Move Constructor

    s.m_Size = 0;
    s.m_Buffer = nullptr;
    // -------
    return *this;                       // This is all rest
}

String& String::operator=(String& s) {
    // ------
    delete[] m_Buffer;                  // Notice this is in the destructor
    // ------
    m_Size = s.m_Size;                  // Notice this is in the
    m_Buffer = new char[m_Size + 1];    // Copy Constructor
    copy(m_Buffer, s.m_Buffer, 0);
    // -------
    return *this;                       // This is all rest
}

These two can be replaced by:

String& String::operator=(String s) noexcept
{                                        // Construction / destruction done automatically (on s)
    s.swap(*this);
    return *this;
}

Notice the parameter is pass by value. If you are doing copy assignment your value is copied into s, while if you are doing move assignment your value is moved into s. All the rest of the work is done by the appropriate constructor then you simply have to swap s with the current state of the object and the destructor of s will handle and cleanup required.

@Deduplicator mentioned implementing swap.

---

This is a bit inefficient as you always have to allocate. If you had allocate some spare space and kept a m_Size and a m_Allocated you may not have to always allocate.

String& String::operator+=(const String& s) {
    int size = m_Size + s.m_Size;
    char* new_buffer = new char[size + 1];
    copy(new_buffer, m_Buffer, 0);
    copy(new_buffer, s.m_Buffer, m_Size);

    delete[] m_Buffer;
    m_Buffer = new_buffer;
    m_Size = size;
    return *this;
}

It becomes a bit more obvious in the next function.

String String::operator+(const String& s) {
    String tmp(*this);
    tmp += s;
    return tmp;
}

You make a copy. Which requires allocating new space. Then you immediately reallocate with more space. Thus doing two memory allocations and copying the the first string multiple times. With a private constructor you could make sure there is enough space for the operator+= during the initial copy then you don't have to do two allocations and two copies.

---

But also why are you using C streams!!!! One of the first things most C++ developers do is unbuckle the C++ streams and the C streams as it is highly inefficient to keep the bound together. C++ is its own language. You don't need C stuff.

void String::print() {           // Should be const does not change state.
    printf("%s\n", m_Buffer);    // NO.
}


class String {
   // Stuff
   void print(std::stream& str = std::cout) const {
        str << m_Buffer;
   }
   friend std::ostream& operator<<(std::ostream& str, String const& data) {
       data.print(str);
       return str;
   }
};