A custom String class implementation

Question

This is my first implementation of a "container" using the Rule of Five, any feedback I'll gladly accept. I've tried to make it as Exception-Safe as possible. I've made it for an assignment.

Header

#pragma once
#include <ostream>
#include "Vector.h"

class String
{
public:
    String();
    String(char _char);
    String(const char* _string);

    //Copy Constructor
    String(const String& _string);

    //Move Constructor
    String(String&& _string) noexcept;

    ~String();

    void reset();
    size_t length() const;
    char* asCString() const;

    //Cast to const char
    operator const char* () const;

    //Bracket Operator overloads
    char operator[](int _index) const;
    char& operator[](int _index);

    //Compare
    bool operator==(const String& _other) const;
    bool operator==(const char* _chars) const;

    //Copy Assignment Operator
    String& operator=(const String& _other);

    //Move Assignment Operator
    String& operator=(String&& _other) noexcept;

    //Add Operators
    String& operator+=(const String& _other);
    String& operator+=(const char* _other);
    String operator+(const String& _other) const;
    String operator+(const char* _other) const;

    //Stream Operator
    friend std::ostream& operator<<(std::ostream& _os, const String& _string);

    Vector<String> split(const char _delimiter);
    void replace(const char _target, const char _replacement);
    void removeCharacter(char _target);
    String toLowerCase() const;
private:
    unsigned int m_length;
    char* m_string;
};

CPP File

#include "stdafx.h"
#include "String.h"
#include <cstring>
#include <stdexcept>
#include <iostream>
#include <algorithm>

String::String() : m_length(0), m_string()
{
    m_string = new char[1];
    m_string[0] = '\0';
}

String::String(char _char) : m_length(1)
{
    m_string = new char[2];
    m_string[0] = _char;
    m_string[1] = '\0';
}

String::String(const char* _string)
{
    if(!_string)
    {
        m_length = 0;
        m_string = new char[0];
    }
    else
    {
        m_length = strlen(_string);
        m_string = new char[m_length + 1];

        std::copy(_string, _string + m_length, m_string);
        m_string[m_length] = '\0';
    }
}

String::String(const String& _string)
{
    char* buffer = new char[_string.m_length + 1];
    std::copy(_string.m_string, _string.m_string + _string.m_length, buffer);
    buffer[_string.m_length] = '\0';

    m_string = buffer;
    m_length = _string.m_length;
}

String::String(String&& _string) noexcept
{
    const auto buffer = _string.m_string;

    m_length = _string.m_length;
    m_string = buffer;

    _string.reset();
}

String::~String()
{
    delete[] m_string;
    m_string = nullptr;
}

void String::reset()
{
    m_length = 0;
    m_string = nullptr;
}

size_t String::length() const
{
    return m_length;
}

char* String::asCString() const
{
    return m_string;
}

String::operator const char*() const
{
    return m_string;
}

char String::operator[](int _index) const
{
    if(_index < 0 || _index >= m_length)
    {
        throw std::out_of_range("String index out of bounds!");
    }
    return m_string[_index];
}

char& String::operator[](int _index)
{
    if (_index < 0 || _index >= m_length)
    {
        throw std::out_of_range("String index out of bounds!");
    }
    return m_string[_index];
}

bool String::operator==(const String& _other) const
{
    if (!_other.m_string) return false;
    return strcmp(m_string, _other.m_string) == 0;
}

bool String::operator==(const char* _chars) const
{
    if (!_chars) return false;
    return strcmp(m_string, _chars) == 0;
}

String& String::operator=(const String& _other)
{
    if (this != &_other) {
        char* buffer = new char[_other.m_length + 1];
        std::copy(_other.m_string, _other.m_string + _other.m_length + 1, buffer);

        std::swap(buffer, m_string);
        m_length = _other.m_length;

        delete[] buffer;
    }

    return *this;
}

String& String::operator=(String&& _other) noexcept
{
    if (this != &_other) {
        const auto buffer = _other.m_string;

        delete[] m_string;

        m_string = buffer;
        m_length = _other.m_length;

        _other.reset();
    }

    return *this;
}

String& String::operator+=(const String& _other)
{
    if (this != &_other)
    {
        auto totalLength = m_length + _other.m_length;
        char* buffer = new char[totalLength + 1];

        std::copy(m_string, m_string + m_length, buffer);
        std::copy(_other.m_string, _other.m_string + _other.m_length, buffer + m_length);
        buffer[totalLength] = '\0';

        std::swap(m_string, buffer);
        m_length = totalLength;

        delete[] buffer;
    }

    return *this;
}

String& String::operator+=(const char* _other)
{
    const auto charLength = strlen(_other);
    const auto totalLength = m_length + charLength;
    char* buffer = new char[totalLength + 1];

    std::copy(m_string, m_string + m_length, buffer);
    std::copy(_other, _other + charLength, buffer + m_length);
    buffer[totalLength] = '\0';

    std::swap(m_string, buffer);
    m_length = totalLength;

    delete[] buffer;

    return *this;
}

String String::operator+(const String& _other) const
{
    const auto totalLength = m_length + _other.m_length;
    char* buffer = new char[totalLength + 1];

    std::copy(m_string, m_string + m_length, buffer);
    std::copy(_other.m_string, _other.m_string + _other.m_length, buffer + m_length);
    buffer[totalLength] = '\0';

    return String(buffer);
}

String String::operator+(const char* _other) const
{
    const auto charLength = strlen(_other);
    const auto totalLength = m_length + charLength;
    char* buffer = new char[totalLength + 1];

    std::copy(m_string, m_string + m_length, buffer);
    std::copy(_other, _other + charLength, buffer + m_length);
    buffer[totalLength] = '\0';

    return String(buffer);
}

std::ostream& operator<<(std::ostream& _os, const String& _string)
{
    _os << _string.m_string;
    return _os;
}

Vector<String> String::split(const char _delimiter)
{
    Vector<String> result;
    char* t;
    char* pch = strtok_s(m_string, &_delimiter, &t);

    while (pch != nullptr)
    {
        result.push(String{ pch });
        pch = strtok_s(nullptr, &_delimiter, &t);
    }

    return result;
}

void String::replace(const char _target, const char _replacement)
{
    for (int i = 0; i < m_length; i++)
    {
        if (m_string[i] == _target)
        {
            if (_replacement == 0) {
                removeCharacter(_target);
            }
            else
            {
                m_string[i] = _replacement;
            }
        }
    }
}

void String::removeCharacter(char _target)
{
    auto newEnd = std::remove(m_string, m_string + m_length, ' ');
    *newEnd = '\0';
}

String String::toLowerCase() const
{
    String buffer = *this;

    for (int i = 0; i < m_length; i++) {
        buffer[i] = tolower(buffer[i]);
    }

    return buffer;
}

Answer 1

Here's some suggestions:

Header

• #include <ostream> incurs a lot of overhead. You do not need the whole std::ostream in the header. #include <iosfwd> is enough.

• It's std::size_t, not size_t. Also, you forgot to #include <cstddef>.

• asCString fails to propagate const.

• Not being implicitly convertible to const char* is one of the basic benefits of proper containers over raw arrays. Use a named function like c_str instead.

• Consistently use std::size_t for indexes. Right now, you are mixing it with int and unsigned int.

• operator== and operator+ should be non-members.

• operator<< does not need to be a friend; just declare it as a free function.

• Do not add top-level const to function parameters.

• The copy and move assignment operators should be merged into one; see below.

Implementation

• Sort the #includes.

• The constructors are written in a very convoluted way. Use in-class member initializers to make sure you don't accidentally leave the members uninitialized:

  class String {
      // ...
      std::size_t m_length{};
      char* m_string{};
  };
  

  And simplify the constructors:

  String::String()
      : m_length{0}, m_string{new char[1]{}}
  {
  }
  
  String::String(char c)
      : m_length{1}, m_string{new char[2]{c}}
  {
  }
  
  String::String(const String& other)
      : m_length{other.m_length}, m_string{new char[m_length + 1]}
  {
      std::copy_n(other.m_string, m_length, m_string);
      m_string[m_length] = '\0';
  }
  

• Use copy-and-swap:

  String::String(String&& other) noexcept
      : String{}
  {
      swap(*this, other);
  }
  
  // take by value
  String& operator=(String other) noexcept
  {
      swap(*this, other);
      return *this;
  }
  
  // in the class body
  friend void swap(String& lhs, String& rhs) noexcept
  {
      using std::swap;
      swap(lhs.m_length, rhs.m_length);
      swap(lhs.m_string, rhs.m_string);
  }
  

• The destructor should not set the pointer to null after deleteing it.

• Currently, there are two "empty" states: m_length = 0, m_string = 0 and m_length = 1, m_string = new char[1]{}. The first is the moved-from state, whereas the second is the empty string. Moved-from objects are placed in a "valid but otherwise unspecified state," so the only operations they should support are destruction and assignment. Therefore:

  • If you want to support String{nullptr}, then it should be an empty string. Otherwise, just require a non-null pointer:

    String::String(const char* str)
        : m_length{std::strlen(str)} // note: 'std::strlen' not 'strlen'
        , m_string{new char[m_length]}
    {
        std::copy_n(str, m_length + 1, m_string); // use 'std::copy_n'; also copy the '\0'
    }
    

  • If I understand correctly, reset means clear. Then, it should result in an empty string.

  • Don't check for if (!_other.m_string).

• strcmp is not efficient when you know the length of both strings. Use std::equal instead:

  // non-member operator==, requires appropriate declaration
  bool operator==(const String& lhs, const String& rhs) noexcept
  {
      // see below for begin and end
      return std::equal(lhs.begin(), lhs.end(), rhs.begin(), rhs.end());
  }
  
  bool operator!=(const String& lhs, const String& rhs) noexcept
  {
      return !(lhs == rhs);
  }
  

• The way operator+= handles self assignment is wrong — str += str is obviously not a no-op. In fact, this is one of the rare cases where I would suggest that operator+= delegate to operator+:

  // friend operator+, requires appropriate declaration
  friend String operator+(const String& lhs, const String& rhs)
  {
      String result;
      result.m_length = lhs.m_length + rhs.m_length;
      result.m_string = new char[result.m_length + 1];
  
      auto p = std::copy_n(lhs.m_string, lhs.m_length, result.m_length);
      std::copy_n(rhs.m_string, rhs.m_length + 1, p); // copy the '\0'
  
      return result;
  }
  
  String& String::operator+=(const String& other)
  {
      *this = *this + other;
      return *this;
  }
  

Answer 2

I. You don't reserve space. Imagine a situation when you need to append a vector of strings of known sizes. You reallocate buffer, and copy its current contents together with appended suffix for each of those. Thus, if you add N small strings of size m each, the whole algorithm runs in N², possibly leaving the heap fragmented heavily.

One typical strategy would be to reserve space growing logarithmically; this makes appending amortized linear.

Even without large amount of rhs operands, when you append a small string to a large one 99% of method runtime is spent on copying the lhs contents.

II. Member operator+ probably deserves ref-qualification. operator+() const && can simply reuse this object's space (well, if it's reserved).

III. Note that remove_character can effectively reduce string's length in the meantime leaving it in the same buffer (so you get an implicit reserved space you can't even benefit from).

IV. reset() makes the buffer leak. If it was introduced for the sole purpose of being used in move construction/assignment it should be made private.

V. You're using functions for trailing zero strings so don't consider cases when Strings have zero characters inside: in your current implementation, String("123\000abc") is equal to String("123\000def"). If it is as intended, perhaps a check in the ctor would make sense.

Bonus. Identifiers starting with underscores. Not that it was illegal but personally I wouldn't name them this way.

Answer 3

1. Use std::string_view to simplify your code, avoid expensive temporaries, and to make your custom string-type more usable.

2. You have three empty states: (0, nullptr), (0, length-zero-array), (1, length-one-array). Standardize to simplify.

   One option to remove special cases and still have noexcept-move-ctor would be using (0, static empty string) for all empty strings.

3. Wherever you don't have a good reason to deviate from it, keep to the interface of std::string. Demonstrating your individuality here just hurts everyone.

4. Implicitly casting to pointer to constant string is inadvisable. Provide the standard accessors for that instead (.begin(), .cbegin(), .data(), .cdata(), .c_str(), and so on. Yes, a lot of them are redundant, though were not when introduced, or had to be added anyway for conformance to common interfaces.

5. If there is no good reason to make something a member, desist. The same for friend. Encapsulation works best when it is actually followed where it's easy enough.

6. Using in-class-initializers, ctor-init-list and delegation allows you to simplify your code and avoid errors.

7. Range-checking is unexpectedly expensive for .operator[](). That's what .at() is for.

8. Make self-assignment safe without explicit check. Yes, that case will be more expensive, but pessimizing the common path really hurts.

9. If you copy a null terminated string, and the result should also be null terminated, copy the terminator too. Setting it separately afterwards is worse in any respect.

10. Your implementation of .replace() is quadratic when the replacement is \0. It should be linear. Anyway, why is your string-type not zero-clean?

11. Best get into the habit of preferring ++var over var++ when you don't need the value. It is more efficient for more complex types.