C++ string operators

Question

I'm trying to practice my C++ on my own; At the moment I'm trying to implement the concept of operator overloading making my own string class/function.

I think I handled this correctly; the code runs successfully. However I'm not so sure if my code is optimally written (do I need to make more methods/params const? Do I use to much excess code?) and did I do good memory management?

Thanks for all the feedback :)

#include <iostream>
#include <vector>
#include <string.h>
#include <stdio.h>
using namespace std;
class DynString {
private:
    char *c;

public:
    ~DynString() { delete[]c; }
    DynString() : c(NULL) {}
    //second constructor
    DynString(const char* input) {
        int x = strlen(input) + 1;
        c = new char[x];
        strcpy(c, input);
    }

    //(copyconstructor) TRIO //QUATRO
    DynString(const DynString& dynstring) {
        c = new char[strlen(dynstring.c) + 1];
        strcpy(this->c, dynstring.c);
    }
    //UNO
    friend ostream& operator<< (ostream& lhs, DynString& a) {
        lhs << a.c;
        return lhs;
    }
    //DUO
    char& operator[] (int i) {
        return c[i];
    }
    //TRIO
    DynString& operator+ (DynString& y) {
        char *newArray = new char[strlen(y.c) + strlen(this->c) + 1];
        strcpy(newArray, this->c);
        strcat(newArray, y.c);
        delete[] c;
        c = new char[strlen(newArray)];
        this->c = newArray; 
        return *this;
    }


    //QUATRO
    friend DynString operator+ (char* x, DynString& y) {
        DynString newString;
        newString.c = new char[strlen(x) + strlen(y.c) + 1];
        strcpy(newString.c, x);
        strcat(newString.c, y.c);

        return newString;
    }
    //QUATRO
    DynString& operator+ (char* x) {
        char *newArray = new char[strlen(x) + strlen(this->c) + 1];
        strcpy(newArray, this->c);
        strcat(newArray, x);
        delete[] c; 
        c = new char[strlen(newArray)];
        this->c = newArray;

        return *this;
    }
    //TRIO // QUATRO
    DynString& operator= (DynString& x) {
        char *newArray = new char[strlen(x.c) + 1];
        strcpy(newArray, x.c);
        delete[] c;
        c = new char[strlen(newArray)];
        this->c = newArray;
        return *this;
    }

};



int main()
{
    DynString stringA("Hello");
    DynString stringB(" Worlt");
    //UNO
    cout << stringA << stringB << std::endl;

    //DUO
    stringB[5] = 'd';

    //TRIO
    DynString stringC = stringA + stringB;
    cout << stringC << std::endl;

    DynString stringD;
    //QUATRO
    stringD = "The" + stringB + " Wide Web";
    cout << stringD << std::endl;

    system("pause");

    return 0;
}

Answer 1

Don't include unnecessary headers

We're not using <vector> or <stdio.h>, so drop them. We only need <ostream> rather than <iostream> for the class, and we prefer <cstring> to <string.h> to place the standard functions in their proper namespace.

Avoid importing all of namespace std

It's a bad habit that will cause problems in bigger programs. Just don't.

Add some compiler warnings

I get some useful diagnostics, worth heeding, with g++ -Weffc++:

165851.cpp:33:39: warning: ‘DynString& DynString::operator+(DynString&)’ should return by value [-Weffc++]
     DynString& operator+ (DynString& y) {
                                       ^
165851.cpp:54:34: warning: ‘DynString& DynString::operator+(char*)’ should return by value [-Weffc++]
     DynString& operator+ (char* x) {
                                  ^
165851.cpp: In constructor ‘DynString::DynString(const char*)’:
165851.cpp:12:5: warning: ‘DynString::c’ should be initialized in the member initialization list [-Weffc++]
     DynString(const char* input) {
     ^~~~~~~~~
165851.cpp: In copy constructor ‘DynString::DynString(const DynString&)’:
165851.cpp:19:5: warning: ‘DynString::c’ should be initialized in the member initialization list [-Weffc++]
     DynString(const DynString& dynstring) {
     ^~~~~~~~~
165851.cpp: In function ‘int main()’:
165851.cpp:95:23: warning: ISO C++ forbids converting a string constant to ‘char*’ [-Wpedantic]
     stringD = "The" + stringB + " Wide Web";
                       ^~~~~~~
165851.cpp:95:33: warning: ISO C++ forbids converting a string constant to ‘char*’ [-Wpedantic]
     stringD = "The" + stringB + " Wide Web";
                                 ^~~~~~~~~~~

The last two are easily fixed by changing operator+ to accept const char* instead of char*.

The usual way of fixing the warnings on operator+ is to implement operator+ in terms of operator+= (the latter returns a reference, but the former an object).

BUG - be careful using a null pointer as sentinel

What happens if you evaluate DynString() + DynString()? In both cases, the internal pointer points to NULL, but you happily call strlen on both - that's Undefined Behaviour. Here's some code that actually checks:

DynString(const DynString& other)
    : c(nullptr)
{
    if (other.c) {
        c = new char[std::strlen(other.c) + 1];
        std::strcpy(c, other.c);
    }
}

But it's probably easier (safer) to always allocate memory:

DynString(const char* input)
    : c(new char[(input ? std::strlen(input) : 0) + 1];)
{
    if (input) {
        std::strcpy(c, input);
    } else {
        *c = '\0';
    }
}

And then all methods can safely assume c isn't null.

Add a move constructor

This one's easy:

DynString(DynString&& other)
    : DynString()
{
    std::swap(c, other.c);
}

Overload operator[]

Allow for both constant and mutable string objects:

char& operator[](size_t i) {
    return c[i];
}

const char& operator[](size_t i) const {
    return c[i];
}

I believe size_t is more appropriate than int as the type of the index.

Eliminate the memory leaks

The example program leaks:

==13517== Memcheck, a memory error detector
==13517== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==13517== Using Valgrind-3.12.0.SVN and LibVEX; rerun with -h for copyright info
==13517== Command: ./165851
==13517== 
Hello Worlt
Hello World
The World Wide Web
sh: 1: pause: not found
==13517== 
==13517== HEAP SUMMARY:
==13517==     in use at exit: 47 bytes in 3 blocks
==13517==   total heap usage: 12 allocs, 9 frees, 73,860 bytes allocated
==13517== 
==13517== 11 bytes in 1 blocks are definitely lost in loss record 1 of 3
==13517==    at 0x4C2C93F: operator new[](unsigned long) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==13517==    by 0x108FF3: DynString::operator+(DynString&) (165851.cpp:38)
==13517==    by 0x108C62: main (165851.cpp:90)
==13517== 
==13517== 18 bytes in 1 blocks are definitely lost in loss record 2 of 3
==13517==    at 0x4C2C93F: operator new[](unsigned long) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==13517==    by 0x10915B: DynString::operator+(char*) (165851.cpp:59)
==13517==    by 0x108CD5: main (165851.cpp:95)
==13517== 
==13517== 18 bytes in 1 blocks are definitely lost in loss record 3 of 3
==13517==    at 0x4C2C93F: operator new[](unsigned long) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==13517==    by 0x1091EF: DynString::operator=(DynString&) (165851.cpp:69)
==13517==    by 0x108CE7: main (165851.cpp:95)
==13517== 
==13517== LEAK SUMMARY:
==13517==    definitely lost: 47 bytes in 3 blocks
==13517==    indirectly lost: 0 bytes in 0 blocks
==13517==      possibly lost: 0 bytes in 0 blocks
==13517==    still reachable: 0 bytes in 0 blocks
==13517==         suppressed: 0 bytes in 0 blocks
==13517== 
==13517== For counts of detected and suppressed errors, rerun with: -v
==13517== ERROR SUMMARY: 3 errors from 3 contexts (suppressed: 0 from 0)

The main cause is:

    char *newArray = new char[strlen(y.c) + strlen(this->c) + 1];
    strcpy(newArray, this->c);
    strcat(newArray, y.c);
    delete[] c;
    c = new char[strlen(newArray)];
    this->c = newArray;

The error is more visible if you remove the redundant this-> from the last line:

    delete[] c;
    c = new char[strlen(newArray)];
    c = newArray;

The first allocation is obviously leaked, and that line of code should be deleted. The same error is present in several other methods.

Implementation of operator+:

I would write it like this, starting with the += that the other versions need:

public:
    DynString& operator+=(const DynString& other)
    {
        char *newArray = new char[strlen(c) + strlen(other.c) + 1];
        std::strcpy(newArray, c);
        std::strcat(newArray, other.c);
        delete[] c;
        c =  newArray;
        return *this;
    }

    DynString operator+(const DynString& other) const
    {
        DynString newString(*this);
        return newString += other;
    }

We don't need operator+(const char*), as a C-style string will be promoted to DynString using the (non-explicit) constructor. We do need an overload to take a C-style string as the first argument - but it doesn't need to be a friend, as it can be an ordinary non-member function instead:

DynString operator+(const char* x, const DynString& other) {
    DynString newString(x);
    return newString += other;
}

---

Improved version

This code is tested and does not leak:

#include <ostream>
#include <cstring>

class DynString {
private:
    char *c;

public:
    ~DynString()
    {
        delete[] c;
    }

    DynString(const char* input = nullptr)
        : c(new char[(input ? std::strlen(input) : 0) + 1])
    {
        if (input) {
            std::strcpy(c, input);
        } else {
            *c = '\0';
        }
    }

    DynString(const DynString& other)
        : c(new char[std::strlen(other.c) + 1])
    {
        std::strcpy(c, other.c);
    }

    DynString(DynString&& other)
        : DynString()
    {
        std::swap(c, other.c);
    }

    friend std::ostream& operator<<(std::ostream& lhs, const DynString& a)
    {
        return lhs << a.c;
    }


    char& operator[](size_t i)
    {
        return c[i];
    }

    const char& operator[](size_t i) const
    {
        return c[i];
    }


    DynString& operator+=(const DynString& other)
    {
        char *newArray = new char[strlen(c) + strlen(other.c) + 1];
        std::strcpy(newArray, c);
        std::strcat(newArray, other.c);
        delete[] c;
        c =  newArray;
        return *this;
    }

    DynString operator+(const DynString& other) const
    {
        DynString newString(*this);
        return newString += other;
    }

   DynString& operator=(DynString other) {
       std::swap(c, other.c);
       return *this;
   }
};


DynString operator+(const char* x, DynString& other) {
    DynString newString(x);
    return newString += other;
}


#include <iostream>
int main()
{
    DynString stringA("Hello");
    DynString stringB(" Worlt");
    //UNO
    std::cout << stringA << stringB << std::endl;

    //DUO
    stringB[5] = 'd';

    //TRIO
    DynString stringC = stringA + stringB;
    std::cout << stringC << std::endl;

    DynString stringD;
    //QUATRO
    stringD = "The" + stringB + " Wide Web";
    std::cout << stringD << std::endl;
}

---

Make the concatenation more efficient

Implementing operator+ by copying and appending is inefficient because it allocates memory for the copy that is immediately released during the append. So we can revisit that method and eliminate the copy step. I'd do it using an extra constructor:

private:
    DynString(const DynString& a, const DynString& b)
        : c(new char[std::strlen(a.c) + std::strlen(b.c) + 1])
    {
        std::strcpy(c, a.c);
        std::strcat(c, b.c);
    }

public:
    DynString operator+(const DynString& other) const
    {
        return DynString(*this, other);
    }

We can now simplify operator+=, implementing that in terms of our new operator+:

void swap(DynString& other)
{
    std::swap(length, other.length);
    std::swap(c, other.c);
}

DynString& operator+=(const DynString& other)
{
    Dynstring newString(*this, other);
    swap(newString);
    return *this;
}

Remember the string length

We can make every method more efficient, and also allow for embedded NUL characters in strings, if we store the length as a member:

private:
    std::size_t length;         // not including terminator
    char *c;

    DynString(const DynString& a, const DynString& b)
        : length(a.length + b.length),
          c(new char[length + 1])
    {
        std::memcpy(c, a.c, a.length);
        std::memcpy(c+a.length, b.c, b.length+1); // copies terminator
    }

Lean on the standard library for memory management

Instead of managing our own memory, we could make c be a unique pointer:

private:
    std::size_t length;         // not including terminator
    std::unique_ptr<char[]> c;

    DynString(const DynString& a, const DynString& b)
        : length(a.length + b.length),
          c(std::make_unique<char[]>(length + 1))
    {
        std::memcpy(c.get(), a.c.get(), a.length);
        std::memcpy(c.get()+a.length, b.c.get(), b.length+1); // copies terminator
    }

You'll see that we do need to sprinkle calls to get() where we need pointers to the contents

---

Improved, efficient, smart-pointer version:

(just the class; everything else unchanged from first "Improved version" above, except an additional include)

#include <memory>

class DynString {
private:
    std::size_t length;         // not including terminator
    std::unique_ptr<char[]> c;

    // Helper constructor - allocates but does not initialize
    DynString(size_t length)
        : length(length),
          c(std::make_unique<char[]>(length + 1))
    {}

    DynString(const DynString& a, const DynString& b)
        : DynString(a.length + b.length)
    {
        std::memcpy(c.get(), a.c.get(), a.length);
        std::memcpy(c.get()+a.length, b.c.get(), b.length+1); // copies terminator
    }

public:
    DynString(const char* input = nullptr)
        : DynString(input ? std::strlen(input) : 0)
    {
        if (input) {
            std::memcpy(c.get(), input, length + 1);
        } else {
            c.get()[0] = '\0';
        }
    }

    DynString(const DynString& other)
        : DynString(other.length)
    {
        if (length != other.length)
            c = std::make_unique<char[]>(other.length+1);
        length = other.length;
        std::memcpy(c.get(), other.c.get(), length + 1);
    }

    DynString(DynString&& other) noexcept
        : DynString()
    {
        swap(other);
    }

    void swap(DynString& other) noexcept
    {
        std::swap(length, other.length);
        std::swap(c, other.c);
    }

    friend std::ostream& operator<<(std::ostream& lhs, const DynString& a)
    {
        return lhs << a.c.get();
    }


    char& operator[](size_t i) noexcept
    {
        return c.get()[i];
    }

    const char& operator[](size_t i) const noexcept
    {
        return c.get()[i];
    }


    DynString& operator+=(const DynString& other)
    {
        DynString newString(*this, other);
        swap(newString);
        return *this;
    }

    DynString operator+(const DynString& other) const
    {
        return DynString(*this, other);
    }


    DynString& operator=(DynString other) noexcept
    {
        swap(other);
        return *this;
    }
};

Answer 2

1. Don't include what you don't use. You only use <string.h> and <stdio.h>.
   While including <vector> only costs compile-time, including <iostream> potentially increases your executable-size and startup-/shutdown-time.

2. You should avoid using namespace std; It's a plague and just biding its time to bite you later.
   Why is “using namespace std;” considered bad practice?

3. I'm sorry, but your comments seem to be completely useless. At least I couldn't extract any useful information from them. So, purge them.

4. Your DynString seems to be a thin wrapper around 0-terminated strings. Are you sure you will never need embedded zeroes, nor have enough use of the length that caching it might be worthwhile?
   Another disadvantage is that due to the also missing capacity appending to the string will always re-allocate.

5. Raw owning pointers are a bad idea. Not only are they leak-prone per se, things get even harder with exceptions.
   Consider a move to the no-cost abstraction of std::unique_ptr.

6. Your nullptr-handling is inconsistent. Is nullptr UB, an empty string, or preserved as a separate state?

7. As an aside, prefer nullptr to NULL or 0. Not only does it have a distinctive type, it also cannot be confused with an integer.

8. Use std::size_t for indices and sizes. That's what it's for.

9. You forgot to mark your functions noexcept where allowed.

10. Your default-constructor should be marked constexpr, or allocate a 0-length string.

11. In DynString(const char*) you calculate the length of the input for allocating the proper amount. Why don't you reuse that for an efficient memcpy instead of doing a less performant strcpy?
    At least it makes sense to allow using this for implicit conversions. Otherwise, you would have to mark it explicit.

12. DynString(const DynString&) should delegate to DynString(const char*) instead of writing everything out all over again.

13. You are missing a move-ctor.

14. The stream-output-operator does not modify the second argument, and should thuss accept it by constant reference.

15. You are missing a constant overload for operator []. And anyway, the index should be a std::size_t, not an int.

16. Your operator+(DynString&) (and operator+(const char*) too) is a first-grade mess:

    • It should return a new DynString by value, not one of the arguments by reference.
    • Both arguments should be constant.
    • You shouldn't allocate a second time in there just to leak it. Besides, that second allocation could also throw...
    • It should be a friend-function, thus enabling conversion on its first argument, delegating to a private method concatenating two const char*s. Doing so allows you to implement the two variants replacing one of the arguments with a const char* for efficiency without duplication.
    • Why do you not reuse the string-lengths you calculated? memcpy is more efficient than strcpy.

17. At least operator+ (char* x, DynString& y) is not wrong, only inefficient.

18. Consider adding operator+= (both varieties for efficiency), implemented in terms of operator+ and operator=.

19. Your operator= should not accept an lvalue-reference. Accept the argument by value and use copy-and-swap. That way you also lose thememory-leak.

20. You really should add some more observers, to wit: .data(), .size(), .begin() and .end() at least.

Modified code, deciding that nullptr is UB, and throwing in some extras:

#include <cstring>
#include <algorithm>
#include <utility>
#include <iterator>
#include <vector>

class DynString {
    char *c;
    DynString(std::picewise_construct_t, const char* a, const char* b) : c() {
        auto na = std::strlen(a), nb = std::strlen(b);
        c = new char[a + b + 1 > a ? a + b + 1 : SIZE_MAX];
        std::copy_n(a, na, c);
        std::copy_n(b, nb + 1, c + na);
    }
    template<class It, decltype((void)(*c = *std::declval<It>()))...>
    DynString(std::forward_iterator_tag, It first, It last) : c() {
        auto n = std::distance(first, last);
        c = new char[0 <= n && n < SIZE_MAX ? std::size_t(n) + 1 : SIZE_MAX];
        std::copy_n(first, std::size_t(n), c);
        c[n] = 0;
    }
    template<class It, decltype((void)(*c = *std::declval<It>()))...>
    DynString(std::input_iterator_tag, It first, It last) : c() {
        std::vector<char> v(first, last);
        swap({v.begin(), v.end()});
    }
public:
    DynString() : c(new char[1]{}) {}
    DynString(const char* s) : DynString(s, s + std::strlen(s)) {}
    DynString(const DynString& s) : DynString(s.c) {}
    DynString(DynString&& s) : DynString() { swap(s); }
    template<class It, decltype((void)DynString(std::iterator_traits<It>::iterator_category(), first, last))...>
    DynString(It first, It last) : DynString(std::iterator_traits<It>::iterator_category(), first, last) {}
    ~DynString() { delete[] c; }
    friend ostream& operator<<(ostream& lhs, const DynString& a) { return lhs << a.c; }
    char& operator[](std::size_t i) noexcept { return c[i]; }
    const char& operator[](std::size_t i) const noexcept { return c[i]; }
    auto data() noexcept { return c; }
    auto data() const noexcept { return c; }
    auto begin() noexcept { return c; }
    auto begin() const noexcept { return c; }
    auto end() noexcept { return c + size(); }
    auto end() const noexcept { return c + size(); }
    auto rbegin() noexcept { return std::make_reverse_iterator(end()); }
    auto rbegin() const noexcept { return std::make_reverse_iterator(end()); }
    auto rend() noexcept { return std::make_reverse_iterator(begin()); }
    auto rend() const noexcept { return std::make_reverse_iterator(begin()); }
    auto size() const noexcept { return std::strlen(c); }
    friend DynString operator+(const DynString& a, const DynString& b)
    { return {std::piecewise_construct, a.c, b.c}; }
    friend DynString operator+(const char* a, const DynString& b)
    { return {std::piecewise_construct, a, b.c}; }
    friend DynString operator+(const DynString& a, const char* b)
    { return {std::piecewise_construct, a.c, b}; }
    DynString& operator+=(const DynString& s) { return *this += s.c; }
    DynString& operator+=(const char* s) { return *this = *this + s; }
    DynString& operator=(DynString s) { swap(s); return *this; }
    void swap(DynString& o) noexcept { using std::swap; swap(c, o.c); }
};

Answer 3

Memory leak here (in operator+):

 c = new char[strlen(newArray)];
 this->c = newArray;

because there is no difference between c and this->c. The code will overwrite the pointer to the new-allocated memory.

Answer 4

One thing I think the other answers missed: you're using strcat, strcpy and strlen. It's 2017, you're writing C++ -- never do this.

Instead of writing C++ as if you're writing C in 1978, please use the strn* family of functions: strncat, strncpy, and a strnlen implementation: Microsoft's strnlen, or the GNU strnlen if you are using glibc (Linux, some BSDs, etc).

Additionally, in C++ you have access to nice functions like std::copy and std::find.

Doing so makes your code inherently safer (less prone to buffer overflows), more predictable and reliable, and far more modern.

Answer 5

operator+ implementation has a couple MAJOR bugs

DynString& operator+ (DynString& y) {
    char *newArray = new char[strlen(y.c) + strlen(this->c) + 1];
    strcpy(newArray, this->c);
    strcat(newArray, y.c);
    delete[] c;
    c = new char[strlen(newArray)];
    this->c = newArray; 
    return *this;
}

This implementation will cause some pretty bad side effects. Consider the following cases where A,B and C are DynString types in the current scope:

DynString A = "foo";
DynString B = "bar";
A = A + B;  //this example works and does what it looks like it should

Now Consider:

DynString A = "foo";
DynString B = "bar";
DynString C = A + B;  //this example not work like it should

or alternatively

DynString A = "foo";
DynString B = "bar";
A + B;  //this example not work like it should

These examples have unintended side effects. After running this block the strings store the following values:

B == "bar"    (Expected Result)
C == "foobar" (Expected Result, in case where C exist)
A == "foobar" (Unexpected Result!)

Lastly, we're returning by reference so consider this case:

DynString A = "foo";
DynString B = A + "bar" + "dude"; //contrived example, but clear point

After running the above case we expect that B is "foobardude" and what we expect of A is dependent on whether we're aware of the bug in the previous example. We should expect A is either "foo" or "foobar" because a temp should be returned. Neither of those will be the case, A will be "foobardude". As written, that implementation won't allow concatenation without altering a variable's original value.

Answer 6

Your destructor will try to free the memory even if it has not been allocated. Maybe create a boolean condition to check for that.