2
\$\begingroup\$

I'm looking to get some feedback on my attempted string implementation. The code is just meant for me to play around with and work on some C++ to try and improve. A lot of the things done here (rule of 4 and 1/2, auto return values, const methods, etc) are features of C++ I don't have a lot or any experience with and I want to get used to them and understand when to use them.

class string
{
public:
    explicit string(const char *array = nullptr) :
        cString(array ? new char[size]: nullptr), size(array ? strlen(array) + 1: 0), capacity(size)
    {
        std::copy(array, array + size, cString);
    }

    string (string& other) : 
        cString(other.cString ? new char[size] : nullptr), size(other.cString ? strlen(other.cString) + 1 : 0), capacity(size)
    {
        std::copy(other.cString, other.cString + other.size, cString);
    }

    string(string&& other) noexcept : string()
    {
        swap(*this, other);
    }

    string& operator=(string& other) noexcept
    {
        swap(*this, other);

        return *this;
    }

    friend std::ostream& operator<<(std::ostream& output, const string& str) noexcept
    {
        output << str.cString;

        return output;
    }

    inline bool operator==(const string& str) const noexcept
    {
        return strcmp(cString, str.cString) == 0;
    }

    inline char operator[](const std::size_t index) const
    {
        if (index > size) {
            throw std::invalid_argument("Index out of bounds");
        }

        return *(cString + index);
    }

    inline bool operator!=(const string& str) const noexcept
    {
        return !(*this == str);
    }

    string& operator +=(const char *str)
    {
        if (!str) {
            throw std::invalid_argument("Null pointer");
        }

        std::size_t stringLen = strlen(str);

        if (size + stringLen > capacity) {
            std::size_t newSize = (size + stringLen + capacity) * 2;
            char *newBuffer = new char[newSize];

            std::copy(cString, cString + size - 1, newBuffer);
            std::copy(str, str + stringLen + 1, newBuffer + size - 1);

            delete[] cString;
            cString = newBuffer;
            capacity = newSize;
        } else {
            std::copy(cString, cString + size - 1, cString);
            std::copy(str, str + stringLen + 1, cString + size - 1);
        }

        size = size + stringLen;

        return *this;
    }

    friend void swap(string& first, string& second) noexcept
    {
        using std::swap;

        swap(first.size, second.size);
        swap(first.cString, second.cString);
    }

    void clear() noexcept
    {
        memset(cString, 0, size);
        size = 0;
    }

    static void copy(const string& src, string& dst)
    {
        if (src.capacity > dst.capacity)
        {
            char *newBuffer = new char[src.size];

            std::copy(src.cString, src.cString + src.size, newBuffer);

            delete[] dst.cString;
            dst.cString = newBuffer;
            dst.size = src.size;
        } else {
            std::copy(src.cString, src.cString + src.size, dst.cString);
        }

    }

    void replace(const char oldChar, const char newChar) const
    {
        for(std::size_t i = 0; i < size - 1; i++) {
            if (*(cString + i) == oldChar) {
                *(cString + i) = newChar;
            }
        }
    }

    decltype(auto) hash() const noexcept
    {
        unsigned long value = 0;
        int character;

        for (std::size_t i = 0; i < size - 1; i++) {
            character = *(cString + i);
            value = character + (value << 6) + (value << 16) - value;
        }

        return value;
    }

    inline bool isEmpty() const noexcept
    {
        return size == 0;
    }

    inline char front() const noexcept
    {
        return *cString;
    }

    inline char back() const noexcept
    {
        return *(cString + size - 2);
    }

    inline char *getArray() const noexcept
    {
        return cString;
    }

    inline decltype(auto) getSize() const noexcept
    {
        return size;
    }

    inline decltype(auto) getCapacity() const noexcept
    {
        return capacity;
    }

    ~string()
    {
        delete [] cString;
    }
private:
    std::size_t size;
    std::size_t capacity;
    char *cString;
};
\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! To clarify, can you give a bit more detail about what your code does or is for, exactly? Is it meant to replicate std::string to some extent, or is it something completely different? \$\endgroup\$ – Phrancis Aug 22 '16 at 0:57
  • \$\begingroup\$ Thanks! The code is just meant for me to play around with and work on some c++ to try and improve. A lot of the things done here (rule of 4 and 1/2, auto return values, const methods, etc) are features of c++ I don't have a lot or any experience with and I want to get used to them and understand when to use them. \$\endgroup\$ – FrozenHawk Aug 22 '16 at 1:08
  • 2
    \$\begingroup\$ May I suggest that you edit your post to add some of this information, then? As it stands it's kind of vague. This will help reviewers give you better answers. \$\endgroup\$ – Phrancis Aug 22 '16 at 1:13
1
\$\begingroup\$
  1. Counting the 0-terminator as part of the string's length is a very questionable design-decision, considering it is quite unique and has interesting consequences for joining strings.
  2. There are null strings and empty strings with the current design, which is a nice source of errors.
  3. The copy-ctor should accept a constant reference. Also, it should not recalculate the sources size instead of just copying it.
  4. The assignment-operator is broken, as it does a swap. The quick and easy fix would be simply accepting the argument by value.
  5. The stream-inserter should use iostream.write for performance and to handle internal 0-bytes.
  6. operator== should properly handle null strings, or preferably they should be eliminated.
  7. operator[] must throw if the index equals the size, because of the curious decision to count the terminator too.
  8. operator+= has a spurious copy string on itself in each branch. Also, there should be the full complement of operators, especially also accepting your custom string-type itself.
  9. swap is broken because you fail to swap capacities.
  10. clear is wasteful, and creates a chimera of null and empty string.
  11. copy is useless, just use the copy-ctor.
  12. You know we have std::replace?
  13. Only use return-type-deduction with decltype(auto) instead of auto if you want to return a reference. A tip, it's an error everywhere you used it.
\$\endgroup\$
  • \$\begingroup\$ Thanks for the feedback. Could you expand on why return-type-deduction should only be used to return a reference? \$\endgroup\$ – FrozenHawk Aug 23 '16 at 14:20
  • \$\begingroup\$ @FrozenHawk There are two possibilities for type deduction: auto (never a reference) and decltype(auto) (can be a reference). You want the former. \$\endgroup\$ – Deduplicator Aug 23 '16 at 14:39
1
\$\begingroup\$

Subscript operator

Your range-checking allows access to one character beyond the string's buffer. I think you want if (index >= size). Also, when using the subscript operator, I think it is expected you are getting a reference into the container. So it should return (in your example) a char const&. You should also define a non-const version when indexing into a non-const string

char& operator[] (

Assignment Operator

Usually the assignment operator promises not to modify the source:

string& operator=(string const&) noexcept;

Your assignment operator changes the value of what was assigned. This will confuse your users.

\$\endgroup\$

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.