2
\$\begingroup\$

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;
}
\$\endgroup\$

2 Answers 2

2
\$\begingroup\$

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.

\$\endgroup\$
2
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.