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.