C++ overloading += operator in custom string class better implementation

Question

I am trying to learn C++, so I started coding a custom string class (using only c-style strings) to get familiar with concepts like operator overloading etc. in the case we have a pointer attribute. I wanted to know if there is a smarter/better/neater way to implement the += operator (or the others).

So I am basically defining a private char pointer to store the c-style string, two constructors (empty, char* arg), a copy constructor, a move constructor, a destructor (to deallocate the allocated space from the heap) and two operators (to concatenate strings).

mystring.h

#include <iostream> #include <cstring> class mystring { private: char* str; public: mystring(); mystring(char* str); mystring(const mystring &s); mystring(mystring &&s) noexcept; ~mystring(); mystring& operator=(const mystring &s); friend std::ostream& operator<<(std::ostream &out, const mystring &s); mystring operator+(const mystring &s); mystring& operator+=(const mystring &s); }; 

mystring.cpp

#include "mystring.h" mystring::mystring() : mystring(nullptr) {}; mystring::mystring(char* str) : str{nullptr} { if(str == nullptr){ this->str = new char; this->str = '\0'; }else { this->str = new char[strlen(str) + 1]; strcpy(this->str, str); } } // Deep copy mystring::mystring(const mystring &s) : str{nullptr} { if(str == nullptr){ this->str = new char; *(this->str) = '\0'; }else { this->str = new char[strlen(s.str) + 1]; strcpy(this->str, s.str); } } // Move constructor mystring::mystring(mystring &&s) noexcept : str{s.str} { s.str = nullptr; } mystring::~mystring() { delete [] str; } mystring& mystring::operator=(const mystring &s){ if(this != &s){ this->str = new char[strlen(s.str) + 1]; strcpy(this->str, s.str); } return *this; } std::ostream& operator<<(std::ostream &out, const mystring &s){ out << s.str; return out; } mystring mystring::operator+(const mystring &s){ mystring concat; concat.str = new char[strlen(this->str) + strlen(s.str) + 1]; strcpy(concat.str, this->str); strcat(concat.str, s.str); return concat; } mystring& mystring::operator+=(const mystring &s){ // temp save "this" string to other mystring other {new char(strlen(this->str) + 1)}; strcpy(other.str, this->str); // allocate space for the concatenated string this->str = new char[strlen(this->str) + strlen(s.str) + 1]; // move "this" and input strings to "this" strcpy(this->str, other.str); strcat(this->str, s.str); return *this; } 

Answer 1

You did a single-null-allocation correctly(ish) once:

if(str == nullptr){ this->str = new char; *(this->str) = '\0'; 

and incorrectly a second time:

if(str == nullptr){ this->str = new char; this->str = '\0'; 

This will produce a memory leak. That aside, if your compiler allowed this without yelling about incompatible types, that makes me sad.

Even then, as @TobySpeight indicates, the first style of allocation has a mismatch with your delete[] and also needs to follow array-like syntax.

Answer 2

The problem with the representation (null-terminated char*, like C) is that it's inefficient for concatenation.

If we use array+length, as is common for strings in many languages (including the C++ standard library), then we don't need to seek to the end for every operation (as we do here, hidden inside strcat() - even though we've already computed the length!). Read about the problems with "Schlemiel the Painter" and his algorithm.

Converting null pointer into empty string is a good idea, avoiding special-case handling throughout the code. But there's a serious bug:

 this->str = new char; 

We need new char[1] there, because new[] and delete[] go together as a pair.

This mismatch would be caught by Valgrind - I really recommend you exercise your code using a memory checker any time you write memory management code (that's every time you use a naked new or new[]; also if you ever use std::malloc() or any C libraries that haven't been wrapped in a RAII wrapper).

Why do we have mystring(mystring&&) but no operator=(mystring&&)? It doesn't make sense to have a move constructor but always force copying for assignment.

Style-wise, I don't like use of this-> all over the place - that's just distracting clutter that can be avoided by giving arguments different names to members.