3
\$\begingroup\$

I wrote a simple String class implementation using unique_ptr and move semantics. Is my implementation good enough in terms of design and efficiency?

Stringy.h

#pragma once

#include <iostream>
#include <cstring>
#include <memory>

class Stringy {
private:
    std::unique_ptr<char[]> p;
    std::size_t size;
    std::size_t len;

    enum {Factor = 3};

    static void _debug(const char *x)
    {
    #if DEBUG
        std::cout << "=> " << x << " <=" << std::endl;
    #endif
    }

public:

    Stringy(const char* ptr);
    Stringy(const Stringy& Other);
    Stringy(Stringy&& Other);

    const Stringy& operator=(Stringy Other);
    const Stringy& operator=(Stringy&& Other);

    void swap(Stringy& Other);

    void concat(const Stringy& Other);
    void concat(const char* One);

    void move(Stringy& Other);

    std::size_t length() const;

    Stringy operator+(const Stringy& Other) const;
    Stringy operator+(const char* ptr) const;

    const char& operator[](std::size_t index) const;
    char& operator[](std::size_t index);

    friend Stringy&& operator+ (Stringy&& Lhs, const char* Rhs);
    friend Stringy&& operator+ (Stringy&& Lhs, const Stringy& Rhs);
    friend std::ostream& operator<< (std::ostream& cout, const Stringy& Other);

    ~Stringy();
};

Stringy.cpp

#define DEBUG false

Stringy::Stringy(const char *ptr)
{
    _debug("default pointer Ctor");
    len = strlen(ptr);
    size = len * Factor;
    p = std::make_unique<char[]>(size);
    std::copy(ptr, ptr + len, p.get());
    p[len] = '\0';
}

Stringy::Stringy(const Stringy &Other): size(Other.size), len(0)
{
    _debug("Copy Ctor");
    p = std::make_unique<char[]>(size);
    concat(Other);
}

Stringy::Stringy(Stringy &&Other)
{
    _debug("Move Ctor");
    move(Other);
}

const Stringy& Stringy::operator=(Stringy Other)
{
    _debug("opterator= copy and swap");
    swap(Other);
    return *this;
}

const Stringy& Stringy::operator=(Stringy &&Other)
{
    _debug("Move operator=");
    move(Other);
    return *this;
}

void Stringy::swap(Stringy &Other)
{
    std::swap(size, Other.size);
    std::swap(len, Other.len);
    std::swap(p, Other.p);
}

//TODO: Reallocate memory if required
void Stringy::concat(const Stringy &Other)
{
    std::copy(Other.p.get(), Other.p.get() + Other.len, p.get() + len);
    len += Other.len;
    p[len] = '\0';
}

//TODO: Reallocate memory if required
void Stringy::concat(const char *One)
{
    std::size_t _len = strlen(One);
    std::copy(One, One + _len, p.get() + len);
    len += _len;
    p[len] = '\0';
}

void Stringy::move(Stringy &Other)
{
    size = Other.size;
    Other.size = 0;

    len = Other.len;
    Other.len = 0;

    p = std::move(Other.p);
    Other.p = NULL;
}

std::size_t Stringy::length() const
{
    return len;
}

Stringy Stringy::operator+(const Stringy &Other) const
{
    _debug("operator+ Other");
    Stringy temp(*this);
    temp.concat(Other);
    return temp;
}

Stringy Stringy::operator+(const char *ptr) const
{
    _debug("operator+ ptr");
    Stringy temp(*this);
    temp.concat(ptr);
    return temp;
}

const char& Stringy::operator[](std::size_t index) const
{
    return this->p[index];
}

char& Stringy::operator[](std::size_t index)
{
    return this->p[index];
}

Stringy::~Stringy()
{
    _debug("Dtor");
}

Stringy&& operator+(Stringy &&Lhs, const char *Rhs)
{
    Stringy::_debug("operator+ move ptr");
    Lhs.concat(Rhs);
    return std::move(Lhs);
}

Stringy&& operator+(Stringy &&Lhs, const Stringy &Rhs)
{
    Stringy::_debug("operator+ move Other");
    Lhs.concat(Rhs);
    return std::move(Lhs);
}

std::ostream &operator<<(std::ostream &stream, const Stringy &Other)
{
    stream << Other.p.get();
    return stream;
}

I benchmarked the class against std::string on QuickBench and it shows 2.5x worse performance, but that's because gcc now uses SSO. With large string size, my class performed comparably to std::string although I would agree they are little biased.

main.cpp

#include <iostream>
#include "Stringy.h"

void custom() {
    Stringy firstName = "Sumit";
    Stringy lastName = "Dhingra";
    Stringy fullName = firstName + "-" + ">" + lastName;

    std::cout << fullName << std::endl;
}

void default_() {
    std::string firstName = "Sumit";
    std::string lastName = "Dhingra";
    std::string fullName = firstName + "-" + ">" + lastName;

    std::cout << fullName << std::endl;
}

int main() {
#ifdef DEFAULT
    default_();
#else
    custom();
#endif
}
\$\endgroup\$
  • 1
    \$\begingroup\$ SSO can mean single sign-on, structure sequence and organization and a boat load of other things. Which do you mean? \$\endgroup\$ – Mast Feb 24 at 13:11
  • 1
    \$\begingroup\$ Short String Optimization. Sorry for not clearing it up. \$\endgroup\$ – Sumit Dhingra Feb 24 at 16:32
  • \$\begingroup\$ stringy.cpp is obviously incomplete. \$\endgroup\$ – Deduplicator Feb 24 at 20:29
2
\$\begingroup\$
  1. private: is redundant: While union and struct default to public, class already defaults to private.

  2. Calling the capacity size is seriously counter-intuitive. std::string has .length() as a legacy-method which is equal to .size().

  3. A factor of two or more when re-allocating is seriously sub-optimal: It inhibits re-use of returned memory-blocks when expanding bit by bit.

  4. Unconditionally over-allocating on creation is in general a serious pessimisation.

  5. Why does _debug() use cout instead of cerr?

  6. Any reason you use your own macro, instead of re-using assert()'s NDEBUG?

  7. It's curious that assignment accepts an rvalue-reference or a copy, but not a constant reference. Also consider accepting a const char* like the ctor does.

  8. Construction from const char* will write out-of-bounds if passed a 0-length string.

  9. Why write the nul-terminator in an extra step when you can simply copy that too?

  10. std::copy_n seems to fit your use better than std::copy().

  11. Not doing the move in the move-ctor, but delegating to a separate method called move(), is seriously weird.

  12. += is the operator for concatenation. Why do you call it concat()?

  13. Consider consolidating the methods accepting a non-modifiable range to one using iterators. Excepting those which might use the fact that the source-range is actually already nul-terminated.

  14. You can restrict member-functions to only be called on xvalues:

    friend Stringy&& operator+ (const char* Other) &&;
    friend Stringy&& operator+ (const Stringy& Other) &&;
    

    If you are at it, consider doing the same for +=.

  15. If a function does not need to have the access, don't declare it a friend.

  16. Take advantage of templates to run the same code with std::string and your own Stringy.

\$\endgroup\$
  • \$\begingroup\$ clog is more appropriate than cerr for debugging messages, IMO \$\endgroup\$ – Toby Speight Feb 25 at 13:49

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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