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;}
C++ string operators
c++;reinventing the wheel;memory management;overloading
Don't include unnecessary headersWe'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 stdIt's a bad habit that will cause problems in bigger programs.  Just don't.Add some compiler warningsI 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 sentinelWhat 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 constructorThis 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 leaksThe 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 WorltHello WorldThe World Wide Websh: 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 versionThis 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 efficientImplementing 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 lengthWe 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 managementInstead 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 contentsImproved, 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;    }};